efi/arm64: work around Image placement issues
Subsystems (2)
Ard Biesheuvel
maintainerv3 cover efi/arm64: work around Image placement issues
Ben reported that distro GRUB may fail to boot in some circumstances, and tracked it down to an issue in the way distro GRUB allocates space for the image. Due to an oversight (addressed in patch #2), this condition is rarely triggered, but let's work around it in any case (#1)
Remaining patches add further warnings for conditions that are unlikely to occur, but should not be ignored.
Build tested only.
Ard Biesheuvel (4):
efi/libstub: arm64: Force Image reallocation if BSS was not reserved efi/libstub: arm64: Relax 2M alignment again for relocatable kernels efi/libstub: arm64: Warn when efi_random_alloc() fails efi/libstub: arm64: Double check image alignment at entry
drivers/firmware/efi/libstub/arm64-stub.c | 71 +++++++++++++++++--- 1 file changed, 61 insertions(+), 10 deletions(-)
Signature · 1 line
2.20.1
Ard Biesheuvel
maintainerPATCH v3 1/4 efi/libstub: arm64: Force Image reallocation if BSS was not reserved
Distro versions of GRUB replace the usual LoadImage/StartImage calls used to load the kernel image with some local code that fails to honor the allocation requirements described in the PE/COFF header, as it does not account for the image's BSS section at all: it fails to allocate space for it, and fails to zero initialize it.
Since the EFI stub itself is allocated in the .init segment, which is in the middle of the image, its BSS section is not impacted by this, and the main consequence of this omission is that the BSS section may overlap with memory regions that are already used by the firmware.
So let's warn about this condition, and force image reallocation to occur in this case, which works around the problem.
--- drivers/firmware/efi/libstub/arm64-stub.c | 49 +++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-)
drivers/firmware/efi/libstub/arm64-stub.c +48 -1
@@ -34,6 +34,51 @@ efi_status_t check_platform_features(void)return EFI_SUCCESS ; } +/* +* 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) +{ +unsigned long map_size, desc_size, buff_size; +efi_memory_desc_t *memory_map; +struct efi_boot_memmap map; +efi_status_t status; +bool ret = false; +int map_offset; + + map.map = &memory_map; + map.map_size = &map_size; + map.desc_size = &desc_size; + map.desc_ver =NULL ; + map.key_ptr =NULL ; + map.buff_size = &buff_size; + + status =efi_get_memory_map (&map); +if (status !=EFI_SUCCESS ) +return false; + +for (map_offset =0 ; map_offset < map_size; map_offset += desc_size) { +efi_memory_desc_t *md = (void *)memory_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, memory_map); + +return ret; +} +/* * Although relocatable kernels can fix up the misalignment with respect to * MIN_KIMG_ALIGN, the resulting virtual text addresses are subtly out of
@@ -92,7 +137,9 @@ efi_status_t handle_kernel_image(unsigned long *image_addr, }if (status !=EFI_SUCCESS ) { -if (IS_ALIGNED ((u64 )_text,min_kimg_align ())) { +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 ())) {/* * Just execute from wherever we were loaded by the * UEFI PE/COFF loader if the alignment is suitable.
Signature · 1 line
2.20.1
Ard Biesheuvel
maintainerPATCH v3 2/4 efi/libstub: arm64: Relax 2M alignment again for relocatable kernels
In reply to Ard BiesheuvelCommit 82046702e288 ("efi/libstub/arm64: Replace 'preferred' offset with alignment check") simplified the way the stub moves the kernel image around in memory before booting it, given that a relocatable image does not need to be copied to a 2M aligned offset if it was loaded on a 64k boundary by EFI.
Commit d32de9130f6c ("efi/arm64: libstub: Deal gracefully with EFI_RNG_PROTOCOL failure") inadvertently defeated this logic by overriding the value of efi_nokaslr if EFI_RNG_PROTOCOL is not available, which was mistaked by the loader logic as an explicit request on the part of the user to disable KASLR and any associated relocation of an Image not loaded on a 2M boundary.
So let's reinstate this functionality, by capturing the value of efi_nokaslr at function entry to choose the minimum alignment.
--- drivers/firmware/efi/libstub/arm64-stub.c | 28 +++++++++----------- 1 file changed, 13 insertions(+), 15 deletions(-)
drivers/firmware/efi/libstub/arm64-stub.c +13 -15
@@ -79,18 +79,6 @@ static bool check_image_region(u64 base, u64 size)return ret; } -/* -* Although relocatable kernels can fix up the misalignment with respect to -* MIN_KIMG_ALIGN, the resulting virtual text addresses are subtly out of -* sync with those recorded in the vmlinux when kaslr is disabled but the -* image required relocation anyway. Therefore retain 2M alignment unless -* KASLR is in use. -*/ -static u64 min_kimg_align (void ) -{ -return efi_nokaslr ?MIN_KIMG_ALIGN :EFI_KIMG_ALIGN ; -} - efi_status_t handle_kernel_image(unsigned long *image_addr, unsigned long *image_size, unsigned long *reserve_addr,
@@ -101,6 +89,16 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,unsigned long kernel_size, kernel_memsize =0 ;u32 phys_seed =0 ; +/* +* Although relocatable kernels can fix up the misalignment with +* respect to MIN_KIMG_ALIGN, the resulting virtual text addresses are +* subtly out of sync with those recorded in the vmlinux when kaslr is +* disabled but the image required relocation anyway. Therefore retain +* 2M alignment if KASLR was explicitly disabled, even if it was not +* going to be activated to begin with. +*/ +u64 min_kimg_align = efi_nokaslr ?MIN_KIMG_ALIGN :EFI_KIMG_ALIGN ; +if (IS_ENABLED (CONFIG_RANDOMIZE_BASE )) {if (!efi_nokaslr) { status = efi_get_random_bytes(sizeof(phys_seed),
@@ -130,7 +128,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,* 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 (), + status =efi_random_alloc (*reserve_size, min_kimg_align, reserve_addr, phys_seed); }else { status =EFI_OUT_OF_RESOURCES ;
@@ -139,7 +137,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,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 ())) { + }else if (IS_ALIGNED ((u64 )_text, min_kimg_align)) {/* * Just execute from wherever we were loaded by the * UEFI PE/COFF loader if the alignment is suitable.
@@ -150,7 +148,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr, } status =efi_allocate_pages_aligned (*reserve_size, reserve_addr, -ULONG_MAX ,min_kimg_align ()); +ULONG_MAX , min_kimg_align);if (status !=EFI_SUCCESS ) {efi_err ("Failed to relocate kernel\n" );
Signature · 1 line
2.20.1
Ard Biesheuvel
maintainerPATCH v3 3/4 efi/libstub: arm64: Warn when efi_random_alloc() fails
In reply to Ard BiesheuvelRandomization of the physical load address of the kernel image relies on efi_random_alloc() returning successfully, and currently, we ignore any failures and just carry on, using the ordinary, non-randomized page allocator routine. This means we never find out if a failure occurs, which could harm security, so let's at least warn about this condition.
--- drivers/firmware/efi/libstub/arm64-stub.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/firmware/efi/libstub/arm64-stub.c +2 -0
@@ -130,6 +130,8 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,*/ status =efi_random_alloc (*reserve_size, min_kimg_align, reserve_addr, phys_seed); +if (status !=EFI_SUCCESS ) +efi_warn ("efi_random_alloc() failed: 0x%lx\n" , status); }else { status =EFI_OUT_OF_RESOURCES ; }
Signature · 1 line
2.20.1
Ard Biesheuvel
maintainerPATCH v3 4/4 efi/libstub: arm64: Double check image alignment at entry
In reply to Ard BiesheuvelOn arm64, the stub only moves the kernel image around in memory if needed, which is typically only for KASLR, given that relocatable kernels (which is the default) can run from any 64k aligned address, which is also the minimum alignment communicated to EFI via the PE/COFF header.
Unfortunately, some loaders appear to ignore this header, and load the kernel at some arbitrary offset in memory. We can deal with this, but let's check for this condition anyway, so non-compliant code can be spotted and fixed.
--- drivers/firmware/efi/libstub/arm64-stub.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/firmware/efi/libstub/arm64-stub.c +4 -0
@@ -119,6 +119,10 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,if (image->image_base != _text)efi_err ("FIRMWARE BUG: efi_loaded_image_t::image_base has bogus value\n" ); +if (!IS_ALIGNED ((u64 )_text,EFI_KIMG_ALIGN )) +efi_err ("FIRMWARE BUG: kernel image not aligned on %ldk boundary\n" , +EFI_KIMG_ALIGN >>10 ); + kernel_size = _edata - _text; kernel_memsize = kernel_size + (_end - _edata); *reserve_size = kernel_memsize;
Signature · 1 line
2.20.1
Quoted 11 lines · Ard Biesheuvel
Ben reported that distro GRUB may fail to boot in some circumstances, and tracked it down to an issue in the way distro GRUB allocates space for the image. Due to an oversight (addressed in patch #2), this condition is rarely triggered, but let's work around it in any case (#1)
Remaining patches add further warnings for conditions that are unlikely to occur, but should not be ignored.
Build tested only.
Cc Benjamin Herrenschmidt <[email protected]>
All 4 patches:
I checked that it catches all of the errors, including reverting my fix for randomalloc.c and verifying that it caught (and successfully worked around) the original boot crash.
Cheers,
Ben.
Ard Biesheuvel
maintainerQuoted 14 lines · Benjamin Herrenschmidt
Quoted 11 lines · Ard Biesheuvel
Ben reported that distro GRUB may fail to boot in some circumstances, and tracked it down to an issue in the way distro GRUB allocates space for the image. Due to an oversight (addressed in patch #2), this condition is rarely triggered, but let's work around it in any case (#1)
Remaining patches add further warnings for conditions that are unlikely to occur, but should not be ignored.
Build tested only.
Cc Benjamin Herrenschmidt <[email protected]>
Quoted 8 lines
All 4 patches:
I checked that it catches all of the errors, including reverting my fix for
randomalloc.cand verifying that it caught (and successfully worked around) the original boot crash.
Excellent. Thanks for tracking this down and getting it fixed properly.