Message ID | 20240516090541.4164270-2-ardb+git@google.com |
---|---|
State | New |
Headers | show |
Series | x86/efistub: Omit physical KASLR when memory reservations exist | expand |
On Thu, 16 May 2024 at 19:29, Chaney, Ben <bchaney@akamai.com> wrote: > > > +static efi_status_t parse_options(const char *cmdline) > > +{ > > + static const char opts[][14] = { > > + "mem=", "memmap=", "efi_fake_mem=", "hugepages=" > > + }; > > + > > I think we probably want to include both crashkernel and pstore as arguments that can disable this randomization. > The existing code in arch/x86/boot/compressed/kaslr.c currently does not take these into account, as far as I can tell.
On Thu, May 16, 2024 at 05:29:11PM +0000, Chaney, Ben wrote: > > +static efi_status_t parse_options(const char *cmdline) > > +{ > > + static const char opts[][14] = { > > + "mem=", "memmap=", "efi_fake_mem=", "hugepages=" > > + }; > > + > > I think we probably want to include both crashkernel and pstore as arguments that can disable this randomization. The carve-outs that pstore uses should already appear in the physical memory mapping that EFI has. (i.e. those things get listed in e820 as non-RAM, etc) I don't know anything about crashkernel, but if we really do have a lot of these, we likely need to find a way to express them to EFI...
On Thu, May 16, 2024 at 11:05:42AM +0200, Ard Biesheuvel wrote: > From: Ard Biesheuvel <ardb@kernel.org> > > The legacy decompressor has elaborate logic to ensure that the > randomized physical placement of the decompressed kernel image does not > conflict with any memory reservations, including ones specified on the > command line using mem=, memmap=, efi_fake_mem= or hugepages=, which are > taken into account by the kernel proper at a later stage. > > When booting in EFI mode, it is the firmware's job to ensure that the > chosen range does not conflict with any memory reservations that it > knows about, and this is trivially achieved by using the firmware's > memory allocation APIs. > > That leaves reservations specified on the command line, though, which > the firmware knows nothing about, as these regions have no other special > significance to the platform. Since commit > > a1b87d54f4e4 ("x86/efistub: Avoid legacy decompressor when doing EFI boot") > > these reservations are not taken into account when randomizing the > physical placement, which may result in conflicts where the memory > cannot be reserved by the kernel proper because its own executable image > resides there. > > To avoid having to duplicate or reuse the existing complicated logic, > disable physical KASLR entirely when such overrides are specified. These > are mostly diagnostic tools or niche features, and physical KASLR (as > opposed to virtual KASLR, which is much more important as it affects the > memory addresses observed by code executing in the kernel) is something > we can live without. > > Closes: https://lkml.kernel.org/r/FA5F6719-8824-4B04-803E-82990E65E627%40akamai.com > Reported-by: Ben Chaney <bchaney@akamai.com> > Fixes: a1b87d54f4e4 ("x86/efistub: Avoid legacy decompressor when doing EFI boot") > Cc: <stable@vger.kernel.org> # v6.1+ > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Yup, all good by me: Reviewed-by: Kees Cook <keescook@chromium.org>
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c index d5a8182cf2e1..1983fd3bf392 100644 --- a/drivers/firmware/efi/libstub/x86-stub.c +++ b/drivers/firmware/efi/libstub/x86-stub.c @@ -776,6 +776,26 @@ static void error(char *str) efi_warn("Decompression failed: %s\n", str); } +static const char *cmdline_memmap_override; + +static efi_status_t parse_options(const char *cmdline) +{ + static const char opts[][14] = { + "mem=", "memmap=", "efi_fake_mem=", "hugepages=" + }; + + for (int i = 0; i < ARRAY_SIZE(opts); i++) { + const char *p = strstr(cmdline, opts[i]); + + if (p == cmdline || (p > cmdline && isspace(p[-1]))) { + cmdline_memmap_override = opts[i]; + break; + } + } + + return efi_parse_options(cmdline); +} + static efi_status_t efi_decompress_kernel(unsigned long *kernel_entry) { unsigned long virt_addr = LOAD_PHYSICAL_ADDR; @@ -807,6 +827,10 @@ static efi_status_t efi_decompress_kernel(unsigned long *kernel_entry) !memcmp(efistub_fw_vendor(), ami, sizeof(ami))) { efi_debug("AMI firmware v2.0 or older detected - disabling physical KASLR\n"); seed[0] = 0; + } else if (cmdline_memmap_override) { + efi_info("%s detected on the kernel command line - disabling physical KASLR\n", + cmdline_memmap_override); + seed[0] = 0; } boot_params_ptr->hdr.loadflags |= KASLR_FLAG; @@ -883,7 +907,7 @@ void __noreturn efi_stub_entry(efi_handle_t handle, } #ifdef CONFIG_CMDLINE_BOOL - status = efi_parse_options(CONFIG_CMDLINE); + status = parse_options(CONFIG_CMDLINE); if (status != EFI_SUCCESS) { efi_err("Failed to parse options\n"); goto fail; @@ -892,7 +916,7 @@ void __noreturn efi_stub_entry(efi_handle_t handle, if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) { unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr | ((u64)boot_params->ext_cmd_line_ptr << 32)); - status = efi_parse_options((char *)cmdline_paddr); + status = parse_options((char *)cmdline_paddr); if (status != EFI_SUCCESS) { efi_err("Failed to parse options\n"); goto fail;