Re: [PATCH v4 3/5] arm64: libstub: Move KASLR handling functions to efi-stub-helper.c

From: Alexandre Ghiti
Date: Thu Jul 13 2023 - 11:07:02 EST

Next message: Greg KH: "Re: [RFC PATCH v1 0/3] docs: stable-kernel-rules: add delayed backporting option and a few tweaks"
Previous message: Ryan Roberts: "Re: [PATCH v1 9/9] selftests/mm: Run all tests from run_vmtests.sh"
In reply to: Ard Biesheuvel: "Re: [PATCH v4 3/5] arm64: libstub: Move KASLR handling functions to efi-stub-helper.c"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Ard,

On Thu, Jul 13, 2023 at 4:30 PM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
>
> Hello Alexandre,
>
> On Thu, 13 Jul 2023 at 15:37, Alexandre Ghiti <alexghiti@xxxxxxxxxxxx> wrote:
> >
> > This prepares for riscv to use the same functions to handle the pĥysical
> > kernel move when KASLR is enabled.
> >
> > Signed-off-by: Alexandre Ghiti <alexghiti@xxxxxxxxxxxx>
> > ---
> > arch/arm64/include/asm/efi.h | 4 +
> > arch/riscv/include/asm/efi.h | 1 +
> > drivers/firmware/efi/libstub/Makefile | 3 +-
> > drivers/firmware/efi/libstub/arm64-stub.c | 117 ++-----------
> > drivers/firmware/efi/libstub/efi-stub-kaslr.c | 159 ++++++++++++++++++
>
> Please just call the file kaslr.c

Ok

>
> > drivers/firmware/efi/libstub/efistub.h | 18 ++
> > 6 files changed, 197 insertions(+), 105 deletions(-)
> > create mode 100644 drivers/firmware/efi/libstub/efi-stub-kaslr.c
> >
> > diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> > index 4cf2cb053bc8..83a4d69fd968 100644
> > --- a/arch/arm64/include/asm/efi.h
> > +++ b/arch/arm64/include/asm/efi.h
> > @@ -111,6 +111,7 @@ static inline unsigned long efi_get_kimg_min_align(void)
> > */
> > return efi_nokaslr ? MIN_KIMG_ALIGN : EFI_KIMG_ALIGN;
> > }
> > +#define efi_get_kimg_min_align efi_get_kimg_min_align
> >
> > #define EFI_ALLOC_ALIGN SZ_64K
> > #define EFI_ALLOC_LIMIT ((1UL << 48) - 1)
> > @@ -168,4 +169,7 @@ static inline void efi_capsule_flush_cache_range(void *addr, int size)
> >
> > efi_status_t efi_handle_corrupted_x18(efi_status_t s, const char *f);
> >
> > +void efi_icache_sync(unsigned long start, unsigned long end);
> > +#define efi_icache_sync efi_icache_sync
> > +
>
> Given that arm64 is the only arch that needs to implement this, can we
> just keep the call in arch code? I.e., after
> efi_kaslr_relocate_kernel() if it returned with EFI_SUCCESS?

Actually, I implement this function for riscv in patch 5 as we need to
sync the icache and the dcache too. But my flaky internet did not
survive long enough.

>
>
> > #endif /* _ASM_EFI_H */
> > diff --git a/arch/riscv/include/asm/efi.h b/arch/riscv/include/asm/efi.h
> > index 29e9a0d84b16..c3dafaab36a2 100644
> > --- a/arch/riscv/include/asm/efi.h
> > +++ b/arch/riscv/include/asm/efi.h
> > @@ -43,6 +43,7 @@ static inline unsigned long efi_get_kimg_min_align(void)
> > */
> > return IS_ENABLED(CONFIG_64BIT) ? SZ_2M : SZ_4M;
> > }
> > +#define efi_get_kimg_min_align efi_get_kimg_min_align

efi_get_kimg_min_align() is implemented here for riscv ^

> >
> > #define EFI_KIMG_PREFERRED_ADDRESS efi_get_kimg_min_align()
> >
> > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > index 16d64a34d1e1..5927fab5c834 100644
> > --- a/drivers/firmware/efi/libstub/Makefile
> > +++ b/drivers/firmware/efi/libstub/Makefile
> > @@ -67,7 +67,8 @@ OBJECT_FILES_NON_STANDARD := y
> > # Prevents link failures: __sanitizer_cov_trace_pc() is not linked in.
> > KCOV_INSTRUMENT := n
> >
> > -lib-y := efi-stub-helper.o gop.o secureboot.o tpm.o \
> > +lib-y := efi-stub-helper.o efi-stub-kaslr.o \
> > + gop.o secureboot.o tpm.o \
> > file.o mem.o random.o randomalloc.o pci.o \
> > skip_spaces.o lib-cmdline.o lib-ctype.o \
> > alignedmem.o relocate.o printk.o vsprintf.o
> > diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
> > index 770b8ecb7398..452b7ccd330e 100644
> > --- a/drivers/firmware/efi/libstub/arm64-stub.c
> > +++ b/drivers/firmware/efi/libstub/arm64-stub.c
> > @@ -14,42 +14,6 @@
> >
> > #include "efistub.h"
> >
> > -/*
> > - * Distro versions of GRUB may ignore the BSS allocation entirely (i.e., fail
> > - * to provide space, and fail to zero it). Check for this condition by double
> > - * checking that the first and the last byte of the image are covered by the
> > - * same EFI memory map entry.
> > - */
> > -static bool check_image_region(u64 base, u64 size)
> > -{
> > - struct efi_boot_memmap *map;
> > - efi_status_t status;
> > - bool ret = false;
> > - int map_offset;
> > -
> > - status = efi_get_memory_map(&map, false);
> > - if (status != EFI_SUCCESS)
> > - return false;
> > -
> > - for (map_offset = 0; map_offset < map->map_size; map_offset += map->desc_size) {
> > - efi_memory_desc_t *md = (void *)map->map + map_offset;
> > - u64 end = md->phys_addr + md->num_pages * EFI_PAGE_SIZE;
> > -
> > - /*
> > - * Find the region that covers base, and return whether
> > - * it covers base+size bytes.
> > - */
> > - if (base >= md->phys_addr && base < end) {
> > - ret = (base + size) <= end;
> > - break;
> > - }
> > - }
> > -
> > - efi_bs_call(free_pool, map);
> > -
> > - return ret;
> > -}
> > -
> > efi_status_t handle_kernel_image(unsigned long *image_addr,
> > unsigned long *image_size,
> > unsigned long *reserve_addr,
> > @@ -59,31 +23,6 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
> > {
> > efi_status_t status;
> > unsigned long kernel_size, kernel_codesize, kernel_memsize;
> > - u32 phys_seed = 0;
> > - u64 min_kimg_align = efi_get_kimg_min_align();
> > -
> > - if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {
> > - efi_guid_t li_fixed_proto = LINUX_EFI_LOADED_IMAGE_FIXED_GUID;
> > - void *p;
> > -
> > - if (efi_nokaslr) {
> > - efi_info("KASLR disabled on kernel command line\n");
> > - } else if (efi_bs_call(handle_protocol, image_handle,
> > - &li_fixed_proto, &p) == EFI_SUCCESS) {
> > - efi_info("Image placement fixed by loader\n");
> > - } else {
> > - status = efi_get_random_bytes(sizeof(phys_seed),
> > - (u8 *)&phys_seed);
> > - if (status == EFI_NOT_FOUND) {
> > - efi_info("EFI_RNG_PROTOCOL unavailable\n");
> > - efi_nokaslr = true;
> > - } else if (status != EFI_SUCCESS) {
> > - efi_err("efi_get_random_bytes() failed (0x%lx)\n",
> > - status);
> > - efi_nokaslr = true;
> > - }
> > - }
> > - }
> >
> > if (image->image_base != _text) {
> > efi_err("FIRMWARE BUG: efi_loaded_image_t::image_base has bogus value\n");
> > @@ -98,50 +37,15 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
> > kernel_codesize = __inittext_end - _text;
> > kernel_memsize = kernel_size + (_end - _edata);
> > *reserve_size = kernel_memsize;
> > + *image_addr = (unsigned long)_text;
> >
> > - if (IS_ENABLED(CONFIG_RANDOMIZE_BASE) && phys_seed != 0) {
> > - /*
> > - * If KASLR is enabled, and we have some randomness available,
> > - * locate the kernel at a randomized offset in physical memory.
> > - */
> > - status = efi_random_alloc(*reserve_size, min_kimg_align,
> > - reserve_addr, phys_seed,
> > - EFI_LOADER_CODE);
> > - if (status != EFI_SUCCESS)
> > - efi_warn("efi_random_alloc() failed: 0x%lx\n", status);
> > - } else {
> > - status = EFI_OUT_OF_RESOURCES;
> > - }
> > -
> > - if (status != EFI_SUCCESS) {
> > - if (!check_image_region((u64)_text, kernel_memsize)) {
> > - efi_err("FIRMWARE BUG: Image BSS overlaps adjacent EFI memory region\n");
> > - } else if (IS_ALIGNED((u64)_text, min_kimg_align) &&
> > - (u64)_end < EFI_ALLOC_LIMIT) {
> > - /*
> > - * Just execute from wherever we were loaded by the
> > - * UEFI PE/COFF loader if the placement is suitable.
> > - */
> > - *image_addr = (u64)_text;
> > - *reserve_size = 0;
> > - return EFI_SUCCESS;
> > - }
> > -
> > - status = efi_allocate_pages_aligned(*reserve_size, reserve_addr,
> > - ULONG_MAX, min_kimg_align,
> > - EFI_LOADER_CODE);
> > -
> > - if (status != EFI_SUCCESS) {
> > - efi_err("Failed to relocate kernel\n");
> > - *reserve_size = 0;
> > - return status;
> > - }
> > - }
> > -
> > - *image_addr = *reserve_addr;
> > - memcpy((void *)*image_addr, _text, kernel_size);
> > - caches_clean_inval_pou(*image_addr, *image_addr + kernel_codesize);
> > - efi_remap_image(*image_addr, *reserve_size, kernel_codesize);
> > + status = efi_kaslr_relocate_kernel(image_addr,
> > + reserve_addr, reserve_size,
> > + kernel_size, kernel_codesize,
> > + kernel_memsize,
> > + efi_kaslr_get_phys_seed(image_handle));
> > + if (status != EFI_SUCCESS)
> > + return status;
> >
> > return EFI_SUCCESS;
> > }
> > @@ -159,3 +63,8 @@ unsigned long primary_entry_offset(void)
> > */
> > return (char *)primary_entry - _text;
> > }
> > +
> > +void efi_icache_sync(unsigned long start, unsigned long end)
> > +{
> > + caches_clean_inval_pou(start, end);
> > +}
> > diff --git a/drivers/firmware/efi/libstub/efi-stub-kaslr.c b/drivers/firmware/efi/libstub/efi-stub-kaslr.c
> > new file mode 100644
> > index 000000000000..be0c8ab0982a
> > --- /dev/null
> > +++ b/drivers/firmware/efi/libstub/efi-stub-kaslr.c
> > @@ -0,0 +1,159 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Helper functions used by the EFI stub on multiple
> > + * architectures to deal with physical address space randomization.
> > + */
> > +#include <linux/efi.h>
> > +
> > +#include "efistub.h"
> > +
> > +/**
> > + * efi_kaslr_get_phys_seed() - Get random seed for physical kernel KASLR
> > + * @image_handle: Handle to the image
> > + *
> > + * If KASLR is not disabled, obtain a random seed using EFI_RNG_PROTOCOL
> > + * that will be used to move the kernel physical mapping.
> > + *
> > + * Return: the random seed
> > + */
> > +u32 efi_kaslr_get_phys_seed(efi_handle_t image_handle)
> > +{
> > + efi_status_t status;
> > + u32 phys_seed;
> > + efi_guid_t li_fixed_proto = LINUX_EFI_LOADED_IMAGE_FIXED_GUID;
> > + void *p;
> > +
> > + if (!IS_ENABLED(CONFIG_RANDOMIZE_BASE))
> > + return 0;
> > +
> > + if (efi_nokaslr) {
> > + efi_info("KASLR disabled on kernel command line\n");
> > + } else if (efi_bs_call(handle_protocol, image_handle,
> > + &li_fixed_proto, &p) == EFI_SUCCESS) {
> > + efi_info("Image placement fixed by loader\n");
> > + } else {
> > + status = efi_get_random_bytes(sizeof(phys_seed),
> > + (u8 *)&phys_seed);
> > + if (status == EFI_SUCCESS) {
> > + return phys_seed;
> > + } else if (status == EFI_NOT_FOUND) {
> > + efi_info("EFI_RNG_PROTOCOL unavailable\n");
> > + efi_nokaslr = true;
> > + } else if (status != EFI_SUCCESS) {
> > + efi_err("efi_get_random_bytes() failed (0x%lx)\n",
> > + status);
> > + efi_nokaslr = true;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Distro versions of GRUB may ignore the BSS allocation entirely (i.e., fail
> > + * to provide space, and fail to zero it). Check for this condition by double
> > + * checking that the first and the last byte of the image are covered by the
> > + * same EFI memory map entry.
> > + */
> > +static bool check_image_region(u64 base, u64 size)
> > +{
> > + struct efi_boot_memmap *map;
> > + efi_status_t status;
> > + bool ret = false;
> > + int map_offset;
> > +
> > + status = efi_get_memory_map(&map, false);
> > + if (status != EFI_SUCCESS)
> > + return false;
> > +
> > + for (map_offset = 0; map_offset < map->map_size; map_offset += map->desc_size) {
> > + efi_memory_desc_t *md = (void *)map->map + map_offset;
> > + u64 end = md->phys_addr + md->num_pages * EFI_PAGE_SIZE;
> > +
> > + /*
> > + * Find the region that covers base, and return whether
> > + * it covers base+size bytes.
> > + */
> > + if (base >= md->phys_addr && base < end) {
> > + ret = (base + size) <= end;
> > + break;
> > + }
> > + }
> > +
> > + efi_bs_call(free_pool, map);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * efi_kaslr_relocate_kernel() - Relocate the kernel (random if KASLR enabled)
> > + * @image_addr: Pointer to the current kernel location
> > + * @reserve_addr: Pointer to the relocated kernel location
> > + * @reserve_size: Size of the relocated kernel
> > + * @kernel_size: Size of the text + data
> > + * @kernel_codesize: Size of the text
> > + * @kernel_memsize: Size of the text + data + bss
> > + * @phys_seed: Random seed used for the relocation
> > + *
> > + * If KASLR is not enabled, this function relocates the kernel to a fixed
> > + * address (or leave it as its current location). If KASLR is enabled, the
> > + * kernel physical location is randomized using the seed in parameter.
> > + *
> > + * Return: status code, EFI_SUCCESS if relocation is successful
> > + */
> > +efi_status_t efi_kaslr_relocate_kernel(unsigned long *image_addr,
> > + unsigned long *reserve_addr,
> > + unsigned long *reserve_size,
> > + unsigned long kernel_size,
> > + unsigned long kernel_codesize,
> > + unsigned long kernel_memsize,
> > + u32 phys_seed)
> > +{
> > + efi_status_t status;
> > + u64 min_kimg_align = efi_get_kimg_min_align();
> > +
> > + if (IS_ENABLED(CONFIG_RANDOMIZE_BASE) && phys_seed != 0) {
> > + /*
> > + * If KASLR is enabled, and we have some randomness available,
> > + * locate the kernel at a randomized offset in physical memory.
> > + */
> > + status = efi_random_alloc(*reserve_size, min_kimg_align,
> > + reserve_addr, phys_seed,
> > + EFI_LOADER_CODE);
> > + if (status != EFI_SUCCESS)
> > + efi_warn("efi_random_alloc() failed: 0x%lx\n", status);
> > + } else {
> > + status = EFI_OUT_OF_RESOURCES;
> > + }
> > +
> > + if (status != EFI_SUCCESS) {
> > + if (!check_image_region(*image_addr, kernel_memsize)) {
> > + efi_err("FIRMWARE BUG: Image BSS overlaps adjacent EFI memory region\n");
> > + } else if (IS_ALIGNED(*image_addr, min_kimg_align) &&
> > + (u64)_end < EFI_ALLOC_LIMIT) {
> > + /*
> > + * Just execute from wherever we were loaded by the
> > + * UEFI PE/COFF loader if the placement is suitable.
> > + */
> > + *reserve_size = 0;
> > + return EFI_SUCCESS;
> > + }
> > +
> > + status = efi_allocate_pages_aligned(*reserve_size, reserve_addr,
> > + ULONG_MAX, min_kimg_align,
> > + EFI_LOADER_CODE);
> > +
> > + if (status != EFI_SUCCESS) {
> > + efi_err("Failed to relocate kernel\n");
> > + *reserve_size = 0;
> > + return status;
> > + }
> > + }
> > +
> > + memcpy((void *)*reserve_addr, (void *)*image_addr, kernel_size);
> > + *image_addr = *reserve_addr;
> > + efi_icache_sync(*image_addr, *image_addr + kernel_codesize);
> > + efi_remap_image(*image_addr, *reserve_size, kernel_codesize);
> > +
> > + return status;
> > +}
> > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> > index 6aa38a1bf126..35337ea5056e 100644
> > --- a/drivers/firmware/efi/libstub/efistub.h
> > +++ b/drivers/firmware/efi/libstub/efistub.h
> > @@ -1132,6 +1132,24 @@ const u8 *__efi_get_smbios_string(const struct efi_smbios_record *record,
> >
> > void efi_remap_image(unsigned long image_base, unsigned alloc_size,
> > unsigned long code_size);
> > +efi_status_t efi_kaslr_relocate_kernel(unsigned long *image_addr,
> > + unsigned long *reserve_addr,
> > + unsigned long *reserve_size,
> > + unsigned long kernel_size,
> > + unsigned long kernel_codesize,
> > + unsigned long kernel_memsize,
> > + u32 phys_seed);
> > +u32 efi_kaslr_get_phys_seed(efi_handle_t image_handle);
> > +
> > +#ifndef efi_get_kimg_min_align
> > +static inline unsigned long efi_get_kimg_min_align(void) { return 0; }
>
> Shouldn't this be EFI_PAGE_SIZE at least? Surely, the RISC-V kernel
> image has some minimal alignment?

efi_get_kimg_min_align() is implemented above for riscv, this stub is
here to prevent build failures for other architectures that do not
define it (ie x86). But actually, since I have moved the new functions
to their own file, I'm not sure I still need those stubs, I'll check
and remove them in v5 if possible.

>
>
> > +#define efi_get_kimg_min_align efi_get_kimg_min_align
> > +#endif
> > +
> > +#ifndef efi_icache_sync
> > +static inline void efi_icache_sync(unsigned long start, unsigned long end) {}
> > +#define efi_icache_sync efi_icache_sync
> > +#endif
> >
> > asmlinkage efi_status_t __efiapi
> > efi_zboot_entry(efi_handle_t handle, efi_system_table_t *systab);
> > --
> > 2.39.2
> >

Thanks, and sorry for the truncated series, I'll resend this version
for everyone to have a look at the next 2 patches but keep in mind
your feedback for v5.

Alex

Next message: Greg KH: "Re: [RFC PATCH v1 0/3] docs: stable-kernel-rules: add delayed backporting option and a few tweaks"
Previous message: Ryan Roberts: "Re: [PATCH v1 9/9] selftests/mm: Run all tests from run_vmtests.sh"
In reply to: Ard Biesheuvel: "Re: [PATCH v4 3/5] arm64: libstub: Move KASLR handling functions to efi-stub-helper.c"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]