[PATCH 0/2] efi: Add a mechanism for embedding SBAT section

[PATCH 0/2] efi: Add a mechanism for embedding SBAT section

[Vitaly Kuznetsov]

Changes since RFC:
(https://lore.kernel.org/linux-efi/20250305101744.1706803-1-vkuznets@redhat.com/)

- Implement SBAT embedding for zboot. (Smoke tested on aarch64 only,
 apologies if I missed some important differences on other arches!)
 It would've been possible to implement SBAT embedding for !zboot case 
 too I think but this looks like an unnecessary complication: 
 SecureBoot signing is likely to be done by distro vendors only and these
 will likely want zboot enabled anyway.
  
- x86: Thanks to Ard, CRC32 is now gone (see commit 9c54baab4401 ("x86/boot:
 Drop CRC-32 checksum and the build tool that generates it")) and thus
 SBAT can be placed to the very end of the binary, this simplifies things
 a bit.

SBAT is a mechanism which improves SecureBoot revocations of UEFI binaries
by introducing a generation-based technique. Compromised or vulnerable UEFI
binaries can be prevented from booting by bumping the minimal required
generation for the specific component in the bootloader. More information
on the SBAT can be obtained here:

https://github.com/rhboot/shim/blob/main/SBAT.md

Currently, shim checks .sbat data for itself in self-test and for second
stage bootloaders (grub, sd-boot, UKIs with sd-stub, ...) but kernel
revocations require cycling signing keys or adding kernel hashes to shim's
internal dbx. Adding .sbat to kernel and enforcing it on kernel loading
will allow to do the same tracking and revocation distros are already
doing with a simplified mechanism, and without having to keep lists of
kernels outside of the git repos.

Previously, an attempt was made to add ".sbat" section to the linux kernel:

https://lwn.net/Articles/938422/

The approach was rejected mainly because currently there's no policy on how
to update SBAT generation number when a new vulnerability is discovered. In
particular, it is unclear what to do with stable kernels which may or may
not backport certain patches making it impossible to describe the current
state with a simple number.

This series suggests a different approach: instead of defining SBAT
information, provide a mechanism for downstream kernel builders (distros)
to include their own SBAT data. This leaves the decision on the policy to
the distro vendors. Basically, each distro implementing SecureBoot today,
will have an option to inject their own SBAT data during kernel build and
before it gets signed by their SecureBoot CA. Different distro do not need
to agree on the common SBAT component names or generation numbers as each
distro ships its own 'shim' with their own 'vendor_cert'/'vendor_db'. Linux
upstream will never, ever need to care about the data unless they choose in
the future to participate in that way.

Vitaly Kuznetsov (2):
  efi/libstub: zboot specific mechanism for embedding SBAT section
  x86/efi: Implement support for embedding SBAT data for x86

 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                | 25 +++++++++++++++++++++
 drivers/firmware/efi/libstub/Makefile       |  7 ++++++
 drivers/firmware/efi/libstub/Makefile.zboot |  3 ++-
 drivers/firmware/efi/libstub/sbat.S         |  7 ++++++
 drivers/firmware/efi/libstub/zboot-header.S | 14 ++++++++++++
 drivers/firmware/efi/libstub/zboot.lds      | 17 ++++++++++++++
 10 files changed, 101 insertions(+), 2 deletions(-)
 create mode 100644 drivers/firmware/efi/libstub/sbat.S

-- 
2.49.0

[PATCH 1/2] efi/libstub: zboot specific mechanism for embedding SBAT section

[Vitaly Kuznetsov]

SBAT is a mechanism which improves SecureBoot revocations of UEFI binaries
by introducing a generation-based technique. Compromised or vulnerable UEFI
binaries can be prevented from booting by bumping the minimal required
generation for the specific component in the bootloader. More information
on the SBAT can be obtained here:

https://github.com/rhboot/shim/blob/main/SBAT.md

Upstream Linux kernel does not currently participate in any way in SBAT as
there's no existing policy in how SBAT generation number should be
defined. Keep the status quo and provide a mechanism for distro vendors and
anyone else who signs their kernel for SecureBoot to include their own SBAT
data. This leaves the decision on the policy to the vendor. Basically, each
distro implementing SecureBoot today, will have an option to inject their
own SBAT data during kernel build and before it gets signed by their
SecureBoot CA. Different distro do not need to agree on the common SBAT
component names or generation numbers as each distro ships its own 'shim'
with their own 'vendor_cert'/'vendor_db'

Implement support for embedding SBAT data for architectures using
zboot (arm64, loongarch, riscv). Build '.sbat' section along with libstub
so it can be reused by x86 implementation later.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/firmware/efi/Kconfig                | 25 +++++++++++++++++++++
 drivers/firmware/efi/libstub/Makefile       |  7 ++++++
 drivers/firmware/efi/libstub/Makefile.zboot |  3 ++-
 drivers/firmware/efi/libstub/sbat.S         |  7 ++++++
 drivers/firmware/efi/libstub/zboot-header.S | 14 ++++++++++++
 drivers/firmware/efi/libstub/zboot.lds      | 17 ++++++++++++++
 6 files changed, 72 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/efi/libstub/sbat.S

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 5fe61b9ab5f9..2edb0167ba49 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -281,6 +281,31 @@ config EFI_EMBEDDED_FIRMWARE
 	bool
 	select CRYPTO_LIB_SHA256
 
+config EFI_SBAT
+	bool "Embed SBAT section in the kernel"
+	depends on EFI_ZBOOT
+	help
+	  SBAT section provides a way to improve SecureBoot revocations of UEFI
+	  binaries by introducing a generation-based mechanism. With SBAT, older
+	  UEFI binaries can be prevented from booting by bumping the minimal
+	  required generation for the specific component in the bootloader.
+
+	  Note: SBAT information is distribution specific, i.e. the owner of the
+	  signing SecureBoot certificate must define the SBAT policy. Linux
+	  kernel upstream does not define SBAT components and their generations.
+
+	  See https://github.com/rhboot/shim/blob/main/SBAT.md for the additional
+	  details.
+
+	  If unsure, say N.
+
+config EFI_SBAT_FILE
+	string "Embedded SBAT section file path"
+	depends on EFI_SBAT
+	help
+	  Specify a file with SBAT data which is going to be embedded as '.sbat'
+	  section into the kernel.
+
 endmenu
 
 config UEFI_CPER
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index d23a1b9fed75..5113cbdadf9a 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -105,6 +105,13 @@ lib-$(CONFIG_UNACCEPTED_MEMORY) += unaccepted_memory.o bitmap.o find.o
 extra-y				:= $(lib-y)
 lib-y				:= $(patsubst %.o,%.stub.o,$(lib-y))
 
+extra-$(CONFIG_EFI_SBAT)	+= sbat.o
+$(obj)/sbat.o: $(obj)/sbat.bin
+targets += sbat.bin
+filechk_sbat.bin = cat $(or $(real-prereqs), /dev/null)
+$(obj)/sbat.bin: $(CONFIG_EFI_SBAT_FILE) FORCE
+	$(call filechk,sbat.bin)
+
 # Even when -mbranch-protection=none is set, Clang will generate a
 # .note.gnu.property for code-less object files (like lib/ctype.c),
 # so work around this by explicitly removing the unwanted section.
diff --git a/drivers/firmware/efi/libstub/Makefile.zboot b/drivers/firmware/efi/libstub/Makefile.zboot
index 48842b5c106b..3d2d0b326f7c 100644
--- a/drivers/firmware/efi/libstub/Makefile.zboot
+++ b/drivers/firmware/efi/libstub/Makefile.zboot
@@ -44,7 +44,8 @@ AFLAGS_zboot-header.o += -DMACHINE_TYPE=IMAGE_FILE_MACHINE_$(EFI_ZBOOT_MACH_TYPE
 $(obj)/zboot-header.o: $(srctree)/drivers/firmware/efi/libstub/zboot-header.S FORCE
 	$(call if_changed_rule,as_o_S)
 
-ZBOOT_DEPS := $(obj)/zboot-header.o $(objtree)/drivers/firmware/efi/libstub/lib.a
+ZBOOT_DEPS := $(obj)/zboot-header.o $(objtree)/drivers/firmware/efi/libstub/lib.a \
+	   $(if $(CONFIG_EFI_SBAT),$(objtree)/drivers/firmware/efi/libstub/sbat.o)
 
 LDFLAGS_vmlinuz.efi.elf := -T $(srctree)/drivers/firmware/efi/libstub/zboot.lds
 $(obj)/vmlinuz.efi.elf: $(obj)/vmlinuz.o $(ZBOOT_DEPS) FORCE
diff --git a/drivers/firmware/efi/libstub/sbat.S b/drivers/firmware/efi/libstub/sbat.S
new file mode 100644
index 000000000000..4e99a1bac794
--- /dev/null
+++ b/drivers/firmware/efi/libstub/sbat.S
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Embed SBAT data in the kernel.
+ */
+	.pushsection ".sbat","a",@progbits
+	.incbin "drivers/firmware/efi/libstub/sbat.bin"
+	.popsection
diff --git a/drivers/firmware/efi/libstub/zboot-header.S b/drivers/firmware/efi/libstub/zboot-header.S
index fb676ded47fa..f2df24504fc5 100644
--- a/drivers/firmware/efi/libstub/zboot-header.S
+++ b/drivers/firmware/efi/libstub/zboot-header.S
@@ -135,6 +135,20 @@ __efistub_efi_zboot_header:
 			IMAGE_SCN_MEM_READ | \
 			IMAGE_SCN_MEM_WRITE
 
+#ifdef CONFIG_EFI_SBAT
+	.ascii		".sbat\0\0\0"
+	.long		__sbat_size
+	.long		_edata - .Ldoshdr
+	.long		__sbat_size
+	.long		_edata - .Ldoshdr
+
+	.long		0, 0
+	.short		0, 0
+	.long		IMAGE_SCN_CNT_INITIALIZED_DATA | \
+			IMAGE_SCN_MEM_READ | \
+			IMAGE_SCN_MEM_DISCARDABLE
+#endif
+
 	.set		.Lsection_count, (. - .Lsection_table) / 40
 
 #ifdef PE_DLL_CHAR_EX
diff --git a/drivers/firmware/efi/libstub/zboot.lds b/drivers/firmware/efi/libstub/zboot.lds
index 9ecc57ff5b45..2cd5015c70ce 100644
--- a/drivers/firmware/efi/libstub/zboot.lds
+++ b/drivers/firmware/efi/libstub/zboot.lds
@@ -31,10 +31,24 @@ SECTIONS
 
 	.data : ALIGN(4096) {
 		*(.data* .init.data*)
+#ifndef CONFIG_EFI_SBAT
 		_edata = ALIGN(512);
+#else
+		/* Avoid gap between '.data' and '.sbat' */
+		_edata = ALIGN(4096);
+#endif
 		. = _edata;
 	}
 
+#ifdef CONFIG_EFI_SBAT
+        .sbat : ALIGN(4096) {
+		_sbat = . ;
+		*(.sbat)
+		_esbat = ALIGN(512);
+		. = _esbat;
+	}
+#endif
+
 	.bss : {
 		*(.bss* .init.bss*)
 		_end = ALIGN(512);
@@ -52,3 +66,6 @@ PROVIDE(__efistub__gzdata_size =
 
 PROVIDE(__data_rawsize = ABSOLUTE(_edata - _etext));
 PROVIDE(__data_size = ABSOLUTE(_end - _etext));
+#ifdef CONFIG_EFI_SBAT
+PROVIDE(__sbat_size = ABSOLUTE(_esbat - _sbat));
+#endif
-- 
2.49.0

Re: [PATCH 1/2] efi/libstub: zboot specific mechanism for embedding SBAT section

[Ard Biesheuvel]

Hi Vitaly,

On Thu, 24 Apr 2025 at 10:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
> SBAT is a mechanism which improves SecureBoot revocations of UEFI binaries
> by introducing a generation-based technique. Compromised or vulnerable UEFI
> binaries can be prevented from booting by bumping the minimal required
> generation for the specific component in the bootloader. More information
> on the SBAT can be obtained here:
>
> https://github.com/rhboot/shim/blob/main/SBAT.md
>
> Upstream Linux kernel does not currently participate in any way in SBAT as
> there's no existing policy in how SBAT generation number should be
> defined. Keep the status quo and provide a mechanism for distro vendors and
> anyone else who signs their kernel for SecureBoot to include their own SBAT
> data. This leaves the decision on the policy to the vendor. Basically, each
> distro implementing SecureBoot today, will have an option to inject their
> own SBAT data during kernel build and before it gets signed by their
> SecureBoot CA. Different distro do not need to agree on the common SBAT
> component names or generation numbers as each distro ships its own 'shim'
> with their own 'vendor_cert'/'vendor_db'
>
> Implement support for embedding SBAT data for architectures using
> zboot (arm64, loongarch, riscv). Build '.sbat' section along with libstub
> so it can be reused by x86 implementation later.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  drivers/firmware/efi/Kconfig                | 25 +++++++++++++++++++++
>  drivers/firmware/efi/libstub/Makefile       |  7 ++++++
>  drivers/firmware/efi/libstub/Makefile.zboot |  3 ++-
>  drivers/firmware/efi/libstub/sbat.S         |  7 ++++++
>  drivers/firmware/efi/libstub/zboot-header.S | 14 ++++++++++++
>  drivers/firmware/efi/libstub/zboot.lds      | 17 ++++++++++++++
>  6 files changed, 72 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/firmware/efi/libstub/sbat.S
>
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index 5fe61b9ab5f9..2edb0167ba49 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -281,6 +281,31 @@ config EFI_EMBEDDED_FIRMWARE
>         bool
>         select CRYPTO_LIB_SHA256
>
> +config EFI_SBAT
> +       bool "Embed SBAT section in the kernel"
> +       depends on EFI_ZBOOT
> +       help
> +         SBAT section provides a way to improve SecureBoot revocations of UEFI
> +         binaries by introducing a generation-based mechanism. With SBAT, older
> +         UEFI binaries can be prevented from booting by bumping the minimal
> +         required generation for the specific component in the bootloader.
> +
> +         Note: SBAT information is distribution specific, i.e. the owner of the
> +         signing SecureBoot certificate must define the SBAT policy. Linux
> +         kernel upstream does not define SBAT components and their generations.
> +
> +         See https://github.com/rhboot/shim/blob/main/SBAT.md for the additional
> +         details.
> +
> +         If unsure, say N.
> +
> +config EFI_SBAT_FILE
> +       string "Embedded SBAT section file path"
> +       depends on EFI_SBAT
> +       help
> +         Specify a file with SBAT data which is going to be embedded as '.sbat'
> +         section into the kernel.
> +

Can we simplify this? CONFIG_EFI_SBAT makes no sense if
CONFIG_EFI_SBAT_FILE is left empty. If you really need both symbols,
set EFI_SBAT automatically based on whether EFI_SBAT_FILE is
non-empty.

>  endmenu
>
>  config UEFI_CPER
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index d23a1b9fed75..5113cbdadf9a 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -105,6 +105,13 @@ lib-$(CONFIG_UNACCEPTED_MEMORY) += unaccepted_memory.o bitmap.o find.o
>  extra-y                                := $(lib-y)
>  lib-y                          := $(patsubst %.o,%.stub.o,$(lib-y))
>
> +extra-$(CONFIG_EFI_SBAT)       += sbat.o
> +$(obj)/sbat.o: $(obj)/sbat.bin
> +targets += sbat.bin
> +filechk_sbat.bin = cat $(or $(real-prereqs), /dev/null)
> +$(obj)/sbat.bin: $(CONFIG_EFI_SBAT_FILE) FORCE
> +       $(call filechk,sbat.bin)
> +

Please get rid of all of this, and move the .incbin into zboot-header.S


>  # Even when -mbranch-protection=none is set, Clang will generate a
>  # .note.gnu.property for code-less object files (like lib/ctype.c),
>  # so work around this by explicitly removing the unwanted section.
> diff --git a/drivers/firmware/efi/libstub/Makefile.zboot b/drivers/firmware/efi/libstub/Makefile.zboot
> index 48842b5c106b..3d2d0b326f7c 100644
> --- a/drivers/firmware/efi/libstub/Makefile.zboot
> +++ b/drivers/firmware/efi/libstub/Makefile.zboot
> @@ -44,7 +44,8 @@ AFLAGS_zboot-header.o += -DMACHINE_TYPE=IMAGE_FILE_MACHINE_$(EFI_ZBOOT_MACH_TYPE
>  $(obj)/zboot-header.o: $(srctree)/drivers/firmware/efi/libstub/zboot-header.S FORCE
>         $(call if_changed_rule,as_o_S)
>
> -ZBOOT_DEPS := $(obj)/zboot-header.o $(objtree)/drivers/firmware/efi/libstub/lib.a
> +ZBOOT_DEPS := $(obj)/zboot-header.o $(objtree)/drivers/firmware/efi/libstub/lib.a \
> +          $(if $(CONFIG_EFI_SBAT),$(objtree)/drivers/firmware/efi/libstub/sbat.o)
>

Drop this too

>  LDFLAGS_vmlinuz.efi.elf := -T $(srctree)/drivers/firmware/efi/libstub/zboot.lds
>  $(obj)/vmlinuz.efi.elf: $(obj)/vmlinuz.o $(ZBOOT_DEPS) FORCE
> diff --git a/drivers/firmware/efi/libstub/sbat.S b/drivers/firmware/efi/libstub/sbat.S
> new file mode 100644
> index 000000000000..4e99a1bac794
> --- /dev/null
> +++ b/drivers/firmware/efi/libstub/sbat.S
> @@ -0,0 +1,7 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Embed SBAT data in the kernel.
> + */
> +       .pushsection ".sbat","a",@progbits
> +       .incbin "drivers/firmware/efi/libstub/sbat.bin"
> +       .popsection
> diff --git a/drivers/firmware/efi/libstub/zboot-header.S b/drivers/firmware/efi/libstub/zboot-header.S
> index fb676ded47fa..f2df24504fc5 100644
> --- a/drivers/firmware/efi/libstub/zboot-header.S
> +++ b/drivers/firmware/efi/libstub/zboot-header.S
> @@ -135,6 +135,20 @@ __efistub_efi_zboot_header:
>                         IMAGE_SCN_MEM_READ | \
>                         IMAGE_SCN_MEM_WRITE
>
> +#ifdef CONFIG_EFI_SBAT
> +       .ascii          ".sbat\0\0\0"
> +       .long           __sbat_size
> +       .long           _edata - .Ldoshdr
> +       .long           __sbat_size
> +       .long           _edata - .Ldoshdr
> +
> +       .long           0, 0
> +       .short          0, 0
> +       .long           IMAGE_SCN_CNT_INITIALIZED_DATA | \
> +                       IMAGE_SCN_MEM_READ | \
> +                       IMAGE_SCN_MEM_DISCARDABLE

You can put the pushsection/popsection right here.

> +#endif
> +
>         .set            .Lsection_count, (. - .Lsection_table) / 40
>
>  #ifdef PE_DLL_CHAR_EX
> diff --git a/drivers/firmware/efi/libstub/zboot.lds b/drivers/firmware/efi/libstub/zboot.lds
> index 9ecc57ff5b45..2cd5015c70ce 100644
> --- a/drivers/firmware/efi/libstub/zboot.lds
> +++ b/drivers/firmware/efi/libstub/zboot.lds
> @@ -31,10 +31,24 @@ SECTIONS
>
>         .data : ALIGN(4096) {
>                 *(.data* .init.data*)
> +#ifndef CONFIG_EFI_SBAT
>                 _edata = ALIGN(512);
> +#else
> +               /* Avoid gap between '.data' and '.sbat' */
> +               _edata = ALIGN(4096);
> +#endif

Just use 4096 in all cases.

>                 . = _edata;
>         }
>
> +#ifdef CONFIG_EFI_SBAT
> +        .sbat : ALIGN(4096) {
> +               _sbat = . ;
> +               *(.sbat)
> +               _esbat = ALIGN(512);
> +               . = _esbat;
> +       }
> +#endif
> +
>         .bss : {
>                 *(.bss* .init.bss*)
>                 _end = ALIGN(512);
> @@ -52,3 +66,6 @@ PROVIDE(__efistub__gzdata_size =
>
>  PROVIDE(__data_rawsize = ABSOLUTE(_edata - _etext));
>  PROVIDE(__data_size = ABSOLUTE(_end - _etext));
> +#ifdef CONFIG_EFI_SBAT
> +PROVIDE(__sbat_size = ABSOLUTE(_esbat - _sbat));
> +#endif

This can be unconditional - it is only evaluated when a reference to it exists.

> --
> 2.49.0
>

Re: [PATCH 1/2] efi/libstub: zboot specific mechanism for embedding SBAT section

[Vitaly Kuznetsov]

Ard Biesheuvel <ardb@kernel.org> writes:

> Hi Vitaly,
>

Ard, thanks for the review!

> On Thu, 24 Apr 2025 at 10:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>>
>> SBAT is a mechanism which improves SecureBoot revocations of UEFI binaries
>> by introducing a generation-based technique. Compromised or vulnerable UEFI
>> binaries can be prevented from booting by bumping the minimal required
>> generation for the specific component in the bootloader. More information
>> on the SBAT can be obtained here:
>>
>> https://github.com/rhboot/shim/blob/main/SBAT.md
>>
>> Upstream Linux kernel does not currently participate in any way in SBAT as
>> there's no existing policy in how SBAT generation number should be
>> defined. Keep the status quo and provide a mechanism for distro vendors and
>> anyone else who signs their kernel for SecureBoot to include their own SBAT
>> data. This leaves the decision on the policy to the vendor. Basically, each
>> distro implementing SecureBoot today, will have an option to inject their
>> own SBAT data during kernel build and before it gets signed by their
>> SecureBoot CA. Different distro do not need to agree on the common SBAT
>> component names or generation numbers as each distro ships its own 'shim'
>> with their own 'vendor_cert'/'vendor_db'
>>
>> Implement support for embedding SBAT data for architectures using
>> zboot (arm64, loongarch, riscv). Build '.sbat' section along with libstub
>> so it can be reused by x86 implementation later.
>>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  drivers/firmware/efi/Kconfig                | 25 +++++++++++++++++++++
>>  drivers/firmware/efi/libstub/Makefile       |  7 ++++++
>>  drivers/firmware/efi/libstub/Makefile.zboot |  3 ++-
>>  drivers/firmware/efi/libstub/sbat.S         |  7 ++++++
>>  drivers/firmware/efi/libstub/zboot-header.S | 14 ++++++++++++
>>  drivers/firmware/efi/libstub/zboot.lds      | 17 ++++++++++++++
>>  6 files changed, 72 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/firmware/efi/libstub/sbat.S
>>
>> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
>> index 5fe61b9ab5f9..2edb0167ba49 100644
>> --- a/drivers/firmware/efi/Kconfig
>> +++ b/drivers/firmware/efi/Kconfig
>> @@ -281,6 +281,31 @@ config EFI_EMBEDDED_FIRMWARE
>>         bool
>>         select CRYPTO_LIB_SHA256
>>
>> +config EFI_SBAT
>> +       bool "Embed SBAT section in the kernel"
>> +       depends on EFI_ZBOOT
>> +       help
>> +         SBAT section provides a way to improve SecureBoot revocations of UEFI
>> +         binaries by introducing a generation-based mechanism. With SBAT, older
>> +         UEFI binaries can be prevented from booting by bumping the minimal
>> +         required generation for the specific component in the bootloader.
>> +
>> +         Note: SBAT information is distribution specific, i.e. the owner of the
>> +         signing SecureBoot certificate must define the SBAT policy. Linux
>> +         kernel upstream does not define SBAT components and their generations.
>> +
>> +         See https://github.com/rhboot/shim/blob/main/SBAT.md for the additional
>> +         details.
>> +
>> +         If unsure, say N.
>> +
>> +config EFI_SBAT_FILE
>> +       string "Embedded SBAT section file path"
>> +       depends on EFI_SBAT
>> +       help
>> +         Specify a file with SBAT data which is going to be embedded as '.sbat'
>> +         section into the kernel.
>> +
>
> Can we simplify this? CONFIG_EFI_SBAT makes no sense if
> CONFIG_EFI_SBAT_FILE is left empty. If you really need both symbols,
> set EFI_SBAT automatically based on whether EFI_SBAT_FILE is
> non-empty.

Sure, but FWIW, I modelled this after MODULE_SIG/MODULE_SIG_KEY and
BOOT_CONFIG_EMBED/BOOT_CONFIG_EMBED_FILE where the selection is also
2-step -- do you think EFI_SBAT/EFI_SBAT_FILE case is different?

>
>>  endmenu
>>
>>  config UEFI_CPER
>> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
>> index d23a1b9fed75..5113cbdadf9a 100644
>> --- a/drivers/firmware/efi/libstub/Makefile
>> +++ b/drivers/firmware/efi/libstub/Makefile
>> @@ -105,6 +105,13 @@ lib-$(CONFIG_UNACCEPTED_MEMORY) += unaccepted_memory.o bitmap.o find.o
>>  extra-y                                := $(lib-y)
>>  lib-y                          := $(patsubst %.o,%.stub.o,$(lib-y))
>>
>> +extra-$(CONFIG_EFI_SBAT)       += sbat.o
>> +$(obj)/sbat.o: $(obj)/sbat.bin
>> +targets += sbat.bin
>> +filechk_sbat.bin = cat $(or $(real-prereqs), /dev/null)
>> +$(obj)/sbat.bin: $(CONFIG_EFI_SBAT_FILE) FORCE
>> +       $(call filechk,sbat.bin)
>> +
>
> Please get rid of all of this, and move the .incbin into
> zboot-header.S

The main prupose of this logic is to track possible sbat data
changes. E.g. if the file with SBAT data has changed, then we need to
rebuild the kernel binary. If we just use a raw 'incbin' somewhere and
don't add a specific Makefile dependency, then the logic will be lost.

I think I can drop the dedicated 'sbat.S' and use zboot-header.S but I'd
like to keep at least the 'filechk' part: we compare what's in
EFI_SBAT_FILE with 'sbat.bin' copy and, if things have changed, rebuild.

>
>
>>  # Even when -mbranch-protection=none is set, Clang will generate a
>>  # .note.gnu.property for code-less object files (like lib/ctype.c),
>>  # so work around this by explicitly removing the unwanted section.
>> diff --git a/drivers/firmware/efi/libstub/Makefile.zboot b/drivers/firmware/efi/libstub/Makefile.zboot
>> index 48842b5c106b..3d2d0b326f7c 100644
>> --- a/drivers/firmware/efi/libstub/Makefile.zboot
>> +++ b/drivers/firmware/efi/libstub/Makefile.zboot
>> @@ -44,7 +44,8 @@ AFLAGS_zboot-header.o += -DMACHINE_TYPE=IMAGE_FILE_MACHINE_$(EFI_ZBOOT_MACH_TYPE
>>  $(obj)/zboot-header.o: $(srctree)/drivers/firmware/efi/libstub/zboot-header.S FORCE
>>         $(call if_changed_rule,as_o_S)
>>
>> -ZBOOT_DEPS := $(obj)/zboot-header.o $(objtree)/drivers/firmware/efi/libstub/lib.a
>> +ZBOOT_DEPS := $(obj)/zboot-header.o $(objtree)/drivers/firmware/efi/libstub/lib.a \
>> +          $(if $(CONFIG_EFI_SBAT),$(objtree)/drivers/firmware/efi/libstub/sbat.o)
>>
>
> Drop this too
>
>>  LDFLAGS_vmlinuz.efi.elf := -T $(srctree)/drivers/firmware/efi/libstub/zboot.lds
>>  $(obj)/vmlinuz.efi.elf: $(obj)/vmlinuz.o $(ZBOOT_DEPS) FORCE
>> diff --git a/drivers/firmware/efi/libstub/sbat.S b/drivers/firmware/efi/libstub/sbat.S
>> new file mode 100644
>> index 000000000000..4e99a1bac794
>> --- /dev/null
>> +++ b/drivers/firmware/efi/libstub/sbat.S
>> @@ -0,0 +1,7 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Embed SBAT data in the kernel.
>> + */
>> +       .pushsection ".sbat","a",@progbits
>> +       .incbin "drivers/firmware/efi/libstub/sbat.bin"
>> +       .popsection
>> diff --git a/drivers/firmware/efi/libstub/zboot-header.S b/drivers/firmware/efi/libstub/zboot-header.S
>> index fb676ded47fa..f2df24504fc5 100644
>> --- a/drivers/firmware/efi/libstub/zboot-header.S
>> +++ b/drivers/firmware/efi/libstub/zboot-header.S
>> @@ -135,6 +135,20 @@ __efistub_efi_zboot_header:
>>                         IMAGE_SCN_MEM_READ | \
>>                         IMAGE_SCN_MEM_WRITE
>>
>> +#ifdef CONFIG_EFI_SBAT
>> +       .ascii          ".sbat\0\0\0"
>> +       .long           __sbat_size
>> +       .long           _edata - .Ldoshdr
>> +       .long           __sbat_size
>> +       .long           _edata - .Ldoshdr
>> +
>> +       .long           0, 0
>> +       .short          0, 0
>> +       .long           IMAGE_SCN_CNT_INITIALIZED_DATA | \
>> +                       IMAGE_SCN_MEM_READ | \
>> +                       IMAGE_SCN_MEM_DISCARDABLE
>
> You can put the pushsection/popsection right here.
>

Ok (but see my comment about Makefile magic above.


>> +#endif
>> +
>>         .set            .Lsection_count, (. - .Lsection_table) / 40
>>
>>  #ifdef PE_DLL_CHAR_EX
>> diff --git a/drivers/firmware/efi/libstub/zboot.lds b/drivers/firmware/efi/libstub/zboot.lds
>> index 9ecc57ff5b45..2cd5015c70ce 100644
>> --- a/drivers/firmware/efi/libstub/zboot.lds
>> +++ b/drivers/firmware/efi/libstub/zboot.lds
>> @@ -31,10 +31,24 @@ SECTIONS
>>
>>         .data : ALIGN(4096) {
>>                 *(.data* .init.data*)
>> +#ifndef CONFIG_EFI_SBAT
>>                 _edata = ALIGN(512);
>> +#else
>> +               /* Avoid gap between '.data' and '.sbat' */
>> +               _edata = ALIGN(4096);
>> +#endif
>
> Just use 4096 in all cases.
>

Ok.

>>                 . = _edata;
>>         }
>>
>> +#ifdef CONFIG_EFI_SBAT
>> +        .sbat : ALIGN(4096) {
>> +               _sbat = . ;
>> +               *(.sbat)
>> +               _esbat = ALIGN(512);
>> +               . = _esbat;
>> +       }
>> +#endif
>> +
>>         .bss : {
>>                 *(.bss* .init.bss*)
>>                 _end = ALIGN(512);
>> @@ -52,3 +66,6 @@ PROVIDE(__efistub__gzdata_size =
>>
>>  PROVIDE(__data_rawsize = ABSOLUTE(_edata - _etext));
>>  PROVIDE(__data_size = ABSOLUTE(_end - _etext));
>> +#ifdef CONFIG_EFI_SBAT
>> +PROVIDE(__sbat_size = ABSOLUTE(_esbat - _sbat));
>> +#endif
>
> This can be unconditional - it is only evaluated when a reference to it exists.
>

Yes, it compiles well, I put #ifdef here mostly for documentational
purposes. Will drop.

>> --
>> 2.49.0
>>
>

-- 
Vitaly

Re: [PATCH 1/2] efi/libstub: zboot specific mechanism for embedding SBAT section

[Ard Biesheuvel]

On Mon, 28 Apr 2025 at 12:54, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
> Ard Biesheuvel <ardb@kernel.org> writes:
>
> > Hi Vitaly,
> >
>
> Ard, thanks for the review!
>
> > On Thu, 24 Apr 2025 at 10:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> >>
> >> SBAT is a mechanism which improves SecureBoot revocations of UEFI binaries
> >> by introducing a generation-based technique. Compromised or vulnerable UEFI
> >> binaries can be prevented from booting by bumping the minimal required
> >> generation for the specific component in the bootloader. More information
> >> on the SBAT can be obtained here:
> >>
> >> https://github.com/rhboot/shim/blob/main/SBAT.md
> >>
> >> Upstream Linux kernel does not currently participate in any way in SBAT as
> >> there's no existing policy in how SBAT generation number should be
> >> defined. Keep the status quo and provide a mechanism for distro vendors and
> >> anyone else who signs their kernel for SecureBoot to include their own SBAT
> >> data. This leaves the decision on the policy to the vendor. Basically, each
> >> distro implementing SecureBoot today, will have an option to inject their
> >> own SBAT data during kernel build and before it gets signed by their
> >> SecureBoot CA. Different distro do not need to agree on the common SBAT
> >> component names or generation numbers as each distro ships its own 'shim'
> >> with their own 'vendor_cert'/'vendor_db'
> >>
> >> Implement support for embedding SBAT data for architectures using
> >> zboot (arm64, loongarch, riscv). Build '.sbat' section along with libstub
> >> so it can be reused by x86 implementation later.
> >>
> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >> ---
> >>  drivers/firmware/efi/Kconfig                | 25 +++++++++++++++++++++
> >>  drivers/firmware/efi/libstub/Makefile       |  7 ++++++
> >>  drivers/firmware/efi/libstub/Makefile.zboot |  3 ++-
> >>  drivers/firmware/efi/libstub/sbat.S         |  7 ++++++
> >>  drivers/firmware/efi/libstub/zboot-header.S | 14 ++++++++++++
> >>  drivers/firmware/efi/libstub/zboot.lds      | 17 ++++++++++++++
> >>  6 files changed, 72 insertions(+), 1 deletion(-)
> >>  create mode 100644 drivers/firmware/efi/libstub/sbat.S
> >>
> >> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> >> index 5fe61b9ab5f9..2edb0167ba49 100644
> >> --- a/drivers/firmware/efi/Kconfig
> >> +++ b/drivers/firmware/efi/Kconfig
> >> @@ -281,6 +281,31 @@ config EFI_EMBEDDED_FIRMWARE
> >>         bool
> >>         select CRYPTO_LIB_SHA256
> >>
> >> +config EFI_SBAT
> >> +       bool "Embed SBAT section in the kernel"
> >> +       depends on EFI_ZBOOT
> >> +       help
> >> +         SBAT section provides a way to improve SecureBoot revocations of UEFI
> >> +         binaries by introducing a generation-based mechanism. With SBAT, older
> >> +         UEFI binaries can be prevented from booting by bumping the minimal
> >> +         required generation for the specific component in the bootloader.
> >> +
> >> +         Note: SBAT information is distribution specific, i.e. the owner of the
> >> +         signing SecureBoot certificate must define the SBAT policy. Linux
> >> +         kernel upstream does not define SBAT components and their generations.
> >> +
> >> +         See https://github.com/rhboot/shim/blob/main/SBAT.md for the additional
> >> +         details.
> >> +
> >> +         If unsure, say N.
> >> +
> >> +config EFI_SBAT_FILE
> >> +       string "Embedded SBAT section file path"
> >> +       depends on EFI_SBAT
> >> +       help
> >> +         Specify a file with SBAT data which is going to be embedded as '.sbat'
> >> +         section into the kernel.
> >> +
> >
> > Can we simplify this? CONFIG_EFI_SBAT makes no sense if
> > CONFIG_EFI_SBAT_FILE is left empty. If you really need both symbols,
> > set EFI_SBAT automatically based on whether EFI_SBAT_FILE is
> > non-empty.
>
> Sure, but FWIW, I modelled this after MODULE_SIG/MODULE_SIG_KEY and
> BOOT_CONFIG_EMBED/BOOT_CONFIG_EMBED_FILE where the selection is also
> 2-step -- do you think EFI_SBAT/EFI_SBAT_FILE case is different?
>

Regardless of the other cases,it at the very least should be a
config-time error for EFI_SBAT to be y when EFI_SBAT_FILE is empty. We
shouldn't have to deal with CONFIG_ options being in an inconsistent
state, that's Kconfig's job.

> >
> >>  endmenu
> >>
> >>  config UEFI_CPER
> >> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> >> index d23a1b9fed75..5113cbdadf9a 100644
> >> --- a/drivers/firmware/efi/libstub/Makefile
> >> +++ b/drivers/firmware/efi/libstub/Makefile
> >> @@ -105,6 +105,13 @@ lib-$(CONFIG_UNACCEPTED_MEMORY) += unaccepted_memory.o bitmap.o find.o
> >>  extra-y                                := $(lib-y)
> >>  lib-y                          := $(patsubst %.o,%.stub.o,$(lib-y))
> >>
> >> +extra-$(CONFIG_EFI_SBAT)       += sbat.o
> >> +$(obj)/sbat.o: $(obj)/sbat.bin
> >> +targets += sbat.bin
> >> +filechk_sbat.bin = cat $(or $(real-prereqs), /dev/null)
> >> +$(obj)/sbat.bin: $(CONFIG_EFI_SBAT_FILE) FORCE
> >> +       $(call filechk,sbat.bin)
> >> +
> >
> > Please get rid of all of this, and move the .incbin into
> > zboot-header.S
>
> The main prupose of this logic is to track possible sbat data
> changes. E.g. if the file with SBAT data has changed, then we need to
> rebuild the kernel binary. If we just use a raw 'incbin' somewhere and
> don't add a specific Makefile dependency, then the logic will be lost.
>

So why isn't it sufficient to make zboot-header.o depend on
$(CONFIG_EFI_SBAT_FILE)?

> I think I can drop the dedicated 'sbat.S' and use zboot-header.S but I'd
> like to keep at least the 'filechk' part: we compare what's in
> EFI_SBAT_FILE with 'sbat.bin' copy and, if things have changed, rebuild.
>

The filechk is just a copy, and the /dev/null hack should be removed
too. So please find another way to track the dependency, and drop all
of this.

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

[Vitaly Kuznetsov]

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
+
 $(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 = . ;
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
+
 	.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

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

[Ard Biesheuvel]

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
>
>

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

[Vitaly Kuznetsov]

Ard Biesheuvel <ardb@kernel.org> writes:

> 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
>

Sure, but as I also commented on zboot patch, we need a logic to track
possible sbat data changes and rebuild when needed. 'sbat.o' was
convenient because we can have this tracking logic in one place (zboot)
and make will do the rest. If we are to drop 'sbat.o', we will need the
tracking logic both in zboot and x86.

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

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

-- 
Vitaly

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

[Ard Biesheuvel]

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:
> >>
> >> 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
> >
>
> Sure, but as I also commented on zboot patch, we need a logic to track
> possible sbat data changes and rebuild when needed. 'sbat.o' was
> convenient because we can have this tracking logic in one place (zboot)
> and make will do the rest. If we are to drop 'sbat.o', we will need the
> tracking logic both in zboot and x86.
>

Same question: why isn't if sufficient to add

ifneq ($(CONFIG_EFI_SBAT_FILE),)
$(obj)/header.o: $(CONFIG_EFI_SBAT_FILE)
endif

to arch/x86/boot/Makefile?


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

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

[Vitaly Kuznetsov]

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

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@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?

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

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

[Vitaly Kuznetsov]

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').

>> 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?
>

In v1 I put '.sbat' right after '.data' and before '.bss' but I didn't
think about the memory overlap problem.

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

This is very reasonable -- unless for some reason we belive that
separating '.bss' into its own PE section is a good idea on its own.

-- 
Vitaly

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

[Ard Biesheuvel]

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.

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

[Vitaly Kuznetsov]

Ard Biesheuvel <ardb@kernel.org> writes:

> On Thu, 24 Apr 2025 at 10:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

...

>> 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
>

I'm sorry I'm probably missing something important but my understanding
is that that header.S is compiled into setup.elf:

 ld -m elf_x86_64 -z noexecstack --no-warn-rwx-segments  -m elf_i386 -z
 noexecstack -T arch/x86/boot/setup.ld  ... arch/x86/boot/header.o ...  -o arch/x86/boot/setup.elf

and then the result gets concatenated with vmlinux.bin to get bzImage:

 objcopy  -O binary arch/x86/boot/setup.elf arch/x86/boot/setup.bin
 cp arch/x86/boot/setup.bin arch/x86/boot/bzImage; truncate -s %4K arch/x86/boot/bzImage; cat arch/x86/boot/vmlinux.bin >>arch/x86/boot/bzImage

so if we want to have SBAT at the very end of bzImage without dirty
tricks it must be at the very end of vmlinux.bin, not setup.bin. I can,
of course, use some existing compilation unit but to be honest I can't
find anything suitable.

-- 
Vitaly

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

[Ard Biesheuvel]

On Tue, 29 Apr 2025 at 11:55, 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:
>
> ...
>
> >> 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
> >
>
> I'm sorry I'm probably missing something important but my understanding
> is that that header.S is compiled into setup.elf:
>
>  ld -m elf_x86_64 -z noexecstack --no-warn-rwx-segments  -m elf_i386 -z
>  noexecstack -T arch/x86/boot/setup.ld  ... arch/x86/boot/header.o ...  -o arch/x86/boot/setup.elf
>
> and then the result gets concatenated with vmlinux.bin to get bzImage:
>
>  objcopy  -O binary arch/x86/boot/setup.elf arch/x86/boot/setup.bin
>  cp arch/x86/boot/setup.bin arch/x86/boot/bzImage; truncate -s %4K arch/x86/boot/bzImage; cat arch/x86/boot/vmlinux.bin >>arch/x86/boot/bzImage
>
> so if we want to have SBAT at the very end of bzImage without dirty
> tricks it must be at the very end of vmlinux.bin, not setup.bin. I can,
> of course, use some existing compilation unit but to be honest I can't
> find anything suitable.
>

Yeah, you're right. I keep forgetting the insane way the bzImage is
put together.

So you'll need to incorporate $(CONFIG_EFI_SBAT_FILE) into
arch/x86/boot/vmlinux. But that does not mean it needs to be
constructed under drivers/firmware/efi/libstub, and it also doesn't
mean you need filechk and a separate .o file, right?

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

[Vitaly Kuznetsov]

Ard Biesheuvel <ardb@kernel.org> writes:

> On Tue, 29 Apr 2025 at 11:55, 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:
>>
>> ...
>>
>> >> 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
>> >
>>
>> I'm sorry I'm probably missing something important but my understanding
>> is that that header.S is compiled into setup.elf:
>>
>>  ld -m elf_x86_64 -z noexecstack --no-warn-rwx-segments  -m elf_i386 -z
>>  noexecstack -T arch/x86/boot/setup.ld  ... arch/x86/boot/header.o ...  -o arch/x86/boot/setup.elf
>>
>> and then the result gets concatenated with vmlinux.bin to get bzImage:
>>
>>  objcopy  -O binary arch/x86/boot/setup.elf arch/x86/boot/setup.bin
>>  cp arch/x86/boot/setup.bin arch/x86/boot/bzImage; truncate -s %4K arch/x86/boot/bzImage; cat arch/x86/boot/vmlinux.bin >>arch/x86/boot/bzImage
>>
>> so if we want to have SBAT at the very end of bzImage without dirty
>> tricks it must be at the very end of vmlinux.bin, not setup.bin. I can,
>> of course, use some existing compilation unit but to be honest I can't
>> find anything suitable.
>>
>
> Yeah, you're right. I keep forgetting the insane way the bzImage is
> put together.
>
> So you'll need to incorporate $(CONFIG_EFI_SBAT_FILE) into
> arch/x86/boot/vmlinux. But that does not mean it needs to be
> constructed under drivers/firmware/efi/libstub, and it also doesn't
> mean you need filechk and a separate .o file, right?

Right, it just needs to be somewhere and this somewhere needs to depend
on the SBAT data to track its possible updates. E.g. looking at asm
files in arch/x86/boot/compressed/ (which go to vmlinux) I see:

arch/x86/boot/compressed/head_32.S
arch/x86/boot/compressed/head_64.S
arch/x86/boot/compressed/idt_handlers_64.S
arch/x86/boot/compressed/kernel_info.S
arch/x86/boot/compressed/la57toggle.S
arch/x86/boot/compressed/mem_encrypt.S
arch/x86/boot/compressed/piggy.S
arch/x86/boot/compressed/tdcall.S

and honestly I don't know which one to pick :-( An alternative would be
to create separate 3-line sbat.S files for x86 and zboot and then make
sbat.o dependent on CONFIG_EFI_SBAT_FILE but that would not satisfy all
the requirements as sbat.o stays)

-- 
Vitaly