[PATCH 0/2] arm64: mm: use fully constructed struct pages from EFI page tables

[PATCH 0/2] arm64: mm: use fully constructed struct pages from EFI page tables

[Ard Biesheuvel]

As reported by Sudeep, the EFI memory attributes table handling may crash
on an uninitialized spinlock in the struct page associated with a UEFI runtime
translation table page.

This is caused by a failure to take into account the fact that generic code
treats such pages differently depending on whether they are associated with
init_mm or not. The EFI page tables are completely separate from both the
kernel and the userland mappings, and are completely static during the
lifetime of the OS, but still, the most robust solution is to simply
construct these pages fully, so that generic code that compares against
&init_mm can work safely and correctly.

Patch #1 is a preparatory patch to drop a reference to late_pgtable_alloc
from code that no longer needs it.

Patch #2 renamed late_pgtable_alloc to pgd_table_alloc, and updates it to
construct the struct page associated with the allocated page.

Ard Biesheuvel (2):
  arm64: mm: make create_mapping_late() non-allocating
  arm64: mm: run pgtable_page_ctor() on non-swapper translation table
    pages

 arch/arm64/mm/mmu.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH 1/2] arm64: mm: make create_mapping_late() non-allocating

[Ard Biesheuvel]

The only purpose served by create_mapping_late() is to remap the already
mapped .text and .rodata kernel segments with read-only permissions. Since
we no longer allow block mappings to be split or merged,
create_mapping_late() should not pass an allocation function pointer into
__create_pgd_mapping(). So pass NULL instead.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/mm/mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index a96a2413fa18..33f36cede02d 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -312,7 +312,7 @@ static void create_mapping_late(phys_addr_t phys, unsigned long virt,
 	}
 
 	__create_pgd_mapping(init_mm.pgd, phys, virt, size, prot,
-			     late_pgtable_alloc, !debug_pagealloc_enabled());
+			     NULL, !debug_pagealloc_enabled());
 }
 
 static void __init __map_memblock(pgd_t *pgd, phys_addr_t start, phys_addr_t end)
-- 
2.7.4

[PATCH 1/2] arm64: mm: make create_mapping_late() non-allocating

[Mark Rutland]

Hi Ard,

On Fri, Jul 22, 2016 at 07:32:24PM +0200, Ard Biesheuvel wrote:
> The only purpose served by create_mapping_late() is to remap the already
> mapped .text and .rodata kernel segments with read-only permissions. Since
> we no longer allow block mappings to be split or merged,
> create_mapping_late() should not pass an allocation function pointer into
> __create_pgd_mapping(). So pass NULL instead.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/mm/mmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index a96a2413fa18..33f36cede02d 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -312,7 +312,7 @@ static void create_mapping_late(phys_addr_t phys, unsigned long virt,
>  	}
>  
>  	__create_pgd_mapping(init_mm.pgd, phys, virt, size, prot,
> -			     late_pgtable_alloc, !debug_pagealloc_enabled());
> +			     NULL, !debug_pagealloc_enabled());
>  }

How about we drop the __init marker from create_mapping_noalloc(), and
update the callers in mark_rodata_ro() to use that, so that we can drop
create_mapping_late() entirely.

Other than the __init marker and name, create_mapping_late() and
create_mapping_noalloc() would be identical after this change, and the
naming of create_mapping_noalloc() does better describe what we're
trying to achieve in mark_rodata_ro().

Thanks,
Mark.

[PATCH 1/2] arm64: mm: make create_mapping_late() non-allocating

[Ard Biesheuvel]

> On 25 jul. 2016, at 15:46, Mark Rutland <mark.rutland@arm.com> wrote:
> 
> Hi Ard,
> 
>> On Fri, Jul 22, 2016 at 07:32:24PM +0200, Ard Biesheuvel wrote:
>> The only purpose served by create_mapping_late() is to remap the already
>> mapped .text and .rodata kernel segments with read-only permissions. Since
>> we no longer allow block mappings to be split or merged,
>> create_mapping_late() should not pass an allocation function pointer into
>> __create_pgd_mapping(). So pass NULL instead.
>> 
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> arch/arm64/mm/mmu.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index a96a2413fa18..33f36cede02d 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -312,7 +312,7 @@ static void create_mapping_late(phys_addr_t phys, unsigned long virt,
>>    }
>> 
>>    __create_pgd_mapping(init_mm.pgd, phys, virt, size, prot,
>> -                 late_pgtable_alloc, !debug_pagealloc_enabled());
>> +                 NULL, !debug_pagealloc_enabled());
>> }
> 
> How about we drop the __init marker from create_mapping_noalloc(), and
> update the callers in mark_rodata_ro() to use that, so that we can drop
> create_mapping_late() entirely.
> 
> Other than the __init marker and name, create_mapping_late() and
> create_mapping_noalloc() would be identical after this change, and the
> naming of create_mapping_noalloc() does better describe what we're
> trying to achieve in mark_rodata_ro().
> 

Not entirely. The _late() variant should map down to pages if the first mapping was mapped down to pages as well. The _noalloc() variant may end attempting to merge regions

[PATCH 1/2] arm64: mm: make create_mapping_late() non-allocating

[Mark Rutland]

On Mon, Jul 25, 2016 at 04:10:50PM +0200, Ard Biesheuvel wrote:
> > On 25 jul. 2016, at 15:46, Mark Rutland <mark.rutland@arm.com> wrote:
> > How about we drop the __init marker from create_mapping_noalloc(), and
> > update the callers in mark_rodata_ro() to use that, so that we can drop
> > create_mapping_late() entirely.
> > 
> > Other than the __init marker and name, create_mapping_late() and
> > create_mapping_noalloc() would be identical after this change, and the
> > naming of create_mapping_noalloc() does better describe what we're
> > trying to achieve in mark_rodata_ro().
> 
> Not entirely. The _late() variant should map down to pages if the
> first mapping was mapped down to pages as well. The _noalloc() variant
> may end attempting to merge regions

Ah, I was comparing against mainline rather than the arm64 for-next/core
branch with the recent rework. Sorry for the noise.

Given that, FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

[PATCH 2/2] arm64: mm: run pgtable_page_ctor() on non-swapper translation table pages

[Ard Biesheuvel]

The kernel page table creation routines are accessible to other subsystems
(e.g., EFI) via the create_pgd_mapping() entry point, which allows mappings
to be created that are not covered by init_mm.

Since generic code such as apply_to_page_range() may expect translation
table pages that are not associated with init_mm to be covered by fully
constructed struct pages, add a call to pgtable_page_ctor() in the alloc
function used by create_pgd_mapping. Since it is no longer used by
create_mapping_late(), also update the name of this function to better
reflect its purpose.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/mm/mmu.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 33f36cede02d..51a558195bb9 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -268,10 +268,11 @@ static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
 	} while (pgd++, addr = next, addr != end);
 }
 
-static phys_addr_t late_pgtable_alloc(void)
+static phys_addr_t pgd_pgtable_alloc(void)
 {
 	void *ptr = (void *)__get_free_page(PGALLOC_GFP);
-	BUG_ON(!ptr);
+	if (!ptr || !pgtable_page_ctor(virt_to_page(ptr)))
+		BUG();
 
 	/* Ensure the zeroed page is visible to the page table walker */
 	dsb(ishst);
@@ -298,8 +299,10 @@ void __init create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
 			       unsigned long virt, phys_addr_t size,
 			       pgprot_t prot, bool allow_block_mappings)
 {
+	BUG_ON(mm == &init_mm);
+
 	__create_pgd_mapping(mm->pgd, phys, virt, size, prot,
-			     late_pgtable_alloc, allow_block_mappings);
+			     pgd_pgtable_alloc, allow_block_mappings);
 }
 
 static void create_mapping_late(phys_addr_t phys, unsigned long virt,
-- 
2.7.4

[PATCH 2/2] arm64: mm: run pgtable_page_ctor() on non-swapper translation table pages

[Suzuki K Poulose]

On 22/07/16 18:32, Ard Biesheuvel wrote:
> The kernel page table creation routines are accessible to other subsystems
> (e.g., EFI) via the create_pgd_mapping() entry point, which allows mappings
> to be created that are not covered by init_mm.
>
> Since generic code such as apply_to_page_range() may expect translation
> table pages that are not associated with init_mm to be covered by fully
> constructed struct pages, add a call to pgtable_page_ctor() in the alloc
> function used by create_pgd_mapping. Since it is no longer used by
> create_mapping_late(), also update the name of this function to better
> reflect its purpose.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/mm/mmu.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 33f36cede02d..51a558195bb9 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -268,10 +268,11 @@ static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
>  	} while (pgd++, addr = next, addr != end);
>  }
>
> -static phys_addr_t late_pgtable_alloc(void)
> +static phys_addr_t pgd_pgtable_alloc(void)
>  {
>  	void *ptr = (void *)__get_free_page(PGALLOC_GFP);
> -	BUG_ON(!ptr);
> +	if (!ptr || !pgtable_page_ctor(virt_to_page(ptr)))

Should we free the page and return NULL when we encounter an error
in pgtable_page_ctor(), like the rest of the callers ?


> +		BUG();

Otherwise, looks good to me.

Suzuki

[PATCH 2/2] arm64: mm: run pgtable_page_ctor() on non-swapper translation table pages

[Suzuki K Poulose]

On 25/07/16 15:31, Suzuki K Poulose wrote:
> On 22/07/16 18:32, Ard Biesheuvel wrote:
>> The kernel page table creation routines are accessible to other subsystems
>> (e.g., EFI) via the create_pgd_mapping() entry point, which allows mappings
>> to be created that are not covered by init_mm.
>>
>> Since generic code such as apply_to_page_range() may expect translation
>> table pages that are not associated with init_mm to be covered by fully
>> constructed struct pages, add a call to pgtable_page_ctor() in the alloc
>> function used by create_pgd_mapping. Since it is no longer used by
>> create_mapping_late(), also update the name of this function to better
>> reflect its purpose.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm64/mm/mmu.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 33f36cede02d..51a558195bb9 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -268,10 +268,11 @@ static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
>>      } while (pgd++, addr = next, addr != end);
>>  }
>>
>> -static phys_addr_t late_pgtable_alloc(void)
>> +static phys_addr_t pgd_pgtable_alloc(void)
>>  {
>>      void *ptr = (void *)__get_free_page(PGALLOC_GFP);
>> -    BUG_ON(!ptr);
>> +    if (!ptr || !pgtable_page_ctor(virt_to_page(ptr)))
>
> Should we free the page and return NULL when we encounter an error
> in pgtable_page_ctor(), like the rest of the callers ?
>
>
>> +        BUG();
>

Just to clarify, of course with the BUG() retained.

Suzuki

[PATCH 2/2] arm64: mm: run pgtable_page_ctor() on non-swapper translation table pages

[Catalin Marinas]

On Mon, Jul 25, 2016 at 03:43:21PM +0100, Suzuki K. Poulose wrote:
> On 25/07/16 15:31, Suzuki K Poulose wrote:
> >On 22/07/16 18:32, Ard Biesheuvel wrote:
> >>The kernel page table creation routines are accessible to other subsystems
> >>(e.g., EFI) via the create_pgd_mapping() entry point, which allows mappings
> >>to be created that are not covered by init_mm.
> >>
> >>Since generic code such as apply_to_page_range() may expect translation
> >>table pages that are not associated with init_mm to be covered by fully
> >>constructed struct pages, add a call to pgtable_page_ctor() in the alloc
> >>function used by create_pgd_mapping. Since it is no longer used by
> >>create_mapping_late(), also update the name of this function to better
> >>reflect its purpose.
> >>
> >>Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>---
> >> arch/arm64/mm/mmu.c | 9 ++++++---
> >> 1 file changed, 6 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >>index 33f36cede02d..51a558195bb9 100644
> >>--- a/arch/arm64/mm/mmu.c
> >>+++ b/arch/arm64/mm/mmu.c
> >>@@ -268,10 +268,11 @@ static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
> >>     } while (pgd++, addr = next, addr != end);
> >> }
> >>
> >>-static phys_addr_t late_pgtable_alloc(void)
> >>+static phys_addr_t pgd_pgtable_alloc(void)
> >> {
> >>     void *ptr = (void *)__get_free_page(PGALLOC_GFP);
> >>-    BUG_ON(!ptr);
> >>+    if (!ptr || !pgtable_page_ctor(virt_to_page(ptr)))
> >
> >Should we free the page and return NULL when we encounter an error
> >in pgtable_page_ctor(), like the rest of the callers ?
> >
> >
> >>+        BUG();
> 
> Just to clarify, of course with the BUG() retained.

But then, if we return, we no longer trigger BUG(). The callers of this
function assume that it always succeeds or does not return.

-- 
Catalin

[PATCH 2/2] arm64: mm: run pgtable_page_ctor() on non-swapper translation table pages

[Suzuki K Poulose]

On 25/07/16 17:46, Catalin Marinas wrote:
> On Mon, Jul 25, 2016 at 03:43:21PM +0100, Suzuki K. Poulose wrote:
>> On 25/07/16 15:31, Suzuki K Poulose wrote:
>>> On 22/07/16 18:32, Ard Biesheuvel wrote:
>>>> The kernel page table creation routines are accessible to other subsystems
>>>> (e.g., EFI) via the create_pgd_mapping() entry point, which allows mappings
>>>> to be created that are not covered by init_mm.
>>>>
>>>> Since generic code such as apply_to_page_range() may expect translation
>>>> table pages that are not associated with init_mm to be covered by fully
>>>> constructed struct pages, add a call to pgtable_page_ctor() in the alloc
>>>> function used by create_pgd_mapping. Since it is no longer used by
>>>> create_mapping_late(), also update the name of this function to better
>>>> reflect its purpose.
>>>>
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> ---
>>>> arch/arm64/mm/mmu.c | 9 ++++++---
>>>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>>>> index 33f36cede02d..51a558195bb9 100644
>>>> --- a/arch/arm64/mm/mmu.c
>>>> +++ b/arch/arm64/mm/mmu.c
>>>> @@ -268,10 +268,11 @@ static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
>>>>     } while (pgd++, addr = next, addr != end);
>>>> }
>>>>
>>>> -static phys_addr_t late_pgtable_alloc(void)
>>>> +static phys_addr_t pgd_pgtable_alloc(void)
>>>> {
>>>>     void *ptr = (void *)__get_free_page(PGALLOC_GFP);
>>>> -    BUG_ON(!ptr);
>>>> +    if (!ptr || !pgtable_page_ctor(virt_to_page(ptr)))
>>>
>>> Should we free the page and return NULL when we encounter an error
>>> in pgtable_page_ctor(), like the rest of the callers ?
>>>
>>>
>>>> +        BUG();
>>
>> Just to clarify, of course with the BUG() retained.
>
> But then, if we return, we no longer trigger BUG(). The callers of this
> function assume that it always succeeds or does not return.

Bah, my bad. I somehow confused myself it with WARN behavior and thought
that we could still continue after BUG(). Please ignore the noise.

Suzuki

[PATCH 0/2] arm64: mm: use fully constructed struct pages from EFI page tables

[Laura Abbott]

On 07/22/2016 10:32 AM, Ard Biesheuvel wrote:
> As reported by Sudeep, the EFI memory attributes table handling may crash
> on an uninitialized spinlock in the struct page associated with a UEFI runtime
> translation table page.
>
> This is caused by a failure to take into account the fact that generic code
> treats such pages differently depending on whether they are associated with
> init_mm or not. The EFI page tables are completely separate from both the
> kernel and the userland mappings, and are completely static during the
> lifetime of the OS, but still, the most robust solution is to simply
> construct these pages fully, so that generic code that compares against
> &init_mm can work safely and correctly.
>
> Patch #1 is a preparatory patch to drop a reference to late_pgtable_alloc
> from code that no longer needs it.
>
> Patch #2 renamed late_pgtable_alloc to pgd_table_alloc, and updates it to
> construct the struct page associated with the allocated page.
>
> Ard Biesheuvel (2):
>   arm64: mm: make create_mapping_late() non-allocating
>   arm64: mm: run pgtable_page_ctor() on non-swapper translation table
>     pages
>
>  arch/arm64/mm/mmu.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>

Reviewed-by: Laura Abbott <labbott@redhat.com>

[PATCH 0/2] arm64: mm: use fully constructed struct pages from EFI page tables

[Sudeep Holla]

On 22/07/16 18:32, Ard Biesheuvel wrote:
> As reported by Sudeep, the EFI memory attributes table handling may crash
> on an uninitialized spinlock in the struct page associated with a UEFI runtime
> translation table page.
>
> This is caused by a failure to take into account the fact that generic code
> treats such pages differently depending on whether they are associated with
> init_mm or not. The EFI page tables are completely separate from both the
> kernel and the userland mappings, and are completely static during the
> lifetime of the OS, but still, the most robust solution is to simply
> construct these pages fully, so that generic code that compares against
> &init_mm can work safely and correctly.
>
> Patch #1 is a preparatory patch to drop a reference to late_pgtable_alloc
> from code that no longer needs it.
>
> Patch #2 renamed late_pgtable_alloc to pgd_table_alloc, and updates it to
> construct the struct page associated with the allocated page.
>

These patches fixes the issue I reported. You can add:

Tested-by: Sudeep Holla <sudeep.holla@arm.com>

-- 
Regards,
Sudeep

[PATCH 0/2] arm64: mm: use fully constructed struct pages from EFI page tables

[Catalin Marinas]

On Fri, Jul 22, 2016 at 07:32:23PM +0200, Ard Biesheuvel wrote:
> As reported by Sudeep, the EFI memory attributes table handling may crash
> on an uninitialized spinlock in the struct page associated with a UEFI runtime
> translation table page.
> 
> This is caused by a failure to take into account the fact that generic code
> treats such pages differently depending on whether they are associated with
> init_mm or not. The EFI page tables are completely separate from both the
> kernel and the userland mappings, and are completely static during the
> lifetime of the OS, but still, the most robust solution is to simply
> construct these pages fully, so that generic code that compares against
> &init_mm can work safely and correctly.
> 
> Patch #1 is a preparatory patch to drop a reference to late_pgtable_alloc
> from code that no longer needs it.
> 
> Patch #2 renamed late_pgtable_alloc to pgd_table_alloc, and updates it to
> construct the struct page associated with the allocated page.
> 
> Ard Biesheuvel (2):
>   arm64: mm: make create_mapping_late() non-allocating
>   arm64: mm: run pgtable_page_ctor() on non-swapper translation table
>     pages

Patches applied. I'll send them with the 4.8 pull request this week.

Thanks.

-- 
Catalin