[PATCH v3] arm: Fix memory attribute inconsistencies when using fixmap

[Ard Biesheuvel]

On 4 April 2017 at 09:48, Jon Medhurst (Tixy) <tixy at linaro.org> wrote:
> On Tue, 2017-04-04 at 08:31 +0100, Ard Biesheuvel wrote:
>> From: Jon Medhurst <tixy at linaro.org>
>>
>> To cope with the variety in ARM architectures and configurations, the
>> pagetable attributes for kernel memory are generated at runtime to match
>> the system the kernel finds itself on. This calculated value is stored
>> in pgprot_kernel.
>>
>> However, when early fixmap support was added for ARM (commit
>> a5f4c561b3b1) the attributes used for mappings were hard coded because
>> pgprot_kernel is not set up early enough. Unfortunately, when fixmap is
>> used after early boot this means the memory being mapped can have
>> different attributes to existing mappings, potentially leading to
>> unpredictable behaviour. A specific problem also exists due to the hard
>> coded values not include the 'shareable' attribute which means on
>> systems where this matters (e.g. those with multiple CPU clusters) the
>> cache contents for a memory location can become inconsistent between
>> CPUs.
>>
>> To resolve these issues we change fixmap to use the same memory
>> attributes (from pgprot_kernel) that the rest of the kernel uses. To
>> enable this we need to refactor the initialisation code so
>> build_mem_type_table() is called early enough. Note, that relies on early
>> param parsing for memory type overrides passed via the kernel command
>> line, so we need to make sure this call is still after
>> parse_early_params().
>>
>> Fixes: a5f4c561b3b1 ("ARM: 8415/1: early fixmap support for earlycon")
>> Cc: <stable at vger.kernel.org> # v4.3+
>> Signed-off-by: Jon Medhurst <tixy at linaro.org>
>> [ardb: keep early_fixmap_init() before param parsing, for earlycon]
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>
>> ---
> [...]
>> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
>> index 4e016d7f37b3..ec81a30479aa 100644
>> --- a/arch/arm/mm/mmu.c
>> +++ b/arch/arm/mm/mmu.c
>> @@ -414,6 +414,11 @@ void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot)
>>                    FIXADDR_END);
>>       BUG_ON(idx >= __end_of_fixed_addresses);
>>
>> +     /* we only support device mappings until pgprot_kernel has been set */
>> +     if (WARN_ON(pgprot_val(prot) != pgprot_val(FIXMAP_PAGE_IO) &&
>> +                 pgprot_val(pgprot_kernel) == 0))
>> +             return;
>> +
>
> Hmm, on return from this function, isn't the caller then going to try
> and access the memory at the fixmap address we didn't map, and so get a
> page fault? If so, would a BUG_ON here be better?
>

Yes, and no. BUG_ON guarantees a crash, while without it, it's only
highly likely. Since this will occur very early during boot, it is
better to limp on if we can than crash unconditionally.


>>       if (pgprot_val(prot))
>>               set_pte_at(NULL, vaddr, pte,
>>                       pfn_pte(phys >> PAGE_SHIFT, prot));
>> @@ -1616,7 +1621,6 @@ void __init paging_init(const struct machine_desc *mdesc)
>>  {
>>       void *zero_page;
>>
>> -     build_mem_type_table();
>>       prepare_page_table();
>>       map_lowmem();
>>       memblock_set_current_limit(arm_lowmem_limit);
>> @@ -1636,3 +1640,9 @@ void __init paging_init(const struct machine_desc *mdesc)
>>       empty_zero_page = virt_to_page(zero_page);
>>       __flush_dcache_page(NULL, empty_zero_page);
>>  }
>> +
>> +void __init early_mm_init(const struct machine_desc *mdesc)
>> +{
>> +     build_mem_type_table();
>> +     early_paging_init(mdesc);
>> +}
>
> I tested this change with kprobes tests on TC2 (the setup that showed
> the original bug) and compile tested for a nommu device (I will boot
> test that in a bit as that's not quick to setup).
>
> Also, with this patch we can now make early_paging_init() static now,
> and remove the extern...
>
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index f5db5548ad42..8ad1e4414024 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -80,7 +80,6 @@ __setup("fpe=", fpe_setup);
>
>  extern void init_default_cache_policy(unsigned long);
>  extern void paging_init(const struct machine_desc *desc);
> -extern void early_paging_init(const struct machine_desc *);
>  extern void adjust_lowmem_bounds(void);
>  extern enum reboot_mode reboot_mode;
>  extern void setup_dma_zone(const struct machine_desc *desc);
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index ec81a30479aa..347cca965783 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -1497,7 +1497,7 @@ pgtables_remap lpae_pgtables_remap_asm;
>   * early_paging_init() recreates boot time page table setup, allowing machines
>   * to switch over to a high (>4G) address space on LPAE systems
>   */
> -void __init early_paging_init(const struct machine_desc *mdesc)
> +static void __init early_paging_init(const struct machine_desc *mdesc)
>  {
>         pgtables_remap *lpae_pgtables_remap;
>         unsigned long pa_pgd;
> @@ -1565,7 +1565,7 @@ void __init early_paging_init(const struct machine_desc *mdesc)
>
>  #else
>
> -void __init early_paging_init(const struct machine_desc *mdesc)
> +static void __init early_paging_init(const struct machine_desc *mdesc)
>  {
>         long long offset;
>

Good point. If everything checks out, we should fold that in when
submitting it to the patch tracker.