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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMj1kXHqmOiNX_DH+8uSsTROzR+hgvZ5DyE=3wVE7-dQ+2BW=Q@mail.gmail.com>
Date: Thu, 24 Apr 2025 18:37:39 +0200
From: Ard Biesheuvel <ardb@...nel.org>
To: Vitaly Kuznetsov <vkuznets@...hat.com>
Cc: x86@...nel.org, linux-efi@...r.kernel.org, 
	Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, "H. Peter Anvin" <hpa@...or.com>, 
	Peter Jones <pjones@...hat.com>, Daniel Berrange <berrange@...hat.com>, 
	Emanuele Giuseppe Esposito <eesposit@...hat.com>, Gerd Hoffmann <kraxel@...hat.com>, 
	Greg KH <gregkh@...uxfoundation.org>, Luca Boccassi <bluca@...ian.org>, 
	Peter Zijlstra <peterz@...radead.org>, Matthew Garrett <mjg59@...f.ucam.org>, 
	James Bottomley <James.Bottomley@...senpartnership.com>, 
	Eric Snowberg <eric.snowberg@...cle.com>, Paolo Bonzini <pbonzini@...hat.com>, 
	Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, 
	Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>, linux-riscv@...ts.infradead.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] efi/libstub: zboot specific mechanism for embedding
 SBAT section

Hi Vitaly,

On Thu, 24 Apr 2025 at 10:10, Vitaly Kuznetsov <vkuznets@...hat.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@...hat.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
>