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

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: <CAMj1kXE0WcenLEB+60=+oc+aKfbqZkwYZf-TrOyDY=ShXQ2pYw@mail.gmail.com>
Date: Mon, 28 Apr 2025 17:16:10 +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 2/2] x86/efi: Implement support for embedding SBAT data
 for x86

On Mon, 28 Apr 2025 at 12:59, Vitaly Kuznetsov <vkuznets@...hat.com> wrote:
>
> Ard Biesheuvel <ardb@...nel.org> writes:
>
> > On Thu, 24 Apr 2025 at 10:10, Vitaly Kuznetsov <vkuznets@...hat.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@...hat.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.