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

Ard Biesheuvel
2025-05-02T13:01:58Z

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?

> 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?

> 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.