| Message ID | 20250618122008.264294-1-vkuznets@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | efi: Fix .data section size calculations when .sbat is present | expand |
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 >
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.
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.
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));
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(-)