| Message ID | 1485971102-23330-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
* Ard Biesheuvel <ard.biesheuvel@linaro.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@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; Nit: could the type cast by dropped? Thanks, Ingo
On 1 February 2017 at 19:03, Ingo Molnar <mingo@kernel.org> wrote: > > * Ard Biesheuvel <ard.biesheuvel@linaro.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@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; > > 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.
* Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 1 February 2017 at 19:03, Ingo Molnar <mingo@kernel.org> wrote: > > > > * Ard Biesheuvel <ard.biesheuvel@linaro.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@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; > > > > 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
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@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> 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.
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@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> > > 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.
On 1 February 2017 at 22:59, Jeffrey Hugo <jhugo@codeaurora.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@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> >> >> >> 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.
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(-)