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

Ard Biesheuvel
2025-04-28T17:31:09Z

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.