Re: [PATCH 2/2] x86/efi: Implement support for embedding SBAT data for x86

Vitaly Kuznetsov
2025-05-02T13:48:40Z

Ard Biesheuvel <ardb@kernel.org> writes:

> On Fri, 2 May 2025 at 14:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>>
>> Ard Biesheuvel <ardb@kernel.org> writes:
>>
>> > On Mon, 28 Apr 2025 at 12:59, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>> >>
>> >> Ard Biesheuvel <ardb@kernel.org> writes:
>> >>
>> >> > On Thu, 24 Apr 2025 at 10:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>>
>> ...
>>
>> >> >> $(obj)/vmlinux: $(vmlinux-objs-y) $(vmlinux-libs-y) FORCE
>> >> >> $(call if_changed,ld)
>> >> >>
>> >> >> diff --git a/arch/x86/boot/compressed/vmlinux.lds.S b/arch/x86/boot/compressed/vmlinux.lds.S
>> >> >> index 3b2bc61c9408..d0a27905de90 100644
>> >> >> --- a/arch/x86/boot/compressed/vmlinux.lds.S
>> >> >> +++ b/arch/x86/boot/compressed/vmlinux.lds.S
>> >> >> @@ -49,9 +49,22 @@ SECTIONS
>> >> >> *(.data.*)
>> >> >>
>> >> >> /* Add 4 bytes of extra space for the obsolete CRC-32 checksum */
>> >> >> +#ifndef CONFIG_EFI_SBAT
>> >> >> . = ALIGN(. + 4, 0x200);
>> >> >> +#else
>> >> >> + /* Avoid gap between '.data' and '.sbat' */
>> >> >> + . = ALIGN(. + 4, 0x1000);
>> >> >> +#endif
>> >> >> _edata = . ;
>> >> >> }
>> >> >> +#ifdef CONFIG_EFI_SBAT
>> >> >> + .sbat : ALIGN(0x1000) {
>> >> >> + _sbat = . ;
>> >> >> + *(.sbat)
>> >> >> + _esbat = ALIGN(0x200);
>> >> >> + . = _esbat;
>> >> >> + }
>> >> >> +#endif
>> >> >> . = ALIGN(L1_CACHE_BYTES);
>> >> >> .bss : {
>> >> >> _bss = . ;
>> >> >
>> >> > This looks a bit odd - see below
>> >> >
>> >> >> diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
>> >> >> index b5c79f43359b..ab851490ef74 100644
>> >> >> --- a/arch/x86/boot/header.S
>> >> >> +++ b/arch/x86/boot/header.S
>> >> >> @@ -207,6 +207,19 @@ pecompat_fstart:
>> >> >> IMAGE_SCN_MEM_READ | \
>> >> >> IMAGE_SCN_MEM_WRITE # Characteristics
>> >> >>
>> >> >> +#ifdef CONFIG_EFI_SBAT
>> >> >> + .ascii ".sbat\0\0\0"
>> >> >> + .long ZO__esbat - ZO__sbat # VirtualSize
>> >> >> + .long setup_size + ZO__sbat # VirtualAddress
>> >> >> + .long ZO__esbat - ZO__sbat # SizeOfRawData
>> >> >> + .long setup_size + ZO__sbat # PointerToRawData
>> >> >> +
>> >> >> + .long 0, 0, 0
>> >> >> + .long IMAGE_SCN_CNT_INITIALIZED_DATA | \
>> >> >> + IMAGE_SCN_MEM_READ | \
>> >> >> + IMAGE_SCN_MEM_DISCARDABLE # Characteristics
>> >> >> +#endif
>> >> >> +
>> >> >
>> >> > This puts the .sbat section at the very end of the file. However, the
>> >> > virtual size of .data is 'ZO__end - ZO__data' not 'ZO__edata -
>> >> > ZO__data', and so the .sbat section will overlap with .bss in the
>> >> > memory view of the image.
>> >>
>> >> Missed that, will fix, thanks! A stupid question though: does this
>> >> matter in practice for SBAT? I don't think anyone needs SBAT data after
>> >> kernel starts booting so we can consider it 'discarded'. BSS data can
>> >> then do whatever it wants.
>> >>
>> >
>> > It violates the PE/COFF spec, and some PE loaders and signing tools
>> > are very pedantic about the layout.
>>
>> Turns out it the problem is slightly harder to address then I initially
>> thought.
>
> Yeah I was afraid this was going to be tricky.
>
> ...
>
>> The problem is similar for zboot.
>
> How so?
>

zboot-header.S has:

.ascii ".data\0\0\0"
.long __data_size
.long _etext - .Ldoshdr
.long __data_rawsize
.long _etext - .Ldoshdr

where the difference between '__data_rawsize' and '__data_size' is:

PROVIDE(__data_rawsize = ABSOLUTE(_edata - _etext));
PROVIDE(__data_size = ABSOLUTE(_end - _etext));

and "_end" is the end of BSS. So if we put '.sbat' right after '.data'
then '.data' will cover it too (so we will get an overlap). If we put
if after '.bss' then we're going to get a hole (size of '.bss') upon
(aarch64 example):

objcopy -O binary arch/arm64/boot/vmlinuz.efi.elf arch/arm64/boot/vmlinuz.efi

as AFAIU it won't be able to squeeze the binary, it only truncates the
tail throwing away secions without content (in particular, '.sbat').

>> I have two ideas:
>> 1) Get back to the idea of putting '.sbat' between '.text' and '.data'
>> (was in my RFC).
>>
>
> This is what zboot does, no?
>

In v1 I put '.sbat' right after '.data' and before '.bss' but I didn't
think about the memory overlap problem.

>> 2) Introduce a separate '.bss' section to the PE binary, basically:
>>
>
> I'd like .sbat to be as unintrusive as we can make it, so this is my
> least preferred option.
>

This is very reasonable -- unless for some reason we belive that
separating '.bss' into its own PE section is a good idea on its own.

--
Vitaly