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

[Laszlo Ersek]

On 8/15/23 10:17, Ard Biesheuvel wrote:
> On Tue, 15 Aug 2023 at 05:42, Vladimir 'phcoder' Serbinenko
> <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. 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.
> 
> 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?) Edk2 has killed off Itanium support, but the UEFI spec still
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.

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.

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)

I'd really like if edk2 changed EFI_GUID to match the spec, but a
git-grep for EFI_GUID in edk2 header files returns 800+ lines. Noone
ever will audit all those lines, to check for the potantial padding
increase that you raise. Absolutely terrible.

Either way, I'd say grub still needs two distinct types, one packed
*and* aligned (at 4 bytes), and another only packed (but not aligned).

Laszlo