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.
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.