| Message ID | 20210310081210.95147-1-ardb@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | efi: use 32-bit alignment for efi_guid_t literals | expand |
On Wed, 10 Mar 2021 at 09:12, Ard Biesheuvel <ardb@kernel.org> wrote: > > Commit 494c704f9af0 ("efi: Use 32-bit alignment for efi_guid_t") updated > the type definition of efi_guid_t to ensure that it always appears > sufficiently aligned (the UEFI spec is ambiguous about this, but given > the fact that its EFI_GUID type is defined in terms of a struct carrying > a uint32_t, the natural alignment is definitely >= 32 bits). > > However, we missed the EFI_GUID() macro which is used to instantiate > efi_guid_t literals: that macro is still based on the guid_t type, > which does not have a minimum alignment at all. This results in warnings > such as > > In file included from drivers/firmware/efi/mokvar-table.c:35: > include/linux/efi.h:1093:34: warning: passing 1-byte aligned argument to > 4-byte aligned parameter 2 of 'get_var' may result in an unaligned pointer > access [-Walign-mismatch] > status = get_var(L"SecureBoot", &EFI_GLOBAL_VARIABLE_GUID, NULL, &size, > ^ > include/linux/efi.h:1101:24: warning: passing 1-byte aligned argument to > 4-byte aligned parameter 2 of 'get_var' may result in an unaligned pointer > access [-Walign-mismatch] > get_var(L"SetupMode", &EFI_GLOBAL_VARIABLE_GUID, NULL, &size, &setupmode); > > The distinction only matters on CPUs that do not support misaligned loads > fully, but 32-bit ARM's load-multiple instructions fall into that category, > and these are likely to be emitted by the compiler that built the firmware > for loading word-aligned 128-bit GUIDs from memory > > Instead of bodging this further, let's simply switch to our own definition > of efi_guid_t that carries a uint32_t as well. Since efi_guid_t is used as > an opaque type everywhere in the EFI code, this is only a minor code change. > > Reported-by: Nathan Chancellor <nathan@kernel.org> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > > I am currently testing this change via my for-kernelci branch. Please give > this some soak time in the other CIs that we have access to. > Note: efivarfs needs a tweak as well: --- a/fs/efivarfs/inode.c +++ b/fs/efivarfs/inode.c @@ -84,7 +84,7 @@ static int efivarfs_create(struct user_namespace *mnt_userns, struct inode *dir, /* length of the variable name itself: remove GUID and separator */ namelen = dentry->d_name.len - EFI_VARIABLE_GUID_LEN - 1; - err = guid_parse(dentry->d_name.name + namelen + 1, &var->var.VendorGuid); + err = guid_parse(dentry->d_name.name + namelen + 1, (guid_t *)&var->var.VendorGuid); if (err) goto out; > include/linux/efi.h | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/include/linux/efi.h b/include/linux/efi.h > index 8710f5710c1d..f39e9ec7485f 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -63,17 +63,22 @@ typedef void *efi_handle_t; > * is 32 bits not 8 bits like our guid_t. In some cases (i.e., on 32-bit ARM), > * this means that firmware services invoked by the kernel may assume that > * efi_guid_t* arguments are 32-bit aligned, and use memory accessors that > - * do not tolerate misalignment. So let's set the minimum alignment to 32 bits. > + * do not tolerate misalignment. > * > * Note that the UEFI spec as well as some comments in the EDK2 code base > * suggest that EFI_GUID should be 64-bit aligned, but this appears to be > * a mistake, given that no code seems to exist that actually enforces that > * or relies on it. > */ > -typedef guid_t efi_guid_t __aligned(__alignof__(u32)); > +typedef struct { > + u32 a; > + u16 b; > + u16 c; > + u8 d[8]; > +} 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) > + (efi_guid_t){ a, b, c, { d0,d1,d2,d3,d4,d5,d6,d7 }} > > /* > * Generic EFI table header > @@ -598,8 +603,8 @@ efi_guidcmp (efi_guid_t left, efi_guid_t right) > static inline char * > efi_guid_to_str(efi_guid_t *guid, char *out) > { > - sprintf(out, "%pUl", guid->b); > - return out; > + sprintf(out, "%pUl", guid); > + return out; > } > > extern void efi_init (void); > -- > 2.30.1 >
On Wed, 10 Mar 2021 at 23:21, Nathan Chancellor <nathan@kernel.org> wrote: > > On Wed, Mar 10, 2021 at 09:12:10AM +0100, Ard Biesheuvel wrote: > > Commit 494c704f9af0 ("efi: Use 32-bit alignment for efi_guid_t") updated > > the type definition of efi_guid_t to ensure that it always appears > > sufficiently aligned (the UEFI spec is ambiguous about this, but given > > the fact that its EFI_GUID type is defined in terms of a struct carrying > > a uint32_t, the natural alignment is definitely >= 32 bits). > > > > However, we missed the EFI_GUID() macro which is used to instantiate > > efi_guid_t literals: that macro is still based on the guid_t type, > > which does not have a minimum alignment at all. This results in warnings > > such as > > > > In file included from drivers/firmware/efi/mokvar-table.c:35: > > include/linux/efi.h:1093:34: warning: passing 1-byte aligned argument to > > 4-byte aligned parameter 2 of 'get_var' may result in an unaligned pointer > > access [-Walign-mismatch] > > status = get_var(L"SecureBoot", &EFI_GLOBAL_VARIABLE_GUID, NULL, &size, > > ^ > > include/linux/efi.h:1101:24: warning: passing 1-byte aligned argument to > > 4-byte aligned parameter 2 of 'get_var' may result in an unaligned pointer > > access [-Walign-mismatch] > > get_var(L"SetupMode", &EFI_GLOBAL_VARIABLE_GUID, NULL, &size, &setupmode); > > > > The distinction only matters on CPUs that do not support misaligned loads > > fully, but 32-bit ARM's load-multiple instructions fall into that category, > > and these are likely to be emitted by the compiler that built the firmware > > for loading word-aligned 128-bit GUIDs from memory > > > > Instead of bodging this further, let's simply switch to our own definition > > of efi_guid_t that carries a uint32_t as well. Since efi_guid_t is used as > > an opaque type everywhere in the EFI code, this is only a minor code change. > > > > Reported-by: Nathan Chancellor <nathan@kernel.org> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > I ran this through my series of 32-bit and 64-bit x86 builds and I did > not see any additional warnings added because of it. > > Reviewed-by: Nathan Chancellor <nathan@kernel.org> > Tested-by: Nathan Chancellor <nathan@kernel.org> > Thanks all, but I am going to drop these, as I have decided to fix it in a different way after all. > > --- > > > > I am currently testing this change via my for-kernelci branch. Please give > > this some soak time in the other CIs that we have access to. > > > > include/linux/efi.h | 15 ++++++++++----- > > 1 file changed, 10 insertions(+), 5 deletions(-) > > > > diff --git a/include/linux/efi.h b/include/linux/efi.h > > index 8710f5710c1d..f39e9ec7485f 100644 > > --- a/include/linux/efi.h > > +++ b/include/linux/efi.h > > @@ -63,17 +63,22 @@ typedef void *efi_handle_t; > > * is 32 bits not 8 bits like our guid_t. In some cases (i.e., on 32-bit ARM), > > * this means that firmware services invoked by the kernel may assume that > > * efi_guid_t* arguments are 32-bit aligned, and use memory accessors that > > - * do not tolerate misalignment. So let's set the minimum alignment to 32 bits. > > + * do not tolerate misalignment. > > * > > * Note that the UEFI spec as well as some comments in the EDK2 code base > > * suggest that EFI_GUID should be 64-bit aligned, but this appears to be > > * a mistake, given that no code seems to exist that actually enforces that > > * or relies on it. > > */ > > -typedef guid_t efi_guid_t __aligned(__alignof__(u32)); > > +typedef struct { > > + u32 a; > > + u16 b; > > + u16 c; > > + u8 d[8]; > > +} 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) > > + (efi_guid_t){ a, b, c, { d0,d1,d2,d3,d4,d5,d6,d7 }} > > > > /* > > * Generic EFI table header > > @@ -598,8 +603,8 @@ efi_guidcmp (efi_guid_t left, efi_guid_t right) > > static inline char * > > efi_guid_to_str(efi_guid_t *guid, char *out) > > { > > - sprintf(out, "%pUl", guid->b); > > - return out; > > + sprintf(out, "%pUl", guid); > > + return out; > > } > > > > extern void efi_init (void); > > -- > > 2.30.1 > >
diff --git a/include/linux/efi.h b/include/linux/efi.h index 8710f5710c1d..f39e9ec7485f 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -63,17 +63,22 @@ typedef void *efi_handle_t; * is 32 bits not 8 bits like our guid_t. In some cases (i.e., on 32-bit ARM), * this means that firmware services invoked by the kernel may assume that * efi_guid_t* arguments are 32-bit aligned, and use memory accessors that - * do not tolerate misalignment. So let's set the minimum alignment to 32 bits. + * do not tolerate misalignment. * * Note that the UEFI spec as well as some comments in the EDK2 code base * suggest that EFI_GUID should be 64-bit aligned, but this appears to be * a mistake, given that no code seems to exist that actually enforces that * or relies on it. */ -typedef guid_t efi_guid_t __aligned(__alignof__(u32)); +typedef struct { + u32 a; + u16 b; + u16 c; + u8 d[8]; +} 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) + (efi_guid_t){ a, b, c, { d0,d1,d2,d3,d4,d5,d6,d7 }} /* * Generic EFI table header @@ -598,8 +603,8 @@ efi_guidcmp (efi_guid_t left, efi_guid_t right) static inline char * efi_guid_to_str(efi_guid_t *guid, char *out) { - sprintf(out, "%pUl", guid->b); - return out; + sprintf(out, "%pUl", guid); + return out; } extern void efi_init (void);
Commit 494c704f9af0 ("efi: Use 32-bit alignment for efi_guid_t") updated the type definition of efi_guid_t to ensure that it always appears sufficiently aligned (the UEFI spec is ambiguous about this, but given the fact that its EFI_GUID type is defined in terms of a struct carrying a uint32_t, the natural alignment is definitely >= 32 bits). However, we missed the EFI_GUID() macro which is used to instantiate efi_guid_t literals: that macro is still based on the guid_t type, which does not have a minimum alignment at all. This results in warnings such as In file included from drivers/firmware/efi/mokvar-table.c:35: include/linux/efi.h:1093:34: warning: passing 1-byte aligned argument to 4-byte aligned parameter 2 of 'get_var' may result in an unaligned pointer access [-Walign-mismatch] status = get_var(L"SecureBoot", &EFI_GLOBAL_VARIABLE_GUID, NULL, &size, ^ include/linux/efi.h:1101:24: warning: passing 1-byte aligned argument to 4-byte aligned parameter 2 of 'get_var' may result in an unaligned pointer access [-Walign-mismatch] get_var(L"SetupMode", &EFI_GLOBAL_VARIABLE_GUID, NULL, &size, &setupmode); The distinction only matters on CPUs that do not support misaligned loads fully, but 32-bit ARM's load-multiple instructions fall into that category, and these are likely to be emitted by the compiler that built the firmware for loading word-aligned 128-bit GUIDs from memory Instead of bodging this further, let's simply switch to our own definition of efi_guid_t that carries a uint32_t as well. Since efi_guid_t is used as an opaque type everywhere in the EFI code, this is only a minor code change. Reported-by: Nathan Chancellor <nathan@kernel.org> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- I am currently testing this change via my for-kernelci branch. Please give this some soak time in the other CIs that we have access to. include/linux/efi.h | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)