From f57fb375a203e28bf7c08ca01d5ee72028b391d9 Mon Sep 17 00:00:00 2001 From: Ard Biesheuvel Date: Mon, 17 Oct 2022 12:48:46 +0200 Subject: efi: libstub: Remove zboot signing from build options The zboot decompressor series introduced a feature to sign the PE/COFF kernel image for secure boot as part of the kernel build. This was necessary because there are actually two images that need to be signed: the kernel with the EFI stub attached, and the decompressor application. This is a bit of a burden, because it means that the images must be signed on the the same system that performs the build, and this is not realistic for distros. During the next cycle, we will introduce changes to the zboot code so that the inner image no longer needs to be signed. This means that the outer PE/COFF image can be handled as usual, and be signed later in the release process. Let's remove the associated Kconfig options now so that they don't end up in a LTS release while already being deprecated. Signed-off-by: Ard Biesheuvel --- drivers/firmware/efi/Kconfig | 22 ---------------------- drivers/firmware/efi/libstub/Makefile.zboot | 29 ++++------------------------- 2 files changed, 4 insertions(+), 47 deletions(-) diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig index 5b79a4a4a88d..6787ed8dfacf 100644 --- a/drivers/firmware/efi/Kconfig +++ b/drivers/firmware/efi/Kconfig @@ -124,28 +124,6 @@ config EFI_ZBOOT is supported by the encapsulated image. (The compression algorithm used is described in the zboot image header) -config EFI_ZBOOT_SIGNED - def_bool y - depends on EFI_ZBOOT_SIGNING_CERT != "" - depends on EFI_ZBOOT_SIGNING_KEY != "" - -config EFI_ZBOOT_SIGNING - bool "Sign the EFI decompressor for UEFI secure boot" - depends on EFI_ZBOOT - help - Use the 'sbsign' command line tool (which must exist on the host - path) to sign both the EFI decompressor PE/COFF image, as well as the - encapsulated PE/COFF image, which is subsequently compressed and - wrapped by the former image. - -config EFI_ZBOOT_SIGNING_CERT - string "Certificate to use for signing the compressed EFI boot image" - depends on EFI_ZBOOT_SIGNING - -config EFI_ZBOOT_SIGNING_KEY - string "Private key to use for signing the compressed EFI boot image" - depends on EFI_ZBOOT_SIGNING - config EFI_ARMSTUB_DTB_LOADER bool "Enable the DTB loader" depends on EFI_GENERIC_STUB && !RISCV && !LOONGARCH diff --git a/drivers/firmware/efi/libstub/Makefile.zboot b/drivers/firmware/efi/libstub/Makefile.zboot index 35f234ad8738..3340b385a05b 100644 --- a/drivers/firmware/efi/libstub/Makefile.zboot +++ b/drivers/firmware/efi/libstub/Makefile.zboot @@ -20,22 +20,11 @@ zboot-size-len-y := 4 zboot-method-$(CONFIG_KERNEL_GZIP) := gzip zboot-size-len-$(CONFIG_KERNEL_GZIP) := 0 -quiet_cmd_sbsign = SBSIGN $@ - cmd_sbsign = sbsign --out $@ $< \ - --key $(CONFIG_EFI_ZBOOT_SIGNING_KEY) \ - --cert $(CONFIG_EFI_ZBOOT_SIGNING_CERT) - -$(obj)/$(EFI_ZBOOT_PAYLOAD).signed: $(obj)/$(EFI_ZBOOT_PAYLOAD) FORCE - $(call if_changed,sbsign) - -ZBOOT_PAYLOAD-y := $(EFI_ZBOOT_PAYLOAD) -ZBOOT_PAYLOAD-$(CONFIG_EFI_ZBOOT_SIGNED) := $(EFI_ZBOOT_PAYLOAD).signed - -$(obj)/vmlinuz: $(obj)/$(ZBOOT_PAYLOAD-y) FORCE +$(obj)/vmlinuz: $(obj)/$(EFI_ZBOOT_PAYLOAD) FORCE $(call if_changed,$(zboot-method-y)) OBJCOPYFLAGS_vmlinuz.o := -I binary -O $(EFI_ZBOOT_BFD_TARGET) \ - --rename-section .data=.gzdata,load,alloc,readonly,contents + --rename-section .data=.gzdata,load,alloc,readonly,contents $(obj)/vmlinuz.o: $(obj)/vmlinuz FORCE $(call if_changed,objcopy) @@ -53,18 +42,8 @@ LDFLAGS_vmlinuz.efi.elf := -T $(srctree)/drivers/firmware/efi/libstub/zboot.lds $(obj)/vmlinuz.efi.elf: $(obj)/vmlinuz.o $(ZBOOT_DEPS) FORCE $(call if_changed,ld) -ZBOOT_EFI-y := vmlinuz.efi -ZBOOT_EFI-$(CONFIG_EFI_ZBOOT_SIGNED) := vmlinuz.efi.unsigned - -OBJCOPYFLAGS_$(ZBOOT_EFI-y) := -O binary -$(obj)/$(ZBOOT_EFI-y): $(obj)/vmlinuz.efi.elf FORCE +OBJCOPYFLAGS_vmlinuz.efi := -O binary +$(obj)/vmlinuz.efi: $(obj)/vmlinuz.efi.elf FORCE $(call if_changed,objcopy) targets += zboot-header.o vmlinuz vmlinuz.o vmlinuz.efi.elf vmlinuz.efi - -ifneq ($(CONFIG_EFI_ZBOOT_SIGNED),) -$(obj)/vmlinuz.efi: $(obj)/vmlinuz.efi.unsigned FORCE - $(call if_changed,sbsign) -endif - -targets += $(EFI_ZBOOT_PAYLOAD).signed vmlinuz.efi.unsigned -- cgit v1.2.3 From 4b017e59f01097f19b938f6dc4dc2c4720701610 Mon Sep 17 00:00:00 2001 From: Ard Biesheuvel Date: Fri, 14 Oct 2022 12:25:52 +0200 Subject: efi: ssdt: Don't free memory if ACPI table was loaded successfully MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Amadeusz reports KASAN use-after-free errors introduced by commit 3881ee0b1edc ("efi: avoid efivars layer when loading SSDTs from variables"). The problem appears to be that the memory that holds the new ACPI table is now freed unconditionally, instead of only when the ACPI core reported a failure to load the table. So let's fix this, by omitting the kfree() on success. Cc: # v6.0 Link: https://lore.kernel.org/all/a101a10a-4fbb-5fae-2e3c-76cf96ed8fbd@linux.intel.com/ Fixes: 3881ee0b1edc ("efi: avoid efivars layer when loading SSDTs from variables") Reported-by: Amadeusz Sławiński Signed-off-by: Ard Biesheuvel --- drivers/firmware/efi/efi.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index 9624735f1575..3ecdc43a3f2b 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -271,6 +271,8 @@ static __init int efivar_ssdt_load(void) acpi_status ret = acpi_load_table(data, NULL); if (ret) pr_err("failed to load table: %u\n", ret); + else + continue; } else { pr_err("failed to get var data: 0x%lx\n", status); } -- cgit v1.2.3 From 8a254d90a77580244ec57e82bca7eb65656cc167 Mon Sep 17 00:00:00 2001 From: Ard Biesheuvel Date: Wed, 19 Oct 2022 23:29:58 +0200 Subject: efi: efivars: Fix variable writes without query_variable_store() Commit bbc6d2c6ef22 ("efi: vars: Switch to new wrapper layer") refactored the efivars layer so that the 'business logic' related to which UEFI variables affect the boot flow in which way could be moved out of it, and into the efivarfs driver. This inadvertently broke setting variables on firmware implementations that lack the QueryVariableInfo() boot service, because we no longer tolerate a EFI_UNSUPPORTED result from check_var_size() when calling efivar_entry_set_get_size(), which now ends up calling check_var_size() a second time inadvertently. If QueryVariableInfo() is missing, we support writes of up to 64k - let's move that logic into check_var_size(), and drop the redundant call. Cc: # v6.0 Fixes: bbc6d2c6ef22 ("efi: vars: Switch to new wrapper layer") Signed-off-by: Ard Biesheuvel --- drivers/firmware/efi/vars.c | 10 +++++----- fs/efivarfs/vars.c | 16 ---------------- include/linux/efi.h | 3 --- 3 files changed, 5 insertions(+), 24 deletions(-) diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c index dd74d2ad3184..433b61587139 100644 --- a/drivers/firmware/efi/vars.c +++ b/drivers/firmware/efi/vars.c @@ -7,6 +7,7 @@ */ #include +#include #include #include #include @@ -20,19 +21,19 @@ static struct efivars *__efivars; static DEFINE_SEMAPHORE(efivars_lock); -efi_status_t check_var_size(u32 attributes, unsigned long size) +static efi_status_t check_var_size(u32 attributes, unsigned long size) { const struct efivar_operations *fops; fops = __efivars->ops; if (!fops->query_variable_store) - return EFI_UNSUPPORTED; + return (size <= SZ_64K) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES; return fops->query_variable_store(attributes, size, false); } -EXPORT_SYMBOL_NS_GPL(check_var_size, EFIVAR); +static efi_status_t check_var_size_nonblocking(u32 attributes, unsigned long size) { const struct efivar_operations *fops; @@ -40,11 +41,10 @@ efi_status_t check_var_size_nonblocking(u32 attributes, unsigned long size) fops = __efivars->ops; if (!fops->query_variable_store) - return EFI_UNSUPPORTED; + return (size <= SZ_64K) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES; return fops->query_variable_store(attributes, size, true); } -EXPORT_SYMBOL_NS_GPL(check_var_size_nonblocking, EFIVAR); /** * efivars_kobject - get the kobject for the registered efivars diff --git a/fs/efivarfs/vars.c b/fs/efivarfs/vars.c index a0ef63cfcecb..9e4f47808bd5 100644 --- a/fs/efivarfs/vars.c +++ b/fs/efivarfs/vars.c @@ -651,22 +651,6 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes, if (err) return err; - /* - * Ensure that the available space hasn't shrunk below the safe level - */ - status = check_var_size(attributes, *size + ucs2_strsize(name, 1024)); - if (status != EFI_SUCCESS) { - if (status != EFI_UNSUPPORTED) { - err = efi_status_to_err(status); - goto out; - } - - if (*size > 65536) { - err = -ENOSPC; - goto out; - } - } - status = efivar_set_variable_locked(name, vendor, attributes, *size, data, false); if (status != EFI_SUCCESS) { diff --git a/include/linux/efi.h b/include/linux/efi.h index da3974bf05d3..80f3c1c7827d 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -1085,9 +1085,6 @@ efi_status_t efivar_set_variable_locked(efi_char16_t *name, efi_guid_t *vendor, efi_status_t efivar_set_variable(efi_char16_t *name, efi_guid_t *vendor, u32 attr, unsigned long data_size, void *data); -efi_status_t check_var_size(u32 attributes, unsigned long size); -efi_status_t check_var_size_nonblocking(u32 attributes, unsigned long size); - #if IS_ENABLED(CONFIG_EFI_CAPSULE_LOADER) extern bool efi_capsule_pending(int *reset_type); -- cgit v1.2.3 From db14655ad7854b69a2efda348e30d02dbc19e8a1 Mon Sep 17 00:00:00 2001 From: Ard Biesheuvel Date: Fri, 14 Oct 2022 19:29:57 +0200 Subject: efi: libstub: Give efi_main() asmlinkage qualification To stop the bots from sending sparse warnings to me and the list about efi_main() not having a prototype, decorate it with asmlinkage so that it is clear that it is called from assembly, and therefore needs to remain external, even if it is never declared in a header file. Signed-off-by: Ard Biesheuvel --- drivers/firmware/efi/libstub/x86-stub.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c index b9ce6393e353..33a7811e12c6 100644 --- a/drivers/firmware/efi/libstub/x86-stub.c +++ b/drivers/firmware/efi/libstub/x86-stub.c @@ -765,9 +765,9 @@ static efi_status_t exit_boot(struct boot_params *boot_params, void *handle) * relocated by efi_relocate_kernel. * On failure, we exit to the firmware via efi_exit instead of returning. */ -unsigned long efi_main(efi_handle_t handle, - efi_system_table_t *sys_table_arg, - struct boot_params *boot_params) +asmlinkage unsigned long efi_main(efi_handle_t handle, + efi_system_table_t *sys_table_arg, + struct boot_params *boot_params) { unsigned long bzimage_addr = (unsigned long)startup_32; unsigned long buffer_start, buffer_end; -- cgit v1.2.3 From 53a7ea284de9eabc0e3b7dee54c2cb670b8e087a Mon Sep 17 00:00:00 2001 From: Ard Biesheuvel Date: Thu, 20 Oct 2022 11:26:42 +0200 Subject: efi: libstub: Fix incorrect payload size in zboot header The linker script symbol definition that captures the size of the compressed payload inside the zboot decompressor (which is exposed via the image header) refers to '.' for the end of the region, which does not give the correct result as the expression is not placed at the end of the payload. So use the symbol name explicitly. Signed-off-by: Ard Biesheuvel --- drivers/firmware/efi/libstub/zboot.lds | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/firmware/efi/libstub/zboot.lds b/drivers/firmware/efi/libstub/zboot.lds index 87a62765bafd..93d33f68333b 100644 --- a/drivers/firmware/efi/libstub/zboot.lds +++ b/drivers/firmware/efi/libstub/zboot.lds @@ -38,7 +38,8 @@ SECTIONS } } -PROVIDE(__efistub__gzdata_size = ABSOLUTE(. - __efistub__gzdata_start)); +PROVIDE(__efistub__gzdata_size = + ABSOLUTE(__efistub__gzdata_end - __efistub__gzdata_start)); PROVIDE(__data_rawsize = ABSOLUTE(_edata - _etext)); PROVIDE(__data_size = ABSOLUTE(_end - _etext)); -- cgit v1.2.3 From 37926f96302d8b6c2bc97990d33e316a3ed6d67f Mon Sep 17 00:00:00 2001 From: Ard Biesheuvel Date: Thu, 20 Oct 2022 15:16:09 +0200 Subject: efi: runtime: Don't assume virtual mappings are missing if VA == PA == 0 The generic EFI stub can be instructed to avoid SetVirtualAddressMap(), and simply run with the firmware's 1:1 mapping. In this case, it populates the virtual address fields of the runtime regions in the memory map with the physical address of each region, so that the mapping code has to be none the wiser. Only if SetVirtualAddressMap() fails, the virtual addresses are wiped and the kernel code knows that the regions cannot be mapped. However, wiping amounts to setting it to zero, and if a runtime region happens to live at physical address 0, its valid 1:1 mapped virtual address could be mistaken for a wiped field, resulting on loss of access to the EFI services at runtime. So let's only assume that VA == 0 means 'no runtime services' if the region in question does not live at PA 0x0. Signed-off-by: Ard Biesheuvel --- drivers/firmware/efi/arm-runtime.c | 2 +- drivers/firmware/efi/libstub/fdt.c | 8 ++++---- drivers/firmware/efi/riscv-runtime.c | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c index 3359ae2adf24..7c48c380d722 100644 --- a/drivers/firmware/efi/arm-runtime.c +++ b/drivers/firmware/efi/arm-runtime.c @@ -63,7 +63,7 @@ static bool __init efi_virtmap_init(void) if (!(md->attribute & EFI_MEMORY_RUNTIME)) continue; - if (md->virt_addr == 0) + if (md->virt_addr == U64_MAX) return false; ret = efi_create_mapping(&efi_mm, md); diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c index 4f4d98e51fbf..70e9789ff9de 100644 --- a/drivers/firmware/efi/libstub/fdt.c +++ b/drivers/firmware/efi/libstub/fdt.c @@ -313,16 +313,16 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle, /* * Set the virtual address field of all - * EFI_MEMORY_RUNTIME entries to 0. This will signal - * the incoming kernel that no virtual translation has - * been installed. + * EFI_MEMORY_RUNTIME entries to U64_MAX. This will + * signal the incoming kernel that no virtual + * translation has been installed. */ for (l = 0; l < priv.boot_memmap->map_size; l += priv.boot_memmap->desc_size) { p = (void *)priv.boot_memmap->map + l; if (p->attribute & EFI_MEMORY_RUNTIME) - p->virt_addr = 0; + p->virt_addr = U64_MAX; } } return EFI_SUCCESS; diff --git a/drivers/firmware/efi/riscv-runtime.c b/drivers/firmware/efi/riscv-runtime.c index d28e715d2bcc..d0daacd2c903 100644 --- a/drivers/firmware/efi/riscv-runtime.c +++ b/drivers/firmware/efi/riscv-runtime.c @@ -41,7 +41,7 @@ static bool __init efi_virtmap_init(void) if (!(md->attribute & EFI_MEMORY_RUNTIME)) continue; - if (md->virt_addr == 0) + if (md->virt_addr == U64_MAX) return false; ret = efi_create_mapping(&efi_mm, md); -- cgit v1.2.3