From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751743AbdBAUQ4 (ORCPT ); Wed, 1 Feb 2017 15:16:56 -0500 Received: from mail-wm0-f67.google.com ([74.125.82.67]:36859 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751478AbdBAUQs (ORCPT ); Wed, 1 Feb 2017 15:16:48 -0500 Date: Wed, 1 Feb 2017 21:16:44 +0100 From: Ingo Molnar To: Ard Biesheuvel Cc: Leif Lindholm , Mark Rutland , Thomas Gleixner , "H . Peter Anvin" , "linux-arm-kernel@lists.infradead.org" , Riku Voipio , Matt Fleming , "linux-kernel@vger.kernel.org" , "linux-efi@vger.kernel.org" Subject: Re: [PATCH] efi: fdt: avoid FDT manipulation after ExitBootServices() Message-ID: <20170201201644.GA20208@gmail.com> References: <1485971102-23330-1-git-send-email-ard.biesheuvel@linaro.org> <1485971102-23330-2-git-send-email-ard.biesheuvel@linaro.org> <20170201190341.GA1060@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Ard Biesheuvel wrote: > On 1 February 2017 at 19:03, Ingo Molnar wrote: > > > > * 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: > >> Fixes: abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the kernel") > >> Tested-by: Riku Voipio > >> Signed-off-by: Ard Biesheuvel > >> --- > >> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ingo Molnar Subject: Re: [PATCH] efi: fdt: avoid FDT manipulation after ExitBootServices() Date: Wed, 1 Feb 2017 21:16:44 +0100 Message-ID: <20170201201644.GA20208@gmail.com> References: <1485971102-23330-1-git-send-email-ard.biesheuvel@linaro.org> <1485971102-23330-2-git-send-email-ard.biesheuvel@linaro.org> <20170201190341.GA1060@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-efi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Ard Biesheuvel Cc: Leif Lindholm , Mark Rutland , Thomas Gleixner , "H . Peter Anvin" , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , Riku Voipio , Matt Fleming , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" List-Id: linux-efi@vger.kernel.org * Ard Biesheuvel wrote: > On 1 February 2017 at 19:03, Ingo Molnar wrote: > > > > * 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: > >> Fixes: abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the kernel") > >> Tested-by: Riku Voipio > >> Signed-off-by: Ard Biesheuvel > >> --- > >> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: mingo@kernel.org (Ingo Molnar) Date: Wed, 1 Feb 2017 21:16:44 +0100 Subject: [PATCH] efi: fdt: avoid FDT manipulation after ExitBootServices() In-Reply-To: References: <1485971102-23330-1-git-send-email-ard.biesheuvel@linaro.org> <1485971102-23330-2-git-send-email-ard.biesheuvel@linaro.org> <20170201190341.GA1060@gmail.com> Message-ID: <20170201201644.GA20208@gmail.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org * Ard Biesheuvel wrote: > On 1 February 2017 at 19:03, Ingo Molnar wrote: > > > > * 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: > >> Fixes: abfb7b686a3e ("efi/libstub/arm*: Pass latest memory map to the kernel") > >> Tested-by: Riku Voipio > >> Signed-off-by: Ard Biesheuvel > >> --- > >> 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