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

[Ard Biesheuvel]

On Fri, 2 May 2025 at 14:10, Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> wrote:
>
> Ard Biesheuvel <ardb@xxxxxxxxxx> writes:
>
> > On Mon, 28 Apr 2025 at 12:59, Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> wrote:
> >>
> >> Ard Biesheuvel <ardb@xxxxxxxxxx> writes:
> >>
> >> > On Thu, 24 Apr 2025 at 10:10, Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> 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.