* [PATCH v2] efi: use 32-bit alignment for efi_guid_t literals
@ 2021-03-18 18:20 Ard Biesheuvel
2021-03-18 20:21 ` Nick Desaulniers
2021-03-18 23:50 ` Nathan Chancellor
0 siblings, 2 replies; 3+ messages in thread
From: Ard Biesheuvel @ 2021-03-18 18:20 UTC (permalink / raw)
To: linux-efi; +Cc: nathan, ndesaulniers, 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
So re-implement the initializer in terms of our own efi_guid_t type, so that
the alignment becomes a property of the literal's type.
Reported-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
include/linux/efi.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 8710f5710c1d..6b5d36babfcc 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -72,8 +72,10 @@ typedef void *efi_handle_t;
*/
typedef guid_t efi_guid_t __aligned(__alignof__(u32));
-#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)
+#define EFI_GUID(a, b, c, d...) (efi_guid_t){ { \
+ (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, ((a) >> 24) & 0xff, \
+ (b) & 0xff, ((b) >> 8) & 0xff, \
+ (c) & 0xff, ((c) >> 8) & 0xff, d } }
/*
* Generic EFI table header
--
2.30.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] efi: use 32-bit alignment for efi_guid_t literals
2021-03-18 18:20 [PATCH v2] efi: use 32-bit alignment for efi_guid_t literals Ard Biesheuvel
@ 2021-03-18 20:21 ` Nick Desaulniers
2021-03-18 23:50 ` Nathan Chancellor
1 sibling, 0 replies; 3+ messages in thread
From: Nick Desaulniers @ 2021-03-18 20:21 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-efi, Nathan Chancellor, Sedat Dilek
On Thu, Mar 18, 2021 at 11:20 AM 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
>
> So re-implement the initializer in terms of our own efi_guid_t type, so that
> the alignment becomes a property of the literal's type.
>
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Nice use of a variadic macro, and thanks for the patch.
494c704f9af0 landed in v5.1-rc1, would a Fixes: tag be appropriate?
Also, a tag like
Link: https://github.com/ClangBuiltLinux/linux/issues/1327
would help us track when/where this lands, in case the issue ever
comes up again, in branches of the stable tree for example.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
It might be further possible to share more code between EFI_GUID and
GUID_INIT if the cast for the literals are factored out, but this
looks fine to me as is. Too much macro nesting gets hard to follow
anyways.
> ---
> include/linux/efi.h | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 8710f5710c1d..6b5d36babfcc 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -72,8 +72,10 @@ typedef void *efi_handle_t;
> */
> typedef guid_t efi_guid_t __aligned(__alignof__(u32));
>
> -#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)
> +#define EFI_GUID(a, b, c, d...) (efi_guid_t){ { \
> + (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, ((a) >> 24) & 0xff, \
> + (b) & 0xff, ((b) >> 8) & 0xff, \
> + (c) & 0xff, ((c) >> 8) & 0xff, d } }
>
> /*
> * Generic EFI table header
> --
> 2.30.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] efi: use 32-bit alignment for efi_guid_t literals
2021-03-18 18:20 [PATCH v2] efi: use 32-bit alignment for efi_guid_t literals Ard Biesheuvel
2021-03-18 20:21 ` Nick Desaulniers
@ 2021-03-18 23:50 ` Nathan Chancellor
1 sibling, 0 replies; 3+ messages in thread
From: Nathan Chancellor @ 2021-03-18 23:50 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-efi, ndesaulniers
On Thu, Mar 18, 2021 at 07:20:46PM +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
>
> So re-implement the initializer in terms of our own efi_guid_t type, so that
> the alignment becomes a property of the literal's type.
>
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Reviewed-by: Nathan Chancellor <nathan@kernel.org>
Tested-by: Nathan Chancellor <nathan@kernel.org>
> ---
> include/linux/efi.h | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 8710f5710c1d..6b5d36babfcc 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -72,8 +72,10 @@ typedef void *efi_handle_t;
> */
> typedef guid_t efi_guid_t __aligned(__alignof__(u32));
>
> -#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)
> +#define EFI_GUID(a, b, c, d...) (efi_guid_t){ { \
> + (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, ((a) >> 24) & 0xff, \
> + (b) & 0xff, ((b) >> 8) & 0xff, \
> + (c) & 0xff, ((c) >> 8) & 0xff, d } }
>
> /*
> * Generic EFI table header
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-03-18 23:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-18 18:20 [PATCH v2] efi: use 32-bit alignment for efi_guid_t literals Ard Biesheuvel
2021-03-18 20:21 ` Nick Desaulniers
2021-03-18 23:50 ` Nathan Chancellor
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).