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 <keesc...@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.
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
