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

[Oliver Steffen]

Quoting Vladimir 'phcoder' Serbinenko (2023-08-15 18:14:11)
>
>
> Le mar. 15 août 2023, 14:00, Laszlo Ersek <[1]lersek@redhat.com> a écrit :
>
>     On 8/15/23 10:17, Ard Biesheuvel wrote:
>     > On Tue, 15 Aug 2023 at 05:42, Vladimir 'phcoder' Serbinenko
>     > <[2]phcoder@gmail.com> wrote:
>     >>>
>     >>> .
>     >>>
>     >>> Ard thinks that a 64 bit alignment for EFI GUIDs is a mistake - see 
> [1]
>     >>> and [2]. Hence Linux uses `__aligned(__alignof__(u32))` for
>     "efi_guid_t"
>     >>> since 2019.
>     >>
>     >>
>     >> My patch presents a reliability paradigm: accept all inputs, produce
>     outputs that are always aligned to 8-byte. It can make sense even though I
>     agree that requiring 8-byte alignment for UUID is weird. Yet it's 
> difficult
>     to know if some obscure firmware does rely on it in some one in a thousand
>     edge case
>     >
>     > +1
>     >
>     > The Linux side change ensures that the OS does not present misaligned
>     > GUIDs to the firmware.
>     >
>     > The problem with 8-byte alignment is that it deviates from EDK2, and
>     > could result in struct layout changes due to padding.
>
> See my audit in previous message
> > > Given that EDK2
>
>     > uses 32-bit alignment only, some of the struct types it defines might
>     > suddenly need the __packed attribute on 32-bit builds if the alignment
>     > of the GUID type is increased to 64 bits.
>     >
>     > E.g.,
>     >
>     > struct {
>     >   void *
>     >   guid
>     > }
>     >
>     > will have no padding in 32-bit EDK2 based firmware builds, even if the
>     > spec claims that GUIDs are 64-bit aligned.
>     >
>
> Yes, this is weird but in case of reference implementation contradicting spec,
> then reference implementation takes priority in practice.
>
>
>     > So the lowest risk change for Linux was to increase to 32-bit
>     > alignment, as it fixes any potential issues with misaligned
>     > load-multiple instructions on ARM, and other architectures don't care
>     > that much about alignment anyway.
>     >
>
>     This is terrible.
>
>     First, the Itanium firmware in question clearly enforces the 8-byte
>     alignment. (BTW, have we checked Itanium with a 4-byte but not 8-byte
>     alignment?)
>
> No, we didn't  AFAIK
>
>     specifies a binding for Itanium. And, we're having this discussion in
>     the first place because of Itanium. So we need to make up our minds
>     about adhering to the 8-byte alignment or not.
>
> I'm biased towards allocating at 8-byte alignment but accepting any alignment
>
>
>     Second, the fact that edk2 does not align EFI_GUID to 8 bytes, contrary
>     to the spec, is absolutely terrible. I don't understand then how it
>     worked on Itanium before Itanium support was killed.
>
> No members in guid have uint64_t type so most likely only 32-bit alignment was
> used in most cases. 64-bit alignment allows accessing guid as 2 uint64_t's but
> I doubt it was ever widely used. A rare use might still happen though.
>
>
>     What I can imagine is the following situation:
>     - the UEFI spec does not match reality (the reference implementation),
>     and the actually required alignment is 4 bytes only
>     - Itanium is happy with the 4 bytes alignment too (IIUC grub's malloc
>     aligns at 1 byte by default, so the 4-byte alignment could still be
>     violated in practice)
>
> grub-malloc is 16-byte aligned.
> The problem is that stack and static allocations are aligned only to type
> alignment.

I am not sure what the best way forward is now, but we at least have the
patches from Vladimir (thanks!).

Pedro, Adrian, could you - if you get a chance - try them with a 4 byte
alignment too?

Thanks a lot, everybody!

Oliver