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

[Oliver Steffen]

Quoting Vladimir 'phcoder' Serbinenko (2023-08-13 09:46:45)
> Full analysis: gpt_partentry can be marked as aligned8. But following are
> problem:
> * protocols_per_handle may return unaligned guids
> * configuration_tables array may be unaligned. On efi32 every entry is 20
> bytes, so guids can't be 8-byte aligned
> * The worst offender is device path as it's packed, unpredictable and contains
> uuids.
> * Efiemu gets guid as argument and probably should be able to handle unaligned
> case. So far it's x86 only and we have no mmx and co so it's not a problem
> right now unless we enable ubsan.
> All in all we do need packed guid type unless those are resolved. I attach
> patches for testing. If they work, I'll rearrange them a bit

Looking at https://uefi.org/specs/UEFI/2.10/02_Overview.html#data-types,
the problem is the alignment, not the packing.

As far as EFI is concerned, GUIDs are always 128bit in size. This means
there shall be no padding between the struct members. To be on the safe
side one could add the "packed" attribute to the struct, but the
original grub_efi_guid struct was not marked like that and it worked.
Whenever we had over GUIDs to EFI, they have to be aligned to 64bit.

GPT GUIDs are also packed, but here we do not want the alignment
restriction.

I am not convinced that we can get away with just one type for both
cases. How about at least reducing from 3 to 2 GUID types, like:

struct grub_efi_guid
{
  grub_uint32_t data1;
  grub_uint16_t data2;
  grub_uint16_t data3;
  grub_uint8_t data4[8];
} GRUB_PACKED __attribute__ ((aligned(8)));

struct grub_guid
{
  grub_uint32_t data1;
  grub_uint16_t data2;
  grub_uint16_t data3;
  grub_uint8_t data4[8];
} GRUB_PACKED;

At least when converting from grub_guid to grub_efi_guid a copy would be
required, but I think that is a rare case. A cast would work in the
other direction (casting to a less strict alignment) so the %pG could
still be used.

Does this make sense? Or am I missing something?

Thanks,
  Oliver

>
> Le dim. 13 août 2023, 05:27, Vladimir 'phcoder' Serbinenko <[1]
> phcoder@gmail.com> a écrit :
>
>
>
>     Le dim. 13 août 2023, 00:57, Pedro Miguel Justo <[2]pmsjt@texair.net> a
>     écrit :
>
>
>
>         > On Aug 12, 2023, at 11:04, John Paul Adrian Glaubitz <[3]
>         glaubitz@physik.fu-berlin.de> wrote:
>         >
>         > Hi Daniel!
>         >
>         > On Fri, 2023-08-11 at 17:31 +0200, Daniel Kiper wrote:
>         >> On Fri, Aug 11, 2023 at 04:10:14AM -0700, Oliver Steffen wrote:
>         >>> Quoting John Paul Adrian Glaubitz (2023-08-11 10:32:17)
>         >>>> Hi Oliver!
>         >>>>
>         >>>> On Fri, 2023-05-26 at 13:35 +0200, Oliver Steffen wrote:
>         >>>>> There are 3 implementations of a GUID in Grub. Replace them with
>         a
>         >>>>> common one, placed in types.h.
>         >>>>>
>         >>>>> It uses the "packed" flavor of the GUID structs, the alignment
>         attribute
>         >>>>> is dropped, since it is not required.
>         >>>>>
>         >>>>> Signed-off-by: Oliver Steffen <[4]osteffen@redhat.com>
>         >>>>> Reviewed-by: Daniel Kiper <[5]daniel.kiper@oracle.com>
>         >>
>         >> [...]
>         >>
>         >>>> According to [1], this change broke GRUB on ia64:
>         >>>>
>         >>>> Welcome to GRUB!
>         >>>>
>         >>>> 7 0 0x00006B 0x000000000000001E unexpected trap
>         >>>> 7 0 0x000066 0x000000000000001E trap taken, number in ext PE
>         >>>> 7 0 0x00003C 0x0000000000005A00 trap taken, offset in ext PE
>         >>>>
>         >>>> I assume this is because of the strict alignment requirements on
>         ia64.
>         >>>>
>         >>>> Could you have a look?
>         >>>
>         >>> I am very sorry for this mistake.
>         >>> My goal was to unify the two GUID types we had in grub but I 
> missed
>         the
>         >>> fact that in my "solution" the alignments are not correct in all
>         cases.
>         >>>
>         >>> The quickest way out could be to revert the GUID unification and
>         printf
>         >>> format specifier commits:
>         >>>
>         >>> 6ad116e5f guid: Make use of GUID printf format specifier
>         >>> f82dbf2bd kern/misc: Add a format specifier GUIDs
>         >>> 06edd40db guid: Unify GUID types
>         >>>
>         >>> And use the explicit, long-winded format string for printing the
>         GUID
>         >>> in the bli module instead (added in the commits following those).
>         >>>
>         >>> I am open to suggestions / comments.
>         >>
>         >> Adrian, could you check what will happen when you add alignment to
>         the
>         >> grub_guid_t as it was suggested by Frank here [2]?
>         >>
>         >> Personally I would avoid adding another GUID type with just
>         alignment
>         >> requirement as the difference. Making one GUID type with always
>         enforced
>         >> alignment should not cost us a lot. Or we can enforce alignment on
>         EFI
>         >> platforms only.
>         >
>         > My Itanium hardware is not available for bootloader tests at the
>         moment, so I would
>         > like to ask Pedro Miguel Justo or Frank Scheiner to test the 
> proposed
>         fix.
>
>
>         The reason that before this unification there were a packed and 
> aligned
>         types suggest that the packed type was necessary in some cases. Frank
>         had shared the concern against his own suggestion that changing the
>         unified type to be aligned (as opposite to packed) would likely 
> regress
>         the cases where the packed might be needed/expected.
>
>         Just for kicks, I gave it a try:
>
>         ```
>         diff --git a/include/grub/types.h b/include/grub/types.h
>         index 0d96006fe..ff415c970 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;
>         +} __attribute__ ((aligned(8)));
>          typedef struct grub_guid grub_guid_t;
>
>          #endif /* ! GRUB_TYPES_HEADER */
>         ```
>
>         As hypothesized, there are places where these structs (now unified) 
> are
>         expected to be packed, meaning to have an alignment of 1 (even smaller
>         than the natural alignment of its larger member).
>
>         One of the cases where the dependency is present is in the
>         grub_gpt_partentry structure. This structure is packed and includes a
>         GUID field. One struct of an enforced alignment ’n’ cannot host a
>         member with enforced alignment ‘m’ where m > n. Thus, an 8 byte 
> aligned
>         grub_guid cannot be hosted inside a 1 byte aligned grub_gpt_partentry:
>
>
>     grub_gpt_partentry doesn't need
>
>     GRUB_PACKED. To be on a safe side we can
>
>     add compile time assert on its size
>
>
>
>         ```
>         In file included from grub-core/disk/ldm.c:26:
>         ./include/grub/gpt_partition.h:70:1: error: alignment 1 of ‘struct
>         grub_gpt_partentry’ is less than 8 [-Werror=packed-not-aligned]
>            70 | } GRUB_PACKED;
>               | ^
>         ./include/grub/gpt_partition.h:70:1: error: alignment 1 of ‘struct
>         grub_gpt_partentry’ is less than 8 [-Werror=packed-not-aligned]
>         ```
>
>         Now, we could go this rabbit hole on and try to hack
>         grub_gpt_partentry, or special-case the GUID inside grub_gpt_partentry
>         to be packed… but at which point will I just end up with the thing we
>         were trying to avoid: more than one definition of GUID?
>
>         >
>         > Thanks,
>         > Adrian
>         >
>         > --
>         > .''`.  John Paul Adrian Glaubitz
>         > : :' :  Debian Developer
>         > `. `'   Physicist
>         >  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913
>
>
>
> References:
>
> [1] mailto:phcoder@gmail.com
> [2] mailto:pmsjt@texair.net
> [3] mailto:glaubitz@physik.fu-berlin.de
> [4] mailto:osteffen@redhat.com
> [5] mailto:daniel.kiper@oracle.com

--
🎩Oliver Steffen (he/him) - Software Engineer, Virtualization
Red Hat GmbH <https://www.redhat.com/de/global/dach>,
Registered seat: Werner-von-Siemens-Ring 12, D-85630 Grasbrunn, Germany
Commercial register: Amtsgericht München/Munich, HRB 153243,
Managing Directors: Ryan Barnhart, Charles Cachera, Michael O'Neill,
Amy Ross

Everyone has different working hours… Please do not feel obligated to
reply outside of your normal work schedule.