Re: [PATCH] efi/libstub/arm64: Report meaningful relocation errors
On Tue, 24 Sep 2019 at 01:17, Kees Cook wrote:
>
> On Fri, Sep 06, 2019 at 10:34:47AM -0700, Ard Biesheuvel wrote:
> > On Fri, 6 Sep 2019 at 03:44, Will Deacon 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.
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 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
Re: [PATCH] efi/libstub/arm64: Report meaningful relocation errors
On Fri, 6 Sep 2019 at 03:44, Will Deacon 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.
Re: [PATCH] efi/libstub/arm64: Report meaningful relocation errors
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 :)
Will
Re: [PATCH] efi/libstub/arm64: Report meaningful relocation errors
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
> > ---
> > 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
Re: [PATCH] efi/libstub/arm64: Report meaningful relocation errors
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
> ---
> 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.
Maybe combine the prints as per the diff below?
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;
}
[PATCH] efi/libstub/arm64: Report meaningful relocation errors
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
---
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");
}
if (status != EFI_SUCCESS) {
@@ -143,7 +147,7 @@ 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;
}
--
2.17.1
--
Kees Cook
