[GIT PULL] EFI urgent fix for v4.10

[GIT PULL] EFI urgent fix for v4.10

[Ard Biesheuvel]

The following changes since commit 090abfc9dc7faf3f84799f956158c8c3af149a81:

  x86/efi: Always map first physical page into EFI pagetables (2017-01-27 20:20:01 +0000)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git tags/efi-urgent

for you to fetch changes up to 3436384c23eec3afff9bde221c7582f9b504ce9e:

  efi: fdt: avoid FDT manipulation after ExitBootServices() (2017-02-01 16:33:37 +0000)

----------------------------------------------------------------
* Fix for unexpected fallout of a previous urgent fix that addressed handling
  of the memory map in the EFI stub FDT code, which turns out to break systems
  that disable the MMU in ExitBootServices() (which is a highly questionable
  practice by itself)

----------------------------------------------------------------
Ard Biesheuvel (1):
      efi: fdt: avoid FDT manipulation after ExitBootServices()

 drivers/firmware/efi/libstub/fdt.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

[PATCH] efi: fdt: avoid FDT manipulation after ExitBootServices()

[Ard Biesheuvel]

Some AArch64 UEFI implementations disable the MMU in ExitBootServices(),
after which unaligned accesses to RAM are no longer supported.

Commit abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the
kernel") fixed an issue in the memory map handling of the stub FDT code,
but inadvertently created an issue with such firmwares, by moving some
of the FDT manipulation to after the invocation of ExitBootServices().
Given that the stub's libfdt implementation uses the ordinary, accelerated
string functions, which rely on hardware handling of unaligned accesses,
manipulating the FDT with the MMU off may result in alignment faults.

So fix the situation by moving the update_fdt_memmap() call into the
callback function invoked by efi_exit_boot_services() right before it
calls the ExitBootServices() UEFI service (which is arguably a better
place for it anyway)

Note that disabling the MMU in ExitBootServices() is not compliant with
the UEFI spec, and carries great risk due to the fact that switching from
cached to uncached memory accesses halfway through compiler generated code
(i.e., involving a stack) can never be done in a way that is architecturally
safe.

Cc: <stable@vger.kernel.org>
Fixes: abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the kernel")
Tested-by: Riku Voipio <riku.voipio@linaro.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/libstub/fdt.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index 921dfa047202..260c4b4b492e 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -187,6 +187,7 @@ static efi_status_t update_fdt_memmap(void *fdt, struct efi_boot_memmap *map)
 struct exit_boot_struct {
 	efi_memory_desc_t *runtime_map;
 	int *runtime_entry_count;
+	void *new_fdt_addr;
 };
 
 static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
@@ -202,7 +203,7 @@ static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
 	efi_get_virtmap(*map->map, *map->map_size, *map->desc_size,
 			p->runtime_map, p->runtime_entry_count);
 
-	return EFI_SUCCESS;
+	return update_fdt_memmap(p->new_fdt_addr, map);
 }
 
 /*
@@ -300,22 +301,13 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 
 	priv.runtime_map = runtime_map;
 	priv.runtime_entry_count = &runtime_entry_count;
+	priv.new_fdt_addr = (void *)*new_fdt_addr;
 	status = efi_exit_boot_services(sys_table, handle, &map, &priv,
 					exit_boot_func);
 
 	if (status == EFI_SUCCESS) {
 		efi_set_virtual_address_map_t *svam;
 
-		status = update_fdt_memmap((void *)*new_fdt_addr, &map);
-		if (status != EFI_SUCCESS) {
-			/*
-			 * The kernel won't get far without the memory map, but
-			 * may still be able to print something meaningful so
-			 * return success here.
-			 */
-			return EFI_SUCCESS;
-		}
-
 		/* Install the new virtual address map */
 		svam = sys_table->runtime->set_virtual_address_map;
 		status = svam(runtime_entry_count * desc_size, desc_size,
-- 
2.7.4

Re: [PATCH] efi: fdt: avoid FDT manipulation after ExitBootServices()

[Leif Lindholm]

On Wed, Feb 01, 2017 at 05:45:02PM +0000, Ard Biesheuvel wrote:
> Some AArch64 UEFI implementations disable the MMU in ExitBootServices(),
> after which unaligned accesses to RAM are no longer supported.
> 
> Commit abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the
> kernel") fixed an issue in the memory map handling of the stub FDT code,
> but inadvertently created an issue with such firmwares, by moving some
> of the FDT manipulation to after the invocation of ExitBootServices().
> Given that the stub's libfdt implementation uses the ordinary, accelerated
> string functions, which rely on hardware handling of unaligned accesses,
> manipulating the FDT with the MMU off may result in alignment faults.
> 
> So fix the situation by moving the update_fdt_memmap() call into the
> callback function invoked by efi_exit_boot_services() right before it
> calls the ExitBootServices() UEFI service (which is arguably a better
> place for it anyway)
> 
> Note that disabling the MMU in ExitBootServices() is not compliant with
> the UEFI spec, and carries great risk due to the fact that switching from
> cached to uncached memory accesses halfway through compiler generated code
> (i.e., involving a stack) can never be done in a way that is architecturally
> safe.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the kernel")
> Tested-by: Riku Voipio <riku.voipio@linaro.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Acked-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  drivers/firmware/efi/libstub/fdt.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> index 921dfa047202..260c4b4b492e 100644
> --- a/drivers/firmware/efi/libstub/fdt.c
> +++ b/drivers/firmware/efi/libstub/fdt.c
> @@ -187,6 +187,7 @@ static efi_status_t update_fdt_memmap(void *fdt, struct efi_boot_memmap *map)
>  struct exit_boot_struct {
>  	efi_memory_desc_t *runtime_map;
>  	int *runtime_entry_count;
> +	void *new_fdt_addr;
>  };
>  
>  static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
> @@ -202,7 +203,7 @@ static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
>  	efi_get_virtmap(*map->map, *map->map_size, *map->desc_size,
>  			p->runtime_map, p->runtime_entry_count);
>  
> -	return EFI_SUCCESS;
> +	return update_fdt_memmap(p->new_fdt_addr, map);
>  }
>  
>  /*
> @@ -300,22 +301,13 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>  
>  	priv.runtime_map = runtime_map;
>  	priv.runtime_entry_count = &runtime_entry_count;
> +	priv.new_fdt_addr = (void *)*new_fdt_addr;
>  	status = efi_exit_boot_services(sys_table, handle, &map, &priv,
>  					exit_boot_func);
>  
>  	if (status == EFI_SUCCESS) {
>  		efi_set_virtual_address_map_t *svam;
>  
> -		status = update_fdt_memmap((void *)*new_fdt_addr, &map);
> -		if (status != EFI_SUCCESS) {
> -			/*
> -			 * The kernel won't get far without the memory map, but
> -			 * may still be able to print something meaningful so
> -			 * return success here.
> -			 */
> -			return EFI_SUCCESS;
> -		}
> -
>  		/* Install the new virtual address map */
>  		svam = sys_table->runtime->set_virtual_address_map;
>  		status = svam(runtime_entry_count * desc_size, desc_size,
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] efi: fdt: avoid FDT manipulation after ExitBootServices()

[Ingo Molnar]

* Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> Some AArch64 UEFI implementations disable the MMU in ExitBootServices(),
> after which unaligned accesses to RAM are no longer supported.
> 
> Commit abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the
> kernel") fixed an issue in the memory map handling of the stub FDT code,
> but inadvertently created an issue with such firmwares, by moving some
> of the FDT manipulation to after the invocation of ExitBootServices().
> Given that the stub's libfdt implementation uses the ordinary, accelerated
> string functions, which rely on hardware handling of unaligned accesses,
> manipulating the FDT with the MMU off may result in alignment faults.
> 
> So fix the situation by moving the update_fdt_memmap() call into the
> callback function invoked by efi_exit_boot_services() right before it
> calls the ExitBootServices() UEFI service (which is arguably a better
> place for it anyway)
> 
> Note that disabling the MMU in ExitBootServices() is not compliant with
> the UEFI spec, and carries great risk due to the fact that switching from
> cached to uncached memory accesses halfway through compiler generated code
> (i.e., involving a stack) can never be done in a way that is architecturally
> safe.
> 
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Fixes: abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the kernel")
> Tested-by: Riku Voipio <riku.voipio-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/libstub/fdt.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> index 921dfa047202..260c4b4b492e 100644
> --- a/drivers/firmware/efi/libstub/fdt.c
> +++ b/drivers/firmware/efi/libstub/fdt.c
> @@ -187,6 +187,7 @@ static efi_status_t update_fdt_memmap(void *fdt, struct efi_boot_memmap *map)
>  struct exit_boot_struct {
>  	efi_memory_desc_t *runtime_map;
>  	int *runtime_entry_count;
> +	void *new_fdt_addr;
>  };
>  
>  static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
> @@ -202,7 +203,7 @@ static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
>  	efi_get_virtmap(*map->map, *map->map_size, *map->desc_size,
>  			p->runtime_map, p->runtime_entry_count);
>  
> -	return EFI_SUCCESS;
> +	return update_fdt_memmap(p->new_fdt_addr, map);
>  }
>  
>  /*
> @@ -300,22 +301,13 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>  
>  	priv.runtime_map = runtime_map;
>  	priv.runtime_entry_count = &runtime_entry_count;
> +	priv.new_fdt_addr = (void *)*new_fdt_addr;

Nit: could the type cast by dropped?

Thanks,

	Ingo

Re: [PATCH] efi: fdt: avoid FDT manipulation after ExitBootServices()

[Ard Biesheuvel]

On 1 February 2017 at 19:03, Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> * Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
>> Some AArch64 UEFI implementations disable the MMU in ExitBootServices(),
>> after which unaligned accesses to RAM are no longer supported.
>>
>> Commit abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the
>> kernel") fixed an issue in the memory map handling of the stub FDT code,
>> but inadvertently created an issue with such firmwares, by moving some
>> of the FDT manipulation to after the invocation of ExitBootServices().
>> Given that the stub's libfdt implementation uses the ordinary, accelerated
>> string functions, which rely on hardware handling of unaligned accesses,
>> manipulating the FDT with the MMU off may result in alignment faults.
>>
>> So fix the situation by moving the update_fdt_memmap() call into the
>> callback function invoked by efi_exit_boot_services() right before it
>> calls the ExitBootServices() UEFI service (which is arguably a better
>> place for it anyway)
>>
>> Note that disabling the MMU in ExitBootServices() is not compliant with
>> the UEFI spec, and carries great risk due to the fact that switching from
>> cached to uncached memory accesses halfway through compiler generated code
>> (i.e., involving a stack) can never be done in a way that is architecturally
>> safe.
>>
>> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
>> Fixes: abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the kernel")
>> Tested-by: Riku Voipio <riku.voipio-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  drivers/firmware/efi/libstub/fdt.c | 14 +++-----------
>>  1 file changed, 3 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
>> index 921dfa047202..260c4b4b492e 100644
>> --- a/drivers/firmware/efi/libstub/fdt.c
>> +++ b/drivers/firmware/efi/libstub/fdt.c
>> @@ -187,6 +187,7 @@ static efi_status_t update_fdt_memmap(void *fdt, struct efi_boot_memmap *map)
>>  struct exit_boot_struct {
>>       efi_memory_desc_t *runtime_map;
>>       int *runtime_entry_count;
>> +     void *new_fdt_addr;
>>  };
>>
>>  static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
>> @@ -202,7 +203,7 @@ static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
>>       efi_get_virtmap(*map->map, *map->map_size, *map->desc_size,
>>                       p->runtime_map, p->runtime_entry_count);
>>
>> -     return EFI_SUCCESS;
>> +     return update_fdt_memmap(p->new_fdt_addr, map);
>>  }
>>
>>  /*
>> @@ -300,22 +301,13 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>>
>>       priv.runtime_map = runtime_map;
>>       priv.runtime_entry_count = &runtime_entry_count;
>> +     priv.new_fdt_addr = (void *)*new_fdt_addr;
>
> Nit: could the type cast by dropped?
>

new_fdt_addr is a pointer to unsigned long, so we can't drop the cast
here without introducing it somewhere else.

Re: [PATCH] efi: fdt: avoid FDT manipulation after ExitBootServices()

[Ingo Molnar]

* Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> On 1 February 2017 at 19:03, Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> > * Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> >
> >> Some AArch64 UEFI implementations disable the MMU in ExitBootServices(),
> >> after which unaligned accesses to RAM are no longer supported.
> >>
> >> Commit abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the
> >> kernel") fixed an issue in the memory map handling of the stub FDT code,
> >> but inadvertently created an issue with such firmwares, by moving some
> >> of the FDT manipulation to after the invocation of ExitBootServices().
> >> Given that the stub's libfdt implementation uses the ordinary, accelerated
> >> string functions, which rely on hardware handling of unaligned accesses,
> >> manipulating the FDT with the MMU off may result in alignment faults.
> >>
> >> So fix the situation by moving the update_fdt_memmap() call into the
> >> callback function invoked by efi_exit_boot_services() right before it
> >> calls the ExitBootServices() UEFI service (which is arguably a better
> >> place for it anyway)
> >>
> >> Note that disabling the MMU in ExitBootServices() is not compliant with
> >> the UEFI spec, and carries great risk due to the fact that switching from
> >> cached to uncached memory accesses halfway through compiler generated code
> >> (i.e., involving a stack) can never be done in a way that is architecturally
> >> safe.
> >>
> >> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> >> Fixes: abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the kernel")
> >> Tested-by: Riku Voipio <riku.voipio-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> ---
> >>  drivers/firmware/efi/libstub/fdt.c | 14 +++-----------
> >>  1 file changed, 3 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> >> index 921dfa047202..260c4b4b492e 100644
> >> --- a/drivers/firmware/efi/libstub/fdt.c
> >> +++ b/drivers/firmware/efi/libstub/fdt.c
> >> @@ -187,6 +187,7 @@ static efi_status_t update_fdt_memmap(void *fdt, struct efi_boot_memmap *map)
> >>  struct exit_boot_struct {
> >>       efi_memory_desc_t *runtime_map;
> >>       int *runtime_entry_count;
> >> +     void *new_fdt_addr;
> >>  };
> >>
> >>  static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
> >> @@ -202,7 +203,7 @@ static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
> >>       efi_get_virtmap(*map->map, *map->map_size, *map->desc_size,
> >>                       p->runtime_map, p->runtime_entry_count);
> >>
> >> -     return EFI_SUCCESS;
> >> +     return update_fdt_memmap(p->new_fdt_addr, map);
> >>  }
> >>
> >>  /*
> >> @@ -300,22 +301,13 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
> >>
> >>       priv.runtime_map = runtime_map;
> >>       priv.runtime_entry_count = &runtime_entry_count;
> >> +     priv.new_fdt_addr = (void *)*new_fdt_addr;
> >
> > Nit: could the type cast by dropped?
> 
> new_fdt_addr is a pointer to unsigned long, so we can't drop the cast
> here without introducing it somewhere else.

Fair enough, I missed the dereference of new_fdt_addr.

Thanks,

	Ingo

Re: [PATCH] efi: fdt: avoid FDT manipulation after ExitBootServices()

[Jeffrey Hugo]

On 2/1/2017 10:45 AM, Ard Biesheuvel wrote:
> Some AArch64 UEFI implementations disable the MMU in ExitBootServices(),
> after which unaligned accesses to RAM are no longer supported.
>
> Commit abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the
> kernel") fixed an issue in the memory map handling of the stub FDT code,
> but inadvertently created an issue with such firmwares, by moving some
> of the FDT manipulation to after the invocation of ExitBootServices().
> Given that the stub's libfdt implementation uses the ordinary, accelerated
> string functions, which rely on hardware handling of unaligned accesses,
> manipulating the FDT with the MMU off may result in alignment faults.
>
> So fix the situation by moving the update_fdt_memmap() call into the
> callback function invoked by efi_exit_boot_services() right before it
> calls the ExitBootServices() UEFI service (which is arguably a better
> place for it anyway)
>
> Note that disabling the MMU in ExitBootServices() is not compliant with
> the UEFI spec, and carries great risk due to the fact that switching from
> cached to uncached memory accesses halfway through compiler generated code
> (i.e., involving a stack) can never be done in a way that is architecturally
> safe.
>
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Fixes: abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the kernel")
> Tested-by: Riku Voipio <riku.voipio-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

NACK, please.  This causes a regression on my platform, in the form of 
an assert in UEFI once ExitBootServices() is called, per initial 
testing.  I'll do more testing to determine why.

-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] efi: fdt: avoid FDT manipulation after ExitBootServices()

[Jeffrey Hugo]

On 2/1/2017 2:08 PM, Jeffrey Hugo wrote:
> On 2/1/2017 10:45 AM, Ard Biesheuvel wrote:
>> Some AArch64 UEFI implementations disable the MMU in ExitBootServices(),
>> after which unaligned accesses to RAM are no longer supported.
>>
>> Commit abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the
>> kernel") fixed an issue in the memory map handling of the stub FDT code,
>> but inadvertently created an issue with such firmwares, by moving some
>> of the FDT manipulation to after the invocation of ExitBootServices().
>> Given that the stub's libfdt implementation uses the ordinary,
>> accelerated
>> string functions, which rely on hardware handling of unaligned accesses,
>> manipulating the FDT with the MMU off may result in alignment faults.
>>
>> So fix the situation by moving the update_fdt_memmap() call into the
>> callback function invoked by efi_exit_boot_services() right before it
>> calls the ExitBootServices() UEFI service (which is arguably a better
>> place for it anyway)
>>
>> Note that disabling the MMU in ExitBootServices() is not compliant with
>> the UEFI spec, and carries great risk due to the fact that switching from
>> cached to uncached memory accesses halfway through compiler generated
>> code
>> (i.e., involving a stack) can never be done in a way that is
>> architecturally
>> safe.
>>
>> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
>> Fixes: abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the
>> kernel")
>> Tested-by: Riku Voipio <riku.voipio-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> NACK, please.  This causes a regression on my platform, in the form of
> an assert in UEFI once ExitBootServices() is called, per initial
> testing.  I'll do more testing to determine why.
>

Sorry, false alarm.  The assert appears to have been the result of a bad 
tree and a bad target configuration, and was not reproduced on another 
setup, nor did the assert make sense in context with this change.

I withdraw my NACK.

-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] efi: fdt: avoid FDT manipulation after ExitBootServices()

[Ard Biesheuvel]

On 1 February 2017 at 22:59, Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> On 2/1/2017 2:08 PM, Jeffrey Hugo wrote:
>>
>> On 2/1/2017 10:45 AM, Ard Biesheuvel wrote:
>>>
>>> Some AArch64 UEFI implementations disable the MMU in ExitBootServices(),
>>> after which unaligned accesses to RAM are no longer supported.
>>>
>>> Commit abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the
>>> kernel") fixed an issue in the memory map handling of the stub FDT code,
>>> but inadvertently created an issue with such firmwares, by moving some
>>> of the FDT manipulation to after the invocation of ExitBootServices().
>>> Given that the stub's libfdt implementation uses the ordinary,
>>> accelerated
>>> string functions, which rely on hardware handling of unaligned accesses,
>>> manipulating the FDT with the MMU off may result in alignment faults.
>>>
>>> So fix the situation by moving the update_fdt_memmap() call into the
>>> callback function invoked by efi_exit_boot_services() right before it
>>> calls the ExitBootServices() UEFI service (which is arguably a better
>>> place for it anyway)
>>>
>>> Note that disabling the MMU in ExitBootServices() is not compliant with
>>> the UEFI spec, and carries great risk due to the fact that switching from
>>> cached to uncached memory accesses halfway through compiler generated
>>> code
>>> (i.e., involving a stack) can never be done in a way that is
>>> architecturally
>>> safe.
>>>
>>> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
>>> Fixes: abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the
>>> kernel")
>>> Tested-by: Riku Voipio <riku.voipio-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>
>>
>> NACK, please.  This causes a regression on my platform, in the form of
>> an assert in UEFI once ExitBootServices() is called, per initial
>> testing.  I'll do more testing to determine why.
>>
>
> Sorry, false alarm.  The assert appears to have been the result of a bad
> tree and a bad target configuration, and was not reproduced on another
> setup, nor did the assert make sense in context with this change.
>

No worries. Thanks for taking the time to test the patch.

-- 
Ard.

Re: [PATCH] efi: fdt: avoid FDT manipulation after ExitBootServices()

[Riku Voipio]

On 1 February 2017 at 15:11, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Some AArch64 UEFI implementations disable the MMU in ExitBootServices(),
> after which unaligned accesses to RAM are no longer supported.
>
> Commit abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the
> kernel") fixed an issue in the memory map handling of the stub FDT code,
> but inadvertently created an issue with such firmwares, by moving some
> of the FDT manipulation to after the invocation of ExitBootServices().
> However, the stub's libfdt implementation uses the ordinary, accelerated
> string functions, which rely on hardware handling of unaligned accesses,
> resulting in alignment faults when executed with the MMU off.
>
> Whether disabling the MMU in ExitBootServices() complies with the UEFI
> spec is unclear, but it is a reality we have to deal with, given that
> it wasn't a problem before commit abfb7b686a3e was applied.
>
> So fix the situation by moving the update_fdt_memmap() into the callback
> invoked by efi_exit_boot_services() right before it hits the button.
>
> Fixes: abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the kernel")
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Tested-by: Riku Voipio <riku.voipio-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

> ---
>  drivers/firmware/efi/libstub/fdt.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> index 921dfa047202..260c4b4b492e 100644
> --- a/drivers/firmware/efi/libstub/fdt.c
> +++ b/drivers/firmware/efi/libstub/fdt.c
> @@ -187,6 +187,7 @@ static efi_status_t update_fdt_memmap(void *fdt, struct efi_boot_memmap *map)
>  struct exit_boot_struct {
>         efi_memory_desc_t *runtime_map;
>         int *runtime_entry_count;
> +       void *new_fdt_addr;
>  };
>
>  static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
> @@ -202,7 +203,7 @@ static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
>         efi_get_virtmap(*map->map, *map->map_size, *map->desc_size,
>                         p->runtime_map, p->runtime_entry_count);
>
> -       return EFI_SUCCESS;
> +       return update_fdt_memmap(p->new_fdt_addr, map);
>  }
>
>  /*
> @@ -300,22 +301,13 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>
>         priv.runtime_map = runtime_map;
>         priv.runtime_entry_count = &runtime_entry_count;
> +       priv.new_fdt_addr = (void *)*new_fdt_addr;
>         status = efi_exit_boot_services(sys_table, handle, &map, &priv,
>                                         exit_boot_func);
>
>         if (status == EFI_SUCCESS) {
>                 efi_set_virtual_address_map_t *svam;
>
> -               status = update_fdt_memmap((void *)*new_fdt_addr, &map);
> -               if (status != EFI_SUCCESS) {
> -                       /*
> -                        * The kernel won't get far without the memory map, but
> -                        * may still be able to print something meaningful so
> -                        * return success here.
> -                        */
> -                       return EFI_SUCCESS;
> -               }
> -
>                 /* Install the new virtual address map */
>                 svam = sys_table->runtime->set_virtual_address_map;
>                 status = svam(runtime_entry_count * desc_size, desc_size,
> --
> 2.7.4
>

Re: [PATCH] efi: fdt: avoid FDT manipulation after ExitBootServices()

[Mark Rutland]

On Wed, Feb 01, 2017 at 01:11:33PM +0000, Ard Biesheuvel wrote:
> Some AArch64 UEFI implementations disable the MMU in ExitBootServices(),
> after which unaligned accesses to RAM are no longer supported.

Urrgh. That's a pretty horrible bug. :(

> Commit abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the
> kernel") fixed an issue in the memory map handling of the stub FDT code,
> but inadvertently created an issue with such firmwares, by moving some
> of the FDT manipulation to after the invocation of ExitBootServices().
> However, the stub's libfdt implementation uses the ordinary, accelerated
> string functions, which rely on hardware handling of unaligned accesses,
> resulting in alignment faults when executed with the MMU off.
> 
> Whether disabling the MMU in ExitBootServices() complies with the UEFI
> spec is unclear, but it is a reality we have to deal with, given that
> it wasn't a problem before commit abfb7b686a3e was applied.

I'd argue that this quite clearly violates the calling convention
requiremments in 2.3.6 (e.g. SCTLR_EL1 configuration, which permits
unaligned accesses). If ExitBootServices() changes the SCTLR_EL1 value,
it clearly violates the calling convention.

This would also mean we're calling SetVirtualAddressMap() in violation
of said convention.

For the timebeing, can we drop this statement regarding the spec from
the commit message?

Regardless, I agree that we need to handle this somehow.

> So fix the situation by moving the update_fdt_memmap() into the callback
> invoked by efi_exit_boot_services() right before it hits the button.

I think this is a sensible change, but I do suspect that we're likely
to have more fun with this broken platform in future as the stub changes
(or compilers decide to generate unaligned accesses elsewhere).

Thanks,
Mark.

Re: [PATCH] efi: fdt: avoid FDT manipulation after ExitBootServices()

[Ard Biesheuvel]

On 1 February 2017 at 13:39, Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 1 February 2017 at 13:11, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> Some AArch64 UEFI implementations disable the MMU in ExitBootServices(),
>> after which unaligned accesses to RAM are no longer supported.
>>
>> Commit abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the
>> kernel") fixed an issue in the memory map handling of the stub FDT code,
>> but inadvertently created an issue with such firmwares, by moving some
>> of the FDT manipulation to after the invocation of ExitBootServices().
>> However, the stub's libfdt implementation uses the ordinary, accelerated
>> string functions, which rely on hardware handling of unaligned accesses,
>> resulting in alignment faults when executed with the MMU off.
>>
>> Whether disabling the MMU in ExitBootServices() complies with the UEFI
>> spec is unclear, but it is a reality we have to deal with, given that
>> it wasn't a problem before commit abfb7b686a3e was applied.
>
> I'm not sure it's unclear, but someone not reading the specification
> and instead looking
> at what the unfortunate LinuxLoader did might incorrectly deduce that
> such is the case.
>
>> So fix the situation by moving the update_fdt_memmap() into the callback
>> invoked by efi_exit_boot_services() right before it hits the button.
>>
>> Fixes: abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the kernel")
>
> I would sort argue against the Fixes: tag, given that I don't consider
> that code to be
> incorrect.
>

Well, that patch ended up in -stable, and is breaking actual systems
that worked 'fine' before.

> I agree that this is a good change regardless.
>

Cheers.

Re: [PATCH] efi: fdt: avoid FDT manipulation after ExitBootServices()

[Leif Lindholm]

On 1 February 2017 at 13:11, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Some AArch64 UEFI implementations disable the MMU in ExitBootServices(),
> after which unaligned accesses to RAM are no longer supported.
>
> Commit abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the
> kernel") fixed an issue in the memory map handling of the stub FDT code,
> but inadvertently created an issue with such firmwares, by moving some
> of the FDT manipulation to after the invocation of ExitBootServices().
> However, the stub's libfdt implementation uses the ordinary, accelerated
> string functions, which rely on hardware handling of unaligned accesses,
> resulting in alignment faults when executed with the MMU off.
>
> Whether disabling the MMU in ExitBootServices() complies with the UEFI
> spec is unclear, but it is a reality we have to deal with, given that
> it wasn't a problem before commit abfb7b686a3e was applied.

I'm not sure it's unclear, but someone not reading the specification
and instead looking
at what the unfortunate LinuxLoader did might incorrectly deduce that
such is the case.

> So fix the situation by moving the update_fdt_memmap() into the callback
> invoked by efi_exit_boot_services() right before it hits the button.
>
> Fixes: abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the kernel")

I would sort argue against the Fixes: tag, given that I don't consider
that code to be
incorrect.

I agree that this is a good change regardless.

/
    Leif

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/libstub/fdt.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> index 921dfa047202..260c4b4b492e 100644
> --- a/drivers/firmware/efi/libstub/fdt.c
> +++ b/drivers/firmware/efi/libstub/fdt.c
> @@ -187,6 +187,7 @@ static efi_status_t update_fdt_memmap(void *fdt, struct efi_boot_memmap *map)
>  struct exit_boot_struct {
>         efi_memory_desc_t *runtime_map;
>         int *runtime_entry_count;
> +       void *new_fdt_addr;
>  };
>
>  static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
> @@ -202,7 +203,7 @@ static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
>         efi_get_virtmap(*map->map, *map->map_size, *map->desc_size,
>                         p->runtime_map, p->runtime_entry_count);
>
> -       return EFI_SUCCESS;
> +       return update_fdt_memmap(p->new_fdt_addr, map);
>  }
>
>  /*
> @@ -300,22 +301,13 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>
>         priv.runtime_map = runtime_map;
>         priv.runtime_entry_count = &runtime_entry_count;
> +       priv.new_fdt_addr = (void *)*new_fdt_addr;
>         status = efi_exit_boot_services(sys_table, handle, &map, &priv,
>                                         exit_boot_func);
>
>         if (status == EFI_SUCCESS) {
>                 efi_set_virtual_address_map_t *svam;
>
> -               status = update_fdt_memmap((void *)*new_fdt_addr, &map);
> -               if (status != EFI_SUCCESS) {
> -                       /*
> -                        * The kernel won't get far without the memory map, but
> -                        * may still be able to print something meaningful so
> -                        * return success here.
> -                        */
> -                       return EFI_SUCCESS;
> -               }
> -
>                 /* Install the new virtual address map */
>                 svam = sys_table->runtime->set_virtual_address_map;
>                 status = svam(runtime_entry_count * desc_size, desc_size,
> --
> 2.7.4
>

[PATCH] efi: fdt: avoid FDT manipulation after ExitBootServices()

[Ard Biesheuvel]

Some AArch64 UEFI implementations disable the MMU in ExitBootServices(),
after which unaligned accesses to RAM are no longer supported.

Commit abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the
kernel") fixed an issue in the memory map handling of the stub FDT code,
but inadvertently created an issue with such firmwares, by moving some
of the FDT manipulation to after the invocation of ExitBootServices().
However, the stub's libfdt implementation uses the ordinary, accelerated
string functions, which rely on hardware handling of unaligned accesses,
resulting in alignment faults when executed with the MMU off.

Whether disabling the MMU in ExitBootServices() complies with the UEFI
spec is unclear, but it is a reality we have to deal with, given that
it wasn't a problem before commit abfb7b686a3e was applied.

So fix the situation by moving the update_fdt_memmap() into the callback
invoked by efi_exit_boot_services() right before it hits the button.

Fixes: abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the kernel")
Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/libstub/fdt.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index 921dfa047202..260c4b4b492e 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -187,6 +187,7 @@ static efi_status_t update_fdt_memmap(void *fdt, struct efi_boot_memmap *map)
 struct exit_boot_struct {
 	efi_memory_desc_t *runtime_map;
 	int *runtime_entry_count;
+	void *new_fdt_addr;
 };
 
 static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
@@ -202,7 +203,7 @@ static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
 	efi_get_virtmap(*map->map, *map->map_size, *map->desc_size,
 			p->runtime_map, p->runtime_entry_count);
 
-	return EFI_SUCCESS;
+	return update_fdt_memmap(p->new_fdt_addr, map);
 }
 
 /*
@@ -300,22 +301,13 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 
 	priv.runtime_map = runtime_map;
 	priv.runtime_entry_count = &runtime_entry_count;
+	priv.new_fdt_addr = (void *)*new_fdt_addr;
 	status = efi_exit_boot_services(sys_table, handle, &map, &priv,
 					exit_boot_func);
 
 	if (status == EFI_SUCCESS) {
 		efi_set_virtual_address_map_t *svam;
 
-		status = update_fdt_memmap((void *)*new_fdt_addr, &map);
-		if (status != EFI_SUCCESS) {
-			/*
-			 * The kernel won't get far without the memory map, but
-			 * may still be able to print something meaningful so
-			 * return success here.
-			 */
-			return EFI_SUCCESS;
-		}
-
 		/* Install the new virtual address map */
 		svam = sys_table->runtime->set_virtual_address_map;
 		status = svam(runtime_entry_count * desc_size, desc_size,
-- 
2.7.4