Re: [PATCH v2 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, 5 May 2025 at 17:46, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>>
>> Similar to zboot architectures, implement support for embedding SBAT data
>> for x86. Put '.sbat' section in between '.data' and '.text' as the former
>> also covers '.bss' and '.pgtable' and thus must be the last one in the
>> file.
>>
>> Note, the obsolete CRC-32 checksum (see commit 9c54baab4401 ("x86/boot:
>> Drop CRC-32 checksum and the build tool that generates it")) is gone and
>> while it would've been possible to reserve the last 4 bytes in '.sbat'
>> section too (like it's done today in '.data'), it seems to be a pointless
>> exercise: SBAT makes zero sense without a signature on the EFI binary so
>> '.sbat' won't be at the very end of the file anyway. Any tool which uses
>> the last 4 bytes of the file as a checksum is broken with signed EFI
>> binaries already.
>>
>
> Is this last paragraph still relevant? If not, please drop it.
>

Ceratinly not relevant anymore, will drop.

>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  arch/x86/boot/Makefile                 |  2 +-
>>  arch/x86/boot/compressed/Makefile      |  5 ++++
>>  arch/x86/boot/compressed/sbat.S        |  7 ++++++
>>  arch/x86/boot/compressed/vmlinux.lds.S |  8 +++++++
>>  arch/x86/boot/header.S                 | 33 +++++++++++++++++++-------
>>  drivers/firmware/efi/Kconfig           |  2 +-
>>  6 files changed, 46 insertions(+), 11 deletions(-)
>>  create mode 100644 arch/x86/boot/compressed/sbat.S
>>
>> diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
>> index 81f55da81967..5f7b52f0e7f5 100644
>> --- a/arch/x86/boot/Makefile
>> +++ b/arch/x86/boot/Makefile
>> @@ -71,7 +71,7 @@ $(obj)/vmlinux.bin: $(obj)/compressed/vmlinux FORCE
>>
>>  SETUP_OBJS = $(addprefix $(obj)/,$(setup-y))
>>
>> -sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [a-zA-Z] \(startup_32\|efi.._stub_entry\|efi\(32\)\?_pe_entry\|input_data\|kernel_info\|_end\|_ehead\|_text\|_e\?data\|z_.*\)$$/\#define ZO_\2 0x\1/p'
>> +sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [a-zA-Z] \(startup_32\|efi.._stub_entry\|efi\(32\)\?_pe_entry\|input_data\|kernel_info\|_end\|_ehead\|_text\|_e\?data\|_e\?sbat\|z_.*\)$$/\#define ZO_\2 0x\1/p'
>>
>>  quiet_cmd_zoffset = ZOFFSET $@
>>        cmd_zoffset = $(NM) $< | sed -n $(sed-zoffset) > $@
>> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
>> index fdbce022db55..1441435869cc 100644
>> --- a/arch/x86/boot/compressed/Makefile
>> +++ b/arch/x86/boot/compressed/Makefile
>> @@ -106,6 +106,11 @@ vmlinux-objs-$(CONFIG_UNACCEPTED_MEMORY) += $(obj)/mem.o
>>
>>  vmlinux-objs-$(CONFIG_EFI) += $(obj)/efi.o
>>  vmlinux-libs-$(CONFIG_EFI_STUB) += $(objtree)/drivers/firmware/efi/libstub/lib.a
>> +vmlinux-objs-$(CONFIG_EFI_SBAT) += $(obj)/sbat.o
>> +
>> +ifdef CONFIG_EFI_SBAT
>> +$(obj)/sbat.o: $(CONFIG_EFI_SBAT_FILE)
>> +endif
>>
>>  $(obj)/vmlinux: $(vmlinux-objs-y) $(vmlinux-libs-y) FORCE
>>         $(call if_changed,ld)
>> diff --git a/arch/x86/boot/compressed/sbat.S b/arch/x86/boot/compressed/sbat.S
>> new file mode 100644
>> index 000000000000..838f70a997dd
>> --- /dev/null
>> +++ b/arch/x86/boot/compressed/sbat.S
>> @@ -0,0 +1,7 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Embed SBAT data in the kernel.
>> + */
>> +       .pushsection ".sbat", "a", @progbits
>> +       .incbin CONFIG_EFI_SBAT_FILE
>> +       .popsection
>> diff --git a/arch/x86/boot/compressed/vmlinux.lds.S b/arch/x86/boot/compressed/vmlinux.lds.S
>> index 3b2bc61c9408..587ce3e7c504 100644
>> --- a/arch/x86/boot/compressed/vmlinux.lds.S
>> +++ b/arch/x86/boot/compressed/vmlinux.lds.S
>> @@ -43,6 +43,14 @@ SECTIONS
>>                 *(.rodata.*)
>>                 _erodata = . ;
>>         }
>> +#ifdef CONFIG_EFI_SBAT
>> +       .sbat : ALIGN(0x1000) {
>> +               _sbat = . ;
>> +               *(.sbat)
>> +               _esbat = ALIGN(0x1000);
>> +               . = _esbat;
>> +       }
>> +#endif
>>         .data : ALIGN(0x1000) {
>>                 _data = . ;
>>                 *(.data)
>> diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
>> index b5c79f43359b..91964818bf50 100644
>> --- a/arch/x86/boot/header.S
>> +++ b/arch/x86/boot/header.S
>> @@ -179,15 +179,17 @@ pecompat_fstart:
>>  #else
>>         .set    pecompat_fstart, setup_size
>>  #endif
>> -       .ascii  ".text"
>> -       .byte   0
>> -       .byte   0
>> -       .byte   0
>> -       .long   ZO__data
>> -       .long   setup_size
>> -       .long   ZO__data                        # Size of initialized data
>> -                                               # on disk
>> -       .long   setup_size
>> +       .ascii  ".text\0\0\0"
>> +#ifdef CONFIG_EFI_SBAT
>> +       .long   ZO__sbat                        # VirtualSize
>> +       .long   setup_size                      # VirtualAddress
>> +       .long   ZO__sbat                        # SizeOfRawData
>> +#else
>> +       .long   ZO__data                        # VirtualSize
>> +       .long   setup_size                      # VirtualAddress
>> +       .long   ZO__data                        # SizeOfRawData
>> +#endif
>> +       .long   setup_size                      # PointerToRawData
>
> Would it work if we do the following here
>
> #ifdef CONFIG_EFI_SBAT
>   .set .Ltextsize, ZO__sbat
> #else
>   .set .Ltextsize, ZO__data
> #endif
>
> and keep a single section definition for .text
>
>   .ascii  ".text\0\0\0"
>   .long   .Ltextsize                  # VirtualSize
>   .long   setup_size                  # VirtualAddress
>   .long   .Ltextsize                  # SizeOfRawData
>   .long   setup_size                  # PointerToRawData
>

As we already have '#ifdef CONFIG_EFI_SBAT' below I'd suggest we set
textsize there, basically:

@@ -199,16 +194,20 @@ pecompat_fstart:
                IMAGE_SCN_MEM_EXECUTE           # 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
+       .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
+
+       .set textsize, ZO__sbat
+#else
+       .set textsize, ZO__data
 #endif
 
        .ascii  ".data\0\0\0"

and nobody seems to care that we use it first and define/set it later.

BTW, does '.L' prefix you suggest has a meaning here? I see we don't use
it for e.g. 'pecompat_fstart', 'section_count'.

>
>>         .long   0                               # PointerToRelocations
>>         .long   0                               # PointerToLineNumbers
>>         .word   0                               # NumberOfRelocations
>> @@ -196,6 +198,19 @@ pecompat_fstart:
>>                 IMAGE_SCN_MEM_READ              | \
>>                 IMAGE_SCN_MEM_EXECUTE           # Characteristics
>>
>> +#ifdef CONFIG_EFI_SBAT
>> +       .ascii ".sbat\0\0\0"
>
> Inconsistent indentation? ^^^
>

Yep, fixing.

>> +       .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
>> +
>>         .ascii  ".data\0\0\0"
>>         .long   ZO__end - ZO__data              # VirtualSize
>>         .long   setup_size + ZO__data           # VirtualAddress
>> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
>> index db8c5c03d3a2..16baa038d412 100644
>> --- a/drivers/firmware/efi/Kconfig
>> +++ b/drivers/firmware/efi/Kconfig
>> @@ -286,7 +286,7 @@ config EFI_SBAT
>>
>>  config EFI_SBAT_FILE
>>         string "Embedded SBAT section file path"
>> -       depends on EFI_ZBOOT
>> +       depends on EFI_ZBOOT || (EFI_STUB && X86)
>>         help
>>           SBAT section provides a way to improve SecureBoot revocations of UEFI
>>           binaries by introducing a generation-based mechanism. With SBAT, older
>> --
>> 2.49.0
>>
>
> Modulo the nits, I think this patch looks fine, but it will need to go
> through the -tip tree.
>
> So with the changes,
>
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

Thanks for the review!

>
> Ingo, Boris, given that this depends on the previous patch, mind
> taking both via the -tip tree? I can take them too, but it doesn't
> make sense splitting them up.
>

-- 
Vitaly