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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

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