Re: [PATCH v9 02/11] Unify GUID types

[Vladimir 'phcoder' Serbinenko]

Le mer. 13 sept. 2023, 12:33, Ard Biesheuvel <ardb@kernel.org> a écrit :

On Wed, 13 Sept 2023 at 12:18, John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
>
> Hi Oliver!
>
> On Wed, 2023-09-13 at 12:14 +0200, Oliver Steffen wrote:
> > On Wed, Sep 13, 2023 at 6:10 AM Pedro Miguel Justo <pmsjt@texair.net> wrote:
> > >
> > >
> > > I can confirm that, taking [1][2] and making [3] on top of it, my Montvale-based rx2660 machine still boots fine.
> >
> > Wonderful! Thanks for testing!
>
> Are you going to submit a patch to fix the issue with the new information?
>
> Would be great if the bug could be fixed before the 2.12 release.
>

Yes, this needs to be fixed. The EFI GUID type should not have the
packed attribute in the general case, only in places where it could
really appear misaligned (e.g., in device path nodes), although I
suspect that adding the packed attribute to the outer struct would be
sufficient there (given that the guid struct has no internal padding
so the attribute only affects its minimum alignment)

E.g,

struct grub_efi_vendor_device_path
{
  grub_efi_device_path_t header;
  grub_guid_t vendor_guid;
  grub_efi_uint8_t vendor_defined_data[0];
} GRUB_PACKED;

Tried this. Compiler doesn't allow it

Here the GUID is already 32-bit aligned due to the size of the preceding field

I think simply dropping the PACKED attribute from grub_guid_t should
be sufficient here - this will affect the placement of all the
grub_guid_t global and automatic variables that are passed to the
firmware APIs by reference. If needed we could bring back the
efi_packed_guid_t as well, but I'm not sure it is necessary.


Note that this affects 32-bit ARM as well: GRUB may pass misaligned
GUIDs to APIs such as Get/SetVariable and this could crash the
firmware if it assumes GUIDs are 32-bit aligned.