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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKv+Gu9yHxUV2GAuPG=HWGRt81LhSVisABDpUZxyDkLJffxy6A@mail.gmail.com>
Date:   Fri, 6 Sep 2019 10:34:47 -0700
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     Will Deacon <will@...nel.org>
Cc:     Kees Cook <keescook@...omium.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH] efi/libstub/arm64: Report meaningful relocation errors

On Fri, 6 Sep 2019 at 03:44, Will Deacon <will@...nel.org> 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.