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

[Ard Biesheuvel <ardb@kernel.org>]

On Fri, 2 May 2025 at 15:46, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
> 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').
>

Ah I misread your patch - I thought .sbat was between .text and .data,
which arguably makes more sense.