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

[Kees Cook]

On Wed, Sep 04, 2019 at 11:38:03AM +0100, Will Deacon wrote:
> 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.

I just did that to help with differentiating the cases. Maybe something
was special about KASLR picking the wrong location that triggered the
failure, etc.

> Maybe combine the prints as per the diff below?

That could work. If you're against the KASLR vs regular thing, I can
respin the patch?

-Kees

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

-- 
Kees Cook