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

Ard Biesheuvel
2025-04-25T06:03:23Z

On Thu, 24 Apr 2025 at 10:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
> Similar to zboot architectures, implement support for embedding SBAT data
> for x86. Put '.sbat' section to the very end of the binary.
>
> 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.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> arch/x86/boot/Makefile | 2 +-
> arch/x86/boot/compressed/Makefile | 2 ++
> arch/x86/boot/compressed/vmlinux.lds.S | 13 +++++++++++++
> arch/x86/boot/header.S | 13 +++++++++++++
> drivers/firmware/efi/Kconfig | 2 +-
> 5 files changed, 30 insertions(+), 2 deletions(-)
>
> 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..b9b80eccdc02 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -107,6 +107,8 @@ 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) += $(objtree)/drivers/firmware/efi/libstub/sbat.o
> +

Please drop this, and put the .incbin directly into header.S

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

> .set section_count, (. - section_table) / 40
> #endif /* CONFIG_EFI_STUB */
>
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index 2edb0167ba49..5022a378fec1 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -283,7 +283,7 @@ config EFI_EMBEDDED_FIRMWARE
>
> config EFI_SBAT
> bool "Embed SBAT section in the kernel"
> - 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