Re: [PATCH] efi: use 32-bit alignment for efi_guid_t literals

[Ard Biesheuvel <ardb@kernel.org>]

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