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

[Ard Biesheuvel]

On Tue, 24 Sep 2019 at 01:17, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Fri, Sep 06, 2019 at 10:34:47AM -0700, Ard Biesheuvel wrote:
> > On Fri, 6 Sep 2019 at 03:44, Will Deacon <will@xxxxxxxxxx> wrote:
> > >
> > > On Wed, Sep 04, 2019 at 01:38:04PM -0700, Kees Cook wrote:
> > > > On Wed, Sep 04, 2019 at 11:38:03AM +0100, Will Deacon wrote:
> > > > > On Wed, Aug 14, 2019 at 01:55:50PM -0700, Kees Cook wrote:
> > > > > > 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?
> > >
> > > Happy to Ack it with that change, although I suppose it's ultimately up
> > > to Ard :)
> > >
> >
> > No objections from me, but I prefer Will's version.
>
> I took a look at this again... to report the failures as Will suggests,
> it would look like this:
>
> --- a/drivers/firmware/efi/libstub/arm64-stub.c
> +++ b/drivers/firmware/efi/libstub/arm64-stub.c
> @@ -138,12 +138,14 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table_arg,
>         }
>
>         if (status != EFI_SUCCESS) {
> +               pr_efi_err(sys_table_arg, "allocate_pages() failed\n");
> +
>                 *reserve_size = kernel_memsize + TEXT_OFFSET;
>                 status = efi_low_alloc(sys_table_arg, *reserve_size,
>                                        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;
>                 }
>
> My reasoning for putting the failure earlier is to differentiate which
> path was taken where the allocate_pages() failed: either regular or
> KASLR. If that's really not considered important here, I can send the
> above patch... Thoughts?
>

The first pr_efi_err() in the patch above complains about a condition
that is not actually an error.

If you are interested in recording the path taken through this
function, I have no objections to putting a normal pr_efi() print
inside the KASLR block that shows that the physical placement of the
kernel is being randomized. Then, we can keep only the second
pr_efi_err() above to report the failure.