Message-ID: <CAMj1kXEFXCjHXrAS2Xe_3xVvkkV6QYsuxcOmLEk-A2KNHUJ7ZA@mail.gmail.com>
Date: Mon, 28 Apr 2025 16:54:59 +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

On Mon, 28 Apr 2025 at 12:54, Vitaly Kuznetsov <vkuznets@...hat.com> wrote:
>
> Ard Biesheuvel <ardb@...nel.org> writes:
>
> > Hi Vitaly,
> >
>
> Ard, thanks for the review!
>
> > 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.
>
> 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.

Powered by blists - more mailing lists