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

[Oliver Steffen]

On Wed, Sep 13, 2023 at 1:43 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 13 Sept 2023 at 13:42, Vladimir 'phcoder' Serbinenko
> <phcoder@gmail.com> wrote:
> >
> >
> >
> > 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
>
> Tried what exactly? This is just copy/paste from the existing tree

With

diff --git a/include/grub/types.h b/include/grub/types.h
index 0d96006fe..fb0dfc8b6 100644
--- a/include/grub/types.h
+++ b/include/grub/types.h
@@ -376,7 +376,7 @@ struct grub_guid
   grub_uint16_t data2;
   grub_uint16_t data3;
   grub_uint8_t data4[8];
-} GRUB_PACKED;
+};
 typedef struct grub_guid grub_guid_t;
 #endif /* ! GRUB_TYPES_HEADER */

I get:

[...]
commands/efi/loadbios.c: In function ‘fake_bios_data’:
commands/efi/loadbios.c:109:9: warning: taking address of packed
member of ‘struct grub_efi_configuration_table’ may result in an
unaligned pointer value [-Waddress-of-packed-member]
  109 |         &grub_efi_system_table->configuration_table[i].vendor_guid;
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
commands/probe.c: In function ‘grub_cmd_probe’:
commands/probe.c:132:22: warning: taking address of packed member of
‘struct grub_gpt_partentry’ may result in an unaligned pointer value
[-Waddress-of-packed-member]
  132 |               guid = &entry.guid;
      |                      ^~~~~~~~~~~
[...]

also in a couple of other places.


  struct grub_efi_configuration_table
  {
    grub_guid_t vendor_guid;
    void *vendor_table;
  } GRUB_PACKED;

and

  struct grub_gpt_partentry
  {
    grub_guid_t type;
    grub_guid_t guid;
    grub_uint64_t start;
    grub_uint64_t end;
    grub_uint64_t attrib;
    char name[72];
  } GRUB_PACKED;



Is this what you mean, Vladimir?

Oliver