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

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

[Vitaly Kuznetsov]

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(-)

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
 	.long		0
 	.long		__efistub_efi_zboot_entry - .Ldoshdr
 	.long		.Lefi_header_end - .Ldoshdr
diff --git a/drivers/firmware/efi/libstub/zboot.lds b/drivers/firmware/efi/libstub/zboot.lds
index 4b8d5cd3dfa2..f423204fba0f 100644
--- a/drivers/firmware/efi/libstub/zboot.lds
+++ b/drivers/firmware/efi/libstub/zboot.lds
@@ -58,6 +58,8 @@ SECTIONS
 PROVIDE(__efistub__gzdata_size =
 		ABSOLUTE(__efistub__gzdata_end - __efistub__gzdata_start));
 
-PROVIDE(__data_rawsize = ABSOLUTE(_edata - _etext));
-PROVIDE(__data_size = ABSOLUTE(_end - _etext));
+PROVIDE(__data_rawsize = ABSOLUTE(_edata - _data));
+PROVIDE(__data_size = ABSOLUTE(_end - _data));
+/* The sum of all data sections, i.e. .data and .sbat */
+PROVIDE(__all_data_size = ABSOLUTE(_end - _etext));
 PROVIDE(__sbat_size = ABSOLUTE(_esbat - _sbat));
-- 
2.49.0

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

[Ard Biesheuvel]

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.


>         .long           0
>         .long           __efistub_efi_zboot_entry - .Ldoshdr
>         .long           .Lefi_header_end - .Ldoshdr
> diff --git a/drivers/firmware/efi/libstub/zboot.lds b/drivers/firmware/efi/libstub/zboot.lds
> index 4b8d5cd3dfa2..f423204fba0f 100644
> --- a/drivers/firmware/efi/libstub/zboot.lds
> +++ b/drivers/firmware/efi/libstub/zboot.lds
> @@ -58,6 +58,8 @@ SECTIONS
>  PROVIDE(__efistub__gzdata_size =
>                 ABSOLUTE(__efistub__gzdata_end - __efistub__gzdata_start));
>
> -PROVIDE(__data_rawsize = ABSOLUTE(_edata - _etext));
> -PROVIDE(__data_size = ABSOLUTE(_end - _etext));
> +PROVIDE(__data_rawsize = ABSOLUTE(_edata - _data));
> +PROVIDE(__data_size = ABSOLUTE(_end - _data));
> +/* The sum of all data sections, i.e. .data and .sbat */
> +PROVIDE(__all_data_size = ABSOLUTE(_end - _etext));
>  PROVIDE(__sbat_size = ABSOLUTE(_esbat - _sbat));
> --
> 2.49.0
>

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

[Ard Biesheuvel]

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)

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

[Vitaly Kuznetsov]

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

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

[Ard Biesheuvel]

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.

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

[Vitaly Kuznetsov]

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