Messages in this thread Patch in this message | | | Date | Mon, 23 Sep 2019 16:17:02 -0700 | From | Kees Cook <> | Subject | Re: [PATCH] efi/libstub/arm64: Report meaningful relocation errors |
| |
On Fri, Sep 06, 2019 at 10:34:47AM -0700, Ard Biesheuvel wrote: > On Fri, 6 Sep 2019 at 03:44, Will Deacon <will@kernel.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.
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?
-- Kees Cook
| |