| 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 >> >> >
On Mon, 28 Apr 2025 at 12:59, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > 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. > 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.
Ard Biesheuvel <ardb@kernel.org> writes: > On Thu, 24 Apr 2025 at 10:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: ... >> 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 > I'm sorry I'm probably missing something important but my understanding is that that header.S is compiled into setup.elf: ld -m elf_x86_64 -z noexecstack --no-warn-rwx-segments -m elf_i386 -z noexecstack -T arch/x86/boot/setup.ld ... arch/x86/boot/header.o ... -o arch/x86/boot/setup.elf and then the result gets concatenated with vmlinux.bin to get bzImage: objcopy -O binary arch/x86/boot/setup.elf arch/x86/boot/setup.bin cp arch/x86/boot/setup.bin arch/x86/boot/bzImage; truncate -s %4K arch/x86/boot/bzImage; cat arch/x86/boot/vmlinux.bin >>arch/x86/boot/bzImage so if we want to have SBAT at the very end of bzImage without dirty tricks it must be at the very end of vmlinux.bin, not setup.bin. I can, of course, use some existing compilation unit but to be honest I can't find anything suitable.
On Tue, 29 Apr 2025 at 11:55, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > Ard Biesheuvel <ardb@kernel.org> writes: > > > On Thu, 24 Apr 2025 at 10:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > ... > > >> 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 > > > > I'm sorry I'm probably missing something important but my understanding > is that that header.S is compiled into setup.elf: > > ld -m elf_x86_64 -z noexecstack --no-warn-rwx-segments -m elf_i386 -z > noexecstack -T arch/x86/boot/setup.ld ... arch/x86/boot/header.o ... -o arch/x86/boot/setup.elf > > and then the result gets concatenated with vmlinux.bin to get bzImage: > > objcopy -O binary arch/x86/boot/setup.elf arch/x86/boot/setup.bin > cp arch/x86/boot/setup.bin arch/x86/boot/bzImage; truncate -s %4K arch/x86/boot/bzImage; cat arch/x86/boot/vmlinux.bin >>arch/x86/boot/bzImage > > so if we want to have SBAT at the very end of bzImage without dirty > tricks it must be at the very end of vmlinux.bin, not setup.bin. I can, > of course, use some existing compilation unit but to be honest I can't > find anything suitable. > Yeah, you're right. I keep forgetting the insane way the bzImage is put together. So you'll need to incorporate $(CONFIG_EFI_SBAT_FILE) into arch/x86/boot/vmlinux. But that does not mean it needs to be constructed under drivers/firmware/efi/libstub, and it also doesn't mean you need filechk and a separate .o file, right?
Ard Biesheuvel <ardb@kernel.org> writes: > On Tue, 29 Apr 2025 at 11:55, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >> >> Ard Biesheuvel <ardb@kernel.org> writes: >> >> > On Thu, 24 Apr 2025 at 10:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >> >> ... >> >> >> 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 >> > >> >> I'm sorry I'm probably missing something important but my understanding >> is that that header.S is compiled into setup.elf: >> >> ld -m elf_x86_64 -z noexecstack --no-warn-rwx-segments -m elf_i386 -z >> noexecstack -T arch/x86/boot/setup.ld ... arch/x86/boot/header.o ... -o arch/x86/boot/setup.elf >> >> and then the result gets concatenated with vmlinux.bin to get bzImage: >> >> objcopy -O binary arch/x86/boot/setup.elf arch/x86/boot/setup.bin >> cp arch/x86/boot/setup.bin arch/x86/boot/bzImage; truncate -s %4K arch/x86/boot/bzImage; cat arch/x86/boot/vmlinux.bin >>arch/x86/boot/bzImage >> >> so if we want to have SBAT at the very end of bzImage without dirty >> tricks it must be at the very end of vmlinux.bin, not setup.bin. I can, >> of course, use some existing compilation unit but to be honest I can't >> find anything suitable. >> > > Yeah, you're right. I keep forgetting the insane way the bzImage is > put together. > > So you'll need to incorporate $(CONFIG_EFI_SBAT_FILE) into > arch/x86/boot/vmlinux. But that does not mean it needs to be > constructed under drivers/firmware/efi/libstub, and it also doesn't > mean you need filechk and a separate .o file, right? Right, it just needs to be somewhere and this somewhere needs to depend on the SBAT data to track its possible updates. E.g. looking at asm files in arch/x86/boot/compressed/ (which go to vmlinux) I see: arch/x86/boot/compressed/head_32.S arch/x86/boot/compressed/head_64.S arch/x86/boot/compressed/idt_handlers_64.S arch/x86/boot/compressed/kernel_info.S arch/x86/boot/compressed/la57toggle.S arch/x86/boot/compressed/mem_encrypt.S arch/x86/boot/compressed/piggy.S arch/x86/boot/compressed/tdcall.S and honestly I don't know which one to pick :-( An alternative would be to create separate 3-line sbat.S files for x86 and zboot and then make sbat.o dependent on CONFIG_EFI_SBAT_FILE but that would not satisfy all the requirements as sbat.o stays)
Ard Biesheuvel <ardb@kernel.org> writes: > On Mon, 28 Apr 2025 at 12:59, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >> >> Ard Biesheuvel <ardb@kernel.org> writes: >> >> > On Thu, 24 Apr 2025 at 10:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: ... >> >> $(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. Turns out it the problem is slightly harder to address then I initially thought. On x86, arch/x86/boot/bzImage is composed of setup.bin and vmlinux.bin and the later is produced from vmlinux with objcopy -O binary -R .note -R .comment -S arch/x86/boot/compressed/vmlinux arch/x86/boot/vmlinux.bin "objcopy -O binary" basically dumps memory representation of vmlinux. In case we put '.sbat' before '.bss' we get: Sections: Idx Name Size VMA LMA File off Algn ... 4 .data 00002000 0000000000aef000 0000000000aef000 00af0000 2**12 CONTENTS, ALLOC, LOAD, DATA 5 .sbat 00001000 0000000000af1000 0000000000af1000 00af2000 2**12 CONTENTS, ALLOC, LOAD, READONLY, DATA 6 .bss 00023078 0000000000af2000 0000000000af2000 00af3000 2**12 ALLOC 7 .pgtable 00021000 0000000000b16000 0000000000b16000 00af3000 2**12 ALLOC ... so we can't have a single '.data' section in PE covering both '.data' and '.bss'/'.pgtable' as '.sbat' is in the middle and we want it to be a separate section. If we try putting '.sbat' after '.bss' we are going to get a hole size of '.bss' + '.pgtable' in arch/x86/boot/vmlinux.bin because 'objcopy -O binary' is not going to squeeze anything (and it has no idea what '.sbat' is and if it can be moved). The problem is similar for zboot. I have two ideas: 1) Get back to the idea of putting '.sbat' between '.text' and '.data' (was in my RFC). 2) Introduce a separate '.bss' section to the PE binary, basically: diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S index b5c79f43359b..dae8202705c4 100644 --- a/arch/x86/boot/header.S +++ b/arch/x86/boot/header.S @@ -197,7 +197,7 @@ pecompat_fstart: IMAGE_SCN_MEM_EXECUTE # Characteristics .ascii ".data\0\0\0" - .long ZO__end - ZO__data # VirtualSize + .long ZO__edata - ZO__data # VirtualSize .long setup_size + ZO__data # VirtualAddress .long ZO__edata - ZO__data # SizeOfRawData .long setup_size + ZO__data # PointerToRawData @@ -207,6 +207,17 @@ pecompat_fstart: IMAGE_SCN_MEM_READ | \ IMAGE_SCN_MEM_WRITE # Characteristics + .ascii ".bss\0\0\0\0" + .long ZO__end - ZO__edata # VirtualSize + .long setup_size + ZO__edata # VirtualAddress + .long 0 # SizeOfRawData + .long 0 # PointerToRawData + + .long 0, 0, 0 + .long IMAGE_SCN_CNT_UNINITIALIZED_DATA| \ + IMAGE_SCN_MEM_READ | \ + IMAGE_SCN_MEM_WRITE # Characteristics + .set section_count, (. - section_table) / 40 #endif /* CONFIG_EFI_STUB */ This way '.data' doesn't need to be the last meaninful section in 'vmlinux' and we can then pub '.sbat' right after it. I'm going to give it a try but also I'm wondering why do we have '.data' covering '.bss' and '.pgtable' in the first place.
On Fri, 2 May 2025 at 14:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > Ard Biesheuvel <ardb@kernel.org> writes: > > > On Mon, 28 Apr 2025 at 12:59, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > >> > >> Ard Biesheuvel <ardb@kernel.org> writes: > >> > >> > On Thu, 24 Apr 2025 at 10:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > ... > > >> >> $(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. > > Turns out it the problem is slightly harder to address then I initially > thought. Yeah I was afraid this was going to be tricky. ... > The problem is similar for zboot. How so? > I have two ideas: > 1) Get back to the idea of putting '.sbat' between '.text' and '.data' > (was in my RFC). > This is what zboot does, no? > 2) Introduce a separate '.bss' section to the PE binary, basically: > I'd like .sbat to be as unintrusive as we can make it, so this is my least preferred option.
Ard Biesheuvel <ardb@kernel.org> writes: > On Fri, 2 May 2025 at 14:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >> >> Ard Biesheuvel <ardb@kernel.org> writes: >> >> > On Mon, 28 Apr 2025 at 12:59, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >> >> >> >> Ard Biesheuvel <ardb@kernel.org> writes: >> >> >> >> > On Thu, 24 Apr 2025 at 10:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >> >> ... >> >> >> >> $(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. >> >> Turns out it the problem is slightly harder to address then I initially >> thought. > > Yeah I was afraid this was going to be tricky. > > ... > >> The problem is similar for zboot. > > How so? > zboot-header.S has: .ascii ".data\0\0\0" .long __data_size .long _etext - .Ldoshdr .long __data_rawsize .long _etext - .Ldoshdr where the difference between '__data_rawsize' and '__data_size' is: PROVIDE(__data_rawsize = ABSOLUTE(_edata - _etext)); PROVIDE(__data_size = ABSOLUTE(_end - _etext)); and "_end" is the end of BSS. So if we put '.sbat' right after '.data' then '.data' will cover it too (so we will get an overlap). If we put if after '.bss' then we're going to get a hole (size of '.bss') upon (aarch64 example): objcopy -O binary arch/arm64/boot/vmlinuz.efi.elf arch/arm64/boot/vmlinuz.efi as AFAIU it won't be able to squeeze the binary, it only truncates the tail throwing away secions without content (in particular, '.sbat'). >> I have two ideas: >> 1) Get back to the idea of putting '.sbat' between '.text' and '.data' >> (was in my RFC). >> > > This is what zboot does, no? > In v1 I put '.sbat' right after '.data' and before '.bss' but I didn't think about the memory overlap problem. >> 2) Introduce a separate '.bss' section to the PE binary, basically: >> > > I'd like .sbat to be as unintrusive as we can make it, so this is my > least preferred option. > This is very reasonable -- unless for some reason we belive that separating '.bss' into its own PE section is a good idea on its own.
On Fri, 2 May 2025 at 15:46, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > Ard Biesheuvel <ardb@kernel.org> writes: > > > On Fri, 2 May 2025 at 14:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > >> > >> Ard Biesheuvel <ardb@kernel.org> writes: > >> > >> > On Mon, 28 Apr 2025 at 12:59, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > >> >> > >> >> Ard Biesheuvel <ardb@kernel.org> writes: > >> >> > >> >> > On Thu, 24 Apr 2025 at 10:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > >> > >> ... > >> > >> >> >> $(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. > >> > >> Turns out it the problem is slightly harder to address then I initially > >> thought. > > > > Yeah I was afraid this was going to be tricky. > > > > ... > > > >> The problem is similar for zboot. > > > > How so? > > > > zboot-header.S has: > > .ascii ".data\0\0\0" > .long __data_size > .long _etext - .Ldoshdr > .long __data_rawsize > .long _etext - .Ldoshdr > > where the difference between '__data_rawsize' and '__data_size' is: > > PROVIDE(__data_rawsize = ABSOLUTE(_edata - _etext)); > PROVIDE(__data_size = ABSOLUTE(_end - _etext)); > > and "_end" is the end of BSS. So if we put '.sbat' right after '.data' > then '.data' will cover it too (so we will get an overlap). If we put > if after '.bss' then we're going to get a hole (size of '.bss') upon > (aarch64 example): > > objcopy -O binary arch/arm64/boot/vmlinuz.efi.elf arch/arm64/boot/vmlinuz.efi > > as AFAIU it won't be able to squeeze the binary, it only truncates the > tail throwing away secions without content (in particular, '.sbat'). > Ah I misread your patch - I thought .sbat was between .text and .data, which arguably makes more sense.
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(-)