Re: [PATCH] efi: Fix .data section size calculations when .sbat is present

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Ard Biesheuvel <ardb@kernel.org> writes:

> On Thu, 19 Jun 2025 at 17:29, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Wed, 18 Jun 2025 at 14:20, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>> >
>> > Commit 0f9a1739dd0e ("efi: zboot specific mechanism for embedding SBAT
>> > section") neglected to adjust the sizes of the .data section when
>> > CONFIG_EFI_SBAT_FILE is set. As the result, the produced PE binary is
>> > incorrect and some tools complain about it. E.g. 'sbsign' reports:
>> >
>> >  # sbsign --key my.key --cert my.crt arch/arm64/boot/vmlinuz.efi
>> >  warning: file-aligned section .data extends beyond end of file
>> >  warning: checksum areas are greater than image size. Invalid section table?
>> >
>> > Note, '__data_size' was also used in the PE optional header. The field is
>> > supposed to reflect "The size of the initialized data section, or the sum
>> > of all such sections if there are multiple data sections.". While OVMF
>> > based firmware doesn't seem to care much about what's there, it sounds like
>> > including .sbat in the calculation is more correct.
>> >
>> > Fixes: 0f9a1739dd0e ("efi: zboot specific mechanism for embedding SBAT section")
>> > Reported-by: Heinrich Schuchardt <heinrich.schuchardt@gmx.de>
>> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> > ---
>> >  drivers/firmware/efi/libstub/zboot-header.S | 2 +-
>> >  drivers/firmware/efi/libstub/zboot.lds      | 6 ++++--
>> >  2 files changed, 5 insertions(+), 3 deletions(-)
>> >
>>
>> Thanks for the fix.
>>
>> > diff --git a/drivers/firmware/efi/libstub/zboot-header.S b/drivers/firmware/efi/libstub/zboot-header.S
>> > index b6431edd0fc9..65df5f52e138 100644
>> > --- a/drivers/firmware/efi/libstub/zboot-header.S
>> > +++ b/drivers/firmware/efi/libstub/zboot-header.S
>> > @@ -41,7 +41,7 @@ __efistub_efi_zboot_header:
>> >         .short          .Lpe_opt_magic
>> >         .byte           0, 0
>> >         .long           _etext - .Lefi_header_end
>> > -       .long           __data_size
>> > +       .long           __all_data_size
>>
>> Frankly, I'm not sure if this is even worth the hassle.
>>
>> There is also a 'size of uninitialized data' field, but given that the
>> .data section has both initialized data and uninitialized data, and
>> the fact that no loaders appear to care about these fields, let's just
>> not bother.
>>
>
> ... or add .sbat to SizeOfCode instead. (the preceding field)
>

I was wondering how to make it consistent with yet-unmerged x86
patch. In v3:
https://lore.kernel.org/linux-efi/20250603091951.57775-1-vkuznets@redhat.com/
.sbat was accounted as SizeOfCode but the section is
"IMAGE_SCN_CNT_INITIALIZED_DATA", not "IMAGE_SCN_CNT_CODE" so I sent v4
yesterday:
https://lore.kernel.org/linux-efi/20250619151759.355893-1-vkuznets@redhat.com/
which made .sbat accounted in SizeOfInitializedData.

-- 
Vitaly