| Message ID | 20181217180214.9436-3-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Final EFI fixes for v4.20 | expand |
On 12/17/18 7:16 PM, tip-bot for Heinrich Schuchardt wrote: > Commit-ID: 793423cf07e51e3185b8680167115813589c057d > Gitweb: https://git.kernel.org/tip/793423cf07e51e3185b8680167115813589c057d > Author: Heinrich Schuchardt <xypron.glpk@gmx.de> > AuthorDate: Mon, 17 Dec 2018 19:02:14 +0100 > Committer: Ingo Molnar <mingo@kernel.org> > CommitDate: Mon, 17 Dec 2018 19:12:48 +0100 > > efi: Align 'efi_guid_t' to 64 bits > > The UEFI Specification Version 2.7 Errata A defines: > > "EFI_GUID > 128-bit buffer containing a unique identifier value. > Unless otherwise specified, aligned on a 64-bit boundary." > > Before this patch efi_guid_t was only 8-bit aligned. > > Note that this could potentially trigger alignment faults during > EFI runtime services calls on 32-bit ARM, given that it does not > permit load/store double or load/store multiple instructions to > operate on memory addresses that are not 32-bit aligned. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: <stable@vger.kernel.org> # v4.9+, or earlier if possible > Cc: Andy Lutomirski <luto@kernel.org> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Dave Hansen <dave.hansen@linux.intel.com> > Cc: H. Peter Anvin <hpa@zytor.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Qian Cai <cai@gmx.us> > Cc: Rik van Riel <riel@surriel.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: linux-efi@vger.kernel.org > Link: http://lkml.kernel.org/r/20181217180214.9436-3-ard.biesheuvel@linaro.org > Signed-off-by: Ingo Molnar <mingo@kernel.org> > --- > include/linux/efi.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/efi.h b/include/linux/efi.h > index 100ce4a4aff6..e6480c805932 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -48,7 +48,7 @@ typedef u16 efi_char16_t; /* UNICODE character */ > typedef u64 efi_physical_addr_t; > typedef void *efi_handle_t; > > -typedef guid_t efi_guid_t; > + > > #define EFI_GUID(a,b,c,d0,d1,d2,d3,d4,d5,d6,d7) \ > GUID_INIT(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) > Before rushing this patch in, we should carefully review its side effects, e.g. on 32bit system this changes the size of efi_config_table_32_t from 20 to 24, which is part of the interface to the UEFI firmware. Best regards Heinrich
On Mon, 17 Dec 2018 at 23:33, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 12/17/18 7:16 PM, tip-bot for Heinrich Schuchardt wrote: > > Commit-ID: 793423cf07e51e3185b8680167115813589c057d > > Gitweb: https://git.kernel.org/tip/793423cf07e51e3185b8680167115813589c057d > > Author: Heinrich Schuchardt <xypron.glpk@gmx.de> > > AuthorDate: Mon, 17 Dec 2018 19:02:14 +0100 > > Committer: Ingo Molnar <mingo@kernel.org> > > CommitDate: Mon, 17 Dec 2018 19:12:48 +0100 > > > > efi: Align 'efi_guid_t' to 64 bits > > > > The UEFI Specification Version 2.7 Errata A defines: > > > > "EFI_GUID > > 128-bit buffer containing a unique identifier value. > > Unless otherwise specified, aligned on a 64-bit boundary." > > > > Before this patch efi_guid_t was only 8-bit aligned. > > > > Note that this could potentially trigger alignment faults during > > EFI runtime services calls on 32-bit ARM, given that it does not > > permit load/store double or load/store multiple instructions to > > operate on memory addresses that are not 32-bit aligned. > > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Cc: <stable@vger.kernel.org> # v4.9+, or earlier if possible > > Cc: Andy Lutomirski <luto@kernel.org> > > Cc: Borislav Petkov <bp@alien8.de> > > Cc: Dave Hansen <dave.hansen@linux.intel.com> > > Cc: H. Peter Anvin <hpa@zytor.com> > > Cc: Linus Torvalds <torvalds@linux-foundation.org> > > Cc: Peter Zijlstra <peterz@infradead.org> > > Cc: Qian Cai <cai@gmx.us> > > Cc: Rik van Riel <riel@surriel.com> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: linux-efi@vger.kernel.org > > Link: http://lkml.kernel.org/r/20181217180214.9436-3-ard.biesheuvel@linaro.org > > Signed-off-by: Ingo Molnar <mingo@kernel.org> > > --- > > include/linux/efi.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/linux/efi.h b/include/linux/efi.h > > index 100ce4a4aff6..e6480c805932 100644 > > --- a/include/linux/efi.h > > +++ b/include/linux/efi.h > > @@ -48,7 +48,7 @@ typedef u16 efi_char16_t; /* UNICODE character */ > > typedef u64 efi_physical_addr_t; > > typedef void *efi_handle_t; > > > > -typedef guid_t efi_guid_t; > > + > > > > #define EFI_GUID(a,b,c,d0,d1,d2,d3,d4,d5,d6,d7) \ > > GUID_INIT(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) > > > > Before rushing this patch in, we should carefully review its side > effects, e.g. on 32bit system this changes the size of > efi_config_table_32_t from 20 to 24, which is part of the interface to > the UEFI firmware. > grmbl. Thanks for spotting that. The UEFI spec defines a GUID struct as { UINT32; UINT16; UINT16; UINT8[8]; } so its natural alignment is 32 bits not 64 bits. The alignment issue on ARM would be solved by using __aligned(4) rather than __aligned(8), while not affecting the size of the config table struct (and potentially others) on 32-bit architectures. Ingo, apologies for the breakage. Do you prefer a replacement patch or a followup patch?
On 12/17/18 11:42 PM, Ard Biesheuvel wrote: > On Mon, 17 Dec 2018 at 23:33, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> On 12/17/18 7:16 PM, tip-bot for Heinrich Schuchardt wrote: >>> Commit-ID: 793423cf07e51e3185b8680167115813589c057d >>> Gitweb: https://git.kernel.org/tip/793423cf07e51e3185b8680167115813589c057d >>> Author: Heinrich Schuchardt <xypron.glpk@gmx.de> >>> AuthorDate: Mon, 17 Dec 2018 19:02:14 +0100 >>> Committer: Ingo Molnar <mingo@kernel.org> >>> CommitDate: Mon, 17 Dec 2018 19:12:48 +0100 >>> >>> efi: Align 'efi_guid_t' to 64 bits >>> >>> The UEFI Specification Version 2.7 Errata A defines: >>> >>> "EFI_GUID >>> 128-bit buffer containing a unique identifier value. >>> Unless otherwise specified, aligned on a 64-bit boundary." >>> >>> Before this patch efi_guid_t was only 8-bit aligned. >>> >>> Note that this could potentially trigger alignment faults during >>> EFI runtime services calls on 32-bit ARM, given that it does not >>> permit load/store double or load/store multiple instructions to >>> operate on memory addresses that are not 32-bit aligned. >>> >>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> Cc: <stable@vger.kernel.org> # v4.9+, or earlier if possible >>> Cc: Andy Lutomirski <luto@kernel.org> >>> Cc: Borislav Petkov <bp@alien8.de> >>> Cc: Dave Hansen <dave.hansen@linux.intel.com> >>> Cc: H. Peter Anvin <hpa@zytor.com> >>> Cc: Linus Torvalds <torvalds@linux-foundation.org> >>> Cc: Peter Zijlstra <peterz@infradead.org> >>> Cc: Qian Cai <cai@gmx.us> >>> Cc: Rik van Riel <riel@surriel.com> >>> Cc: Thomas Gleixner <tglx@linutronix.de> >>> Cc: linux-efi@vger.kernel.org >>> Link: http://lkml.kernel.org/r/20181217180214.9436-3-ard.biesheuvel@linaro.org >>> Signed-off-by: Ingo Molnar <mingo@kernel.org> >>> --- >>> include/linux/efi.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/include/linux/efi.h b/include/linux/efi.h >>> index 100ce4a4aff6..e6480c805932 100644 >>> --- a/include/linux/efi.h >>> +++ b/include/linux/efi.h >>> @@ -48,7 +48,7 @@ typedef u16 efi_char16_t; /* UNICODE character */ >>> typedef u64 efi_physical_addr_t; >>> typedef void *efi_handle_t; >>> >>> -typedef guid_t efi_guid_t; >>> + >>> >>> #define EFI_GUID(a,b,c,d0,d1,d2,d3,d4,d5,d6,d7) \ >>> GUID_INIT(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) >>> >> >> Before rushing this patch in, we should carefully review its side >> effects, e.g. on 32bit system this changes the size of >> efi_config_table_32_t from 20 to 24, which is part of the interface to >> the UEFI firmware. >> > > grmbl. > > Thanks for spotting that. > > The UEFI spec defines a GUID struct as { UINT32; UINT16; UINT16; > UINT8[8]; } so its natural alignment is 32 bits not 64 bits. The > alignment issue on ARM would be solved by using __aligned(4) rather > than __aligned(8), while not affecting the size of the config table > struct (and potentially others) on 32-bit architectures. > > Ingo, apologies for the breakage. Do you prefer a replacement patch or > a followup patch? > The UEFI spec explicitly requires EFI_GUID to be 64-bit aligned. On the other hand neither EDK2 nor GRUB not U-Boot cared about this requirement up to now. So the array of efi_config_table_32_t had 20 byte long members at least on Linux, EDK2, GRUB, U-Boot, and possibly on other UEFI implementations though the UEFI spec does not mention packing here. As the 4.20 kernel release is imminent, please, keep this patch out before causing breakage. Best regards Heinrich
On Tue, 18 Dec 2018 at 00:20, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 12/17/18 11:42 PM, Ard Biesheuvel wrote: > > On Mon, 17 Dec 2018 at 23:33, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >> > >> On 12/17/18 7:16 PM, tip-bot for Heinrich Schuchardt wrote: > >>> Commit-ID: 793423cf07e51e3185b8680167115813589c057d > >>> Gitweb: https://git.kernel.org/tip/793423cf07e51e3185b8680167115813589c057d > >>> Author: Heinrich Schuchardt <xypron.glpk@gmx.de> > >>> AuthorDate: Mon, 17 Dec 2018 19:02:14 +0100 > >>> Committer: Ingo Molnar <mingo@kernel.org> > >>> CommitDate: Mon, 17 Dec 2018 19:12:48 +0100 > >>> > >>> efi: Align 'efi_guid_t' to 64 bits > >>> > >>> The UEFI Specification Version 2.7 Errata A defines: > >>> > >>> "EFI_GUID > >>> 128-bit buffer containing a unique identifier value. > >>> Unless otherwise specified, aligned on a 64-bit boundary." > >>> > >>> Before this patch efi_guid_t was only 8-bit aligned. > >>> > >>> Note that this could potentially trigger alignment faults during > >>> EFI runtime services calls on 32-bit ARM, given that it does not > >>> permit load/store double or load/store multiple instructions to > >>> operate on memory addresses that are not 32-bit aligned. > >>> > >>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >>> Cc: <stable@vger.kernel.org> # v4.9+, or earlier if possible > >>> Cc: Andy Lutomirski <luto@kernel.org> > >>> Cc: Borislav Petkov <bp@alien8.de> > >>> Cc: Dave Hansen <dave.hansen@linux.intel.com> > >>> Cc: H. Peter Anvin <hpa@zytor.com> > >>> Cc: Linus Torvalds <torvalds@linux-foundation.org> > >>> Cc: Peter Zijlstra <peterz@infradead.org> > >>> Cc: Qian Cai <cai@gmx.us> > >>> Cc: Rik van Riel <riel@surriel.com> > >>> Cc: Thomas Gleixner <tglx@linutronix.de> > >>> Cc: linux-efi@vger.kernel.org > >>> Link: http://lkml.kernel.org/r/20181217180214.9436-3-ard.biesheuvel@linaro.org > >>> Signed-off-by: Ingo Molnar <mingo@kernel.org> > >>> --- > >>> include/linux/efi.h | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/include/linux/efi.h b/include/linux/efi.h > >>> index 100ce4a4aff6..e6480c805932 100644 > >>> --- a/include/linux/efi.h > >>> +++ b/include/linux/efi.h > >>> @@ -48,7 +48,7 @@ typedef u16 efi_char16_t; /* UNICODE character */ > >>> typedef u64 efi_physical_addr_t; > >>> typedef void *efi_handle_t; > >>> > >>> -typedef guid_t efi_guid_t; > >>> + > >>> > >>> #define EFI_GUID(a,b,c,d0,d1,d2,d3,d4,d5,d6,d7) \ > >>> GUID_INIT(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) > >>> > >> > >> Before rushing this patch in, we should carefully review its side > >> effects, e.g. on 32bit system this changes the size of > >> efi_config_table_32_t from 20 to 24, which is part of the interface to > >> the UEFI firmware. > >> > > > > grmbl. > > > > Thanks for spotting that. > > > > The UEFI spec defines a GUID struct as { UINT32; UINT16; UINT16; > > UINT8[8]; } so its natural alignment is 32 bits not 64 bits. The > > alignment issue on ARM would be solved by using __aligned(4) rather > > than __aligned(8), while not affecting the size of the config table > > struct (and potentially others) on 32-bit architectures. > > > > Ingo, apologies for the breakage. Do you prefer a replacement patch or > > a followup patch? > > > > The UEFI spec explicitly requires EFI_GUID to be 64-bit aligned. On the > other hand neither EDK2 nor GRUB not U-Boot cared about this requirement > up to now. So the array of efi_config_table_32_t had 20 byte long > members at least on Linux, EDK2, GRUB, U-Boot, and possibly on other > UEFI implementations though the UEFI spec does not mention packing here. > > As the 4.20 kernel release is imminent, please, keep this patch out > before causing breakage. > Ingo, Let's just drop Heinrich's patch for now, and I will come back to it for the next release. There is very little EFI on 32-bit ARM in the field, and this patch in its current form will break IA-32 as well, so a quick followup fix is probably not the best approach here.
On Wed, 19 Dec 2018 at 23:50, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On Tue, 18 Dec 2018 at 00:20, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > On 12/17/18 11:42 PM, Ard Biesheuvel wrote: > > > On Mon, 17 Dec 2018 at 23:33, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > >> > > >> On 12/17/18 7:16 PM, tip-bot for Heinrich Schuchardt wrote: > > >>> Commit-ID: 793423cf07e51e3185b8680167115813589c057d > > >>> Gitweb: https://git.kernel.org/tip/793423cf07e51e3185b8680167115813589c057d > > >>> Author: Heinrich Schuchardt <xypron.glpk@gmx.de> > > >>> AuthorDate: Mon, 17 Dec 2018 19:02:14 +0100 > > >>> Committer: Ingo Molnar <mingo@kernel.org> > > >>> CommitDate: Mon, 17 Dec 2018 19:12:48 +0100 > > >>> > > >>> efi: Align 'efi_guid_t' to 64 bits > > >>> > > >>> The UEFI Specification Version 2.7 Errata A defines: > > >>> > > >>> "EFI_GUID > > >>> 128-bit buffer containing a unique identifier value. > > >>> Unless otherwise specified, aligned on a 64-bit boundary." > > >>> > > >>> Before this patch efi_guid_t was only 8-bit aligned. > > >>> > > >>> Note that this could potentially trigger alignment faults during > > >>> EFI runtime services calls on 32-bit ARM, given that it does not > > >>> permit load/store double or load/store multiple instructions to > > >>> operate on memory addresses that are not 32-bit aligned. > > >>> > > >>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > >>> Cc: <stable@vger.kernel.org> # v4.9+, or earlier if possible > > >>> Cc: Andy Lutomirski <luto@kernel.org> > > >>> Cc: Borislav Petkov <bp@alien8.de> > > >>> Cc: Dave Hansen <dave.hansen@linux.intel.com> > > >>> Cc: H. Peter Anvin <hpa@zytor.com> > > >>> Cc: Linus Torvalds <torvalds@linux-foundation.org> > > >>> Cc: Peter Zijlstra <peterz@infradead.org> > > >>> Cc: Qian Cai <cai@gmx.us> > > >>> Cc: Rik van Riel <riel@surriel.com> > > >>> Cc: Thomas Gleixner <tglx@linutronix.de> > > >>> Cc: linux-efi@vger.kernel.org > > >>> Link: http://lkml.kernel.org/r/20181217180214.9436-3-ard.biesheuvel@linaro.org > > >>> Signed-off-by: Ingo Molnar <mingo@kernel.org> > > >>> --- > > >>> include/linux/efi.h | 2 +- > > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > > >>> > > >>> diff --git a/include/linux/efi.h b/include/linux/efi.h > > >>> index 100ce4a4aff6..e6480c805932 100644 > > >>> --- a/include/linux/efi.h > > >>> +++ b/include/linux/efi.h > > >>> @@ -48,7 +48,7 @@ typedef u16 efi_char16_t; /* UNICODE character */ > > >>> typedef u64 efi_physical_addr_t; > > >>> typedef void *efi_handle_t; > > >>> > > >>> -typedef guid_t efi_guid_t; > > >>> + > > >>> > > >>> #define EFI_GUID(a,b,c,d0,d1,d2,d3,d4,d5,d6,d7) \ > > >>> GUID_INIT(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) > > >>> > > >> > > >> Before rushing this patch in, we should carefully review its side > > >> effects, e.g. on 32bit system this changes the size of > > >> efi_config_table_32_t from 20 to 24, which is part of the interface to > > >> the UEFI firmware. > > >> > > > > > > grmbl. > > > > > > Thanks for spotting that. > > > > > > The UEFI spec defines a GUID struct as { UINT32; UINT16; UINT16; > > > UINT8[8]; } so its natural alignment is 32 bits not 64 bits. The > > > alignment issue on ARM would be solved by using __aligned(4) rather > > > than __aligned(8), while not affecting the size of the config table > > > struct (and potentially others) on 32-bit architectures. > > > > > > Ingo, apologies for the breakage. Do you prefer a replacement patch or > > > a followup patch? > > > > > > > The UEFI spec explicitly requires EFI_GUID to be 64-bit aligned. On the > > other hand neither EDK2 nor GRUB not U-Boot cared about this requirement > > up to now. So the array of efi_config_table_32_t had 20 byte long > > members at least on Linux, EDK2, GRUB, U-Boot, and possibly on other > > UEFI implementations though the UEFI spec does not mention packing here. > > > > As the 4.20 kernel release is imminent, please, keep this patch out > > before causing breakage. > > > > Ingo, > > Let's just drop Heinrich's patch for now, and I will come back to it > for the next release. There is very little EFI on 32-bit ARM in the > field, and this patch in its current form will break IA-32 as well, so > a quick followup fix is probably not the best approach here. Ingo, Could you please let me know what your plans are with this patch? It has been pulled into -next now. Please either drop it or revert it.
diff --git a/include/linux/efi.h b/include/linux/efi.h index 100ce4a4aff6..e6480c805932 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -48,7 +48,7 @@ typedef u16 efi_char16_t; /* UNICODE character */ typedef u64 efi_physical_addr_t; typedef void *efi_handle_t; -typedef guid_t efi_guid_t; +typedef guid_t efi_guid_t __aligned(8); #define EFI_GUID(a,b,c,d0,d1,d2,d3,d4,d5,d6,d7) \ GUID_INIT(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)