efi: use 32-bit alignment for efi_guid_t literals

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 <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---

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(-)

include/linux/efi.h +10 -6

index 8710f5710c1d..f39e9ec7485f 100644
@@ -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

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 <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---

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(-)

include/linux/efi.h +10 -5

index 8710f5710c1d..f39e9ec7485f 100644
@@ -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);

Signature · 1 line

2.30.1

Quoted 31 lines · Ard Biesheuvel

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.

Quoted 2 lines

Reported-by Nathan Chancellor <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

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 <[email protected]>

Tested-by Nathan Chancellor <[email protected]>

Quoted 52 lines

---

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(-)

include/linux/efi.h +10 -6

index 8710f5710c1d..f39e9ec7485f 100644
@@ -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