* [PATCH] efi: use 32-bit alignment for efi_guid_t literals
@ 2021-03-10 8:12 Ard Biesheuvel
2021-03-10 8:32 ` Ard Biesheuvel
2021-03-10 22:21 ` Nathan Chancellor
0 siblings, 2 replies; 4+ messages in thread
From: Ard Biesheuvel @ 2021-03-10 8:12 UTC (permalink / raw)
To: linux-efi; +Cc: linux-arm-kernel, Ard Biesheuvel, Nathan Chancellor
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(-)
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] efi: use 32-bit alignment for efi_guid_t literals
2021-03-10 8:12 [PATCH] efi: use 32-bit alignment for efi_guid_t literals Ard Biesheuvel
@ 2021-03-10 8:32 ` Ard Biesheuvel
2021-03-10 22:21 ` Nathan Chancellor
1 sibling, 0 replies; 4+ messages in thread
From: Ard Biesheuvel @ 2021-03-10 8:32 UTC (permalink / raw)
To: linux-efi; +Cc: Linux ARM, Nathan Chancellor
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
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] efi: use 32-bit alignment for efi_guid_t literals
2021-03-10 8:12 [PATCH] efi: use 32-bit alignment for efi_guid_t literals Ard Biesheuvel
2021-03-10 8:32 ` Ard Biesheuvel
@ 2021-03-10 22:21 ` Nathan Chancellor
2021-03-18 17:52 ` Ard Biesheuvel
1 sibling, 1 reply; 4+ messages in thread
From: Nathan Chancellor @ 2021-03-10 22:21 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-efi, linux-arm-kernel
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>
> ---
>
> 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
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] efi: use 32-bit alignment for efi_guid_t literals
2021-03-10 22:21 ` Nathan Chancellor
@ 2021-03-18 17:52 ` Ard Biesheuvel
0 siblings, 0 replies; 4+ messages in thread
From: Ard Biesheuvel @ 2021-03-18 17:52 UTC (permalink / raw)
To: Nathan Chancellor; +Cc: linux-efi, Linux ARM
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
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-03-18 17:54 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-10 8:12 [PATCH] efi: use 32-bit alignment for efi_guid_t literals Ard Biesheuvel
2021-03-10 8:32 ` Ard Biesheuvel
2021-03-10 22:21 ` Nathan Chancellor
2021-03-18 17:52 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).