Messages in this thread Patch in this message | | | Date | Wed, 4 Sep 2019 11:38:03 +0100 | From | Will Deacon <> | Subject | Re: [PATCH] efi/libstub/arm64: Report meaningful relocation errors |
| |
Hi Kees,
On Wed, Aug 14, 2019 at 01:55:50PM -0700, Kees Cook wrote: > When UEFI booting, if allocate_pages() fails (either via KASLR or > regular boot), efi_low_alloc() is used for fall back. If it, too, fails, > it reports "Failed to relocate kernel". Then handle_kernel_image() > reports the failure to its caller, which unhelpfully reports exactly > the same string again: > > EFI stub: ERROR: Failed to relocate kernel > EFI stub: ERROR: Failed to relocate kernel > > While debugging linker errors in the UEFI code that created insane memory > sizes that all the allocation attempts would fail at, this was a cause > for confusion. Knowing each allocation had failed would have helped me > isolate the issue sooner. To that end, this improves the error messages > to detail which specific allocations have failed. > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > drivers/firmware/efi/libstub/arm64-stub.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c > index 1550d244e996..24022f956e01 100644 > --- a/drivers/firmware/efi/libstub/arm64-stub.c > +++ b/drivers/firmware/efi/libstub/arm64-stub.c > @@ -111,6 +111,8 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table_arg, > status = efi_random_alloc(sys_table_arg, *reserve_size, > MIN_KIMG_ALIGN, reserve_addr, > (u32)phys_seed); > + if (status != EFI_SUCCESS) > + pr_efi_err(sys_table_arg, "KASLR allocate_pages() failed\n"); > > *image_addr = *reserve_addr + offset; > } else { > @@ -135,6 +137,8 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table_arg, > EFI_LOADER_DATA, > *reserve_size / EFI_PAGE_SIZE, > (efi_physical_addr_t *)reserve_addr); > + if (status != EFI_SUCCESS) > + pr_efi_err(sys_table_arg, "regular allocate_pages() failed\n"); > }
Not sure I see the need to distinsuish the 'KASLR' case from the 'regular' case -- only one should run, right? That also didn't seem to be part of the use-case in the commit, unless I'm missing something.
Maybe combine the prints as per the diff below?
Will
--->8
diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c index 1550d244e996..820c58cc149e 100644 --- a/drivers/firmware/efi/libstub/arm64-stub.c +++ b/drivers/firmware/efi/libstub/arm64-stub.c @@ -143,13 +143,15 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table_arg, MIN_KIMG_ALIGN, reserve_addr); if (status != EFI_SUCCESS) { - pr_efi_err(sys_table_arg, "Failed to relocate kernel\n"); + pr_efi_err(sys_table_arg, "efi_low_alloc() failed\n"); *reserve_size = 0; return status; } *image_addr = *reserve_addr + TEXT_OFFSET; + } else { + pr_efi_err(sys_table_arg, "allocate_pages() failed\n"); } - memcpy((void *)*image_addr, old_image_addr, kernel_size); + memcpy((void *)*image_addr, old_image_addr, kernel_size); return EFI_SUCCESS; }
| |