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 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. On x86, arch/x86/boot/bzImage is composed of setup.bin and
vmlinux.bin and the later is produced from vmlinux with

 objcopy -O binary -R .note -R .comment -S arch/x86/boot/compressed/vmlinux arch/x86/boot/vmlinux.bin

"objcopy -O binary" basically dumps memory representation of vmlinux. In
case we put '.sbat' before '.bss' we get:

Sections:
Idx Name          Size      VMA               LMA               File off  Algn
...
  4 .data         00002000  0000000000aef000  0000000000aef000  00af0000  2**12
                  CONTENTS, ALLOC, LOAD, DATA
  5 .sbat         00001000  0000000000af1000  0000000000af1000  00af2000  2**12
                  CONTENTS, ALLOC, LOAD, READONLY, DATA
  6 .bss          00023078  0000000000af2000  0000000000af2000  00af3000  2**12
                  ALLOC
  7 .pgtable      00021000  0000000000b16000  0000000000b16000  00af3000  2**12
                  ALLOC
...

so we can't have a single '.data' section in PE covering both '.data'
and '.bss'/'.pgtable' as '.sbat' is in the middle and we want it to be a
separate section. If we try putting '.sbat' after '.bss' we are going to
get a hole size of '.bss' + '.pgtable' in arch/x86/boot/vmlinux.bin
because 'objcopy -O binary' is not going to squeeze anything (and it has
no idea what '.sbat' is and if it can be moved).

The problem is similar for zboot. I have two ideas:
1) Get back to the idea of putting '.sbat' between '.text' and '.data'
(was in my RFC). 

2) Introduce a separate '.bss' section to the PE binary, basically:

diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index b5c79f43359b..dae8202705c4 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -197,7 +197,7 @@ pecompat_fstart:
                IMAGE_SCN_MEM_EXECUTE           # Characteristics
 
        .ascii  ".data\0\0\0"
-       .long   ZO__end - ZO__data              # VirtualSize
+       .long   ZO__edata - ZO__data            # VirtualSize
        .long   setup_size + ZO__data           # VirtualAddress
        .long   ZO__edata - ZO__data            # SizeOfRawData
        .long   setup_size + ZO__data           # PointerToRawData
@@ -207,6 +207,17 @@ pecompat_fstart:
                IMAGE_SCN_MEM_READ              | \
                IMAGE_SCN_MEM_WRITE             # Characteristics
 
+       .ascii  ".bss\0\0\0\0"
+       .long   ZO__end - ZO__edata             # VirtualSize
+       .long   setup_size + ZO__edata          # VirtualAddress
+       .long   0                               # SizeOfRawData
+       .long   0                               # PointerToRawData
+
+       .long   0, 0, 0
+       .long   IMAGE_SCN_CNT_UNINITIALIZED_DATA| \
+               IMAGE_SCN_MEM_READ              | \
+               IMAGE_SCN_MEM_WRITE             # Characteristics
+
        .set    section_count, (. - section_table) / 40
 #endif /* CONFIG_EFI_STUB */

This way '.data' doesn't need to be the last meaninful section in
'vmlinux' and we can then pub '.sbat' right after it. I'm going to give
it a try but also I'm wondering why do we have '.data' covering '.bss'
and '.pgtable' in the first place.

-- 
Vitaly