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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Ard Biesheuvel <ardb@kernel.org> writes:

> On Fri, 20 Jun 2025 at 10:19, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>>
>> 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.
>>
>
> I agree that making it consistent is preferred, but I really don't see
> the point of obsessing over this - the field is basically unused in
> practice.
>

OK, let me then just do v2 of this patch with '__all_data_size' magic
dropped.

-- 
Vitaly