Re: [PATCH] efi/libstub/arm64: Report meaningful relocation errors

[Will Deacon]

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@xxxxxxxxxxxx>
> ---
>  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;
 }