| Message ID | 20250424080950.289864-3-vkuznets@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | efi: Add a mechanism for embedding SBAT section | expand |
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 > >
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 >> >> >
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
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(-)