[PATCH] efi-bgrt: Add error handling; inform the user when ignoring the BGRT

[PATCH] efi-bgrt: Add error handling; inform the user when ignoring the BGRT

[Josh Triplett]

Gracefully handle failures to allocate memory for the image, which might
be arbitrarily large.

efi_bgrt_init can fail in various ways as well, usually because the
BIOS-provided BGRT structure does not match expectations.  Add
appropriate error messages rather than failing silently.

Reported-by: Srihari Vijayaraghavan
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=81321
Signed-off-by: Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org>
---
 arch/x86/platform/efi/efi-bgrt.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
index f15103d..d143d21 100644
--- a/arch/x86/platform/efi/efi-bgrt.c
+++ b/arch/x86/platform/efi/efi-bgrt.c
@@ -40,20 +40,40 @@ void __init efi_bgrt_init(void)
 	if (ACPI_FAILURE(status))
 		return;
 
-	if (bgrt_tab->header.length < sizeof(*bgrt_tab))
+	if (bgrt_tab->header.length < sizeof(*bgrt_tab)) {
+		pr_err("Ignoring BGRT: invalid length %u (expected %zu)\n",
+		       bgrt_tab->header.length, sizeof(*bgrt_tab));
 		return;
-	if (bgrt_tab->version != 1 || bgrt_tab->status != 1)
+	}
+	if (bgrt_tab->version != 1) {
+		pr_err("Ignoring BGRT: invalid version %u (expected 1)\n",
+		       bgrt_tab->version);
+		return;
+	}
+	if (bgrt_tab->status != 1) {
+		pr_err("Ignoring BGRT: invalid status %u (expected 1)\n",
+		       bgrt_tab->status);
+		return;
+	}
+	if (bgrt_tab->image_type != 0) {
+		pr_err("Ignoring BGRT: invalid image type %u (expected 0)\n",
+		       bgrt_tab->image_type);
 		return;
-	if (bgrt_tab->image_type != 0 || !bgrt_tab->image_address)
+	}
+	if (!bgrt_tab->image_address) {
+		pr_err("Ignoring BGRT: null image address\n");
 		return;
+	}
 
 	image = efi_lookup_mapped_addr(bgrt_tab->image_address);
 	if (!image) {
 		image = early_memremap(bgrt_tab->image_address,
 				       sizeof(bmp_header));
 		ioremapped = true;
-		if (!image)
+		if (!image) {
+			pr_err("Ignoring BGRT: failed to map image header memory\n");
 			return;
+		}
 	}
 
 	memcpy_fromio(&bmp_header, image, sizeof(bmp_header));
@@ -61,14 +81,18 @@ void __init efi_bgrt_init(void)
 		early_iounmap(image, sizeof(bmp_header));
 	bgrt_image_size = bmp_header.size;
 
-	bgrt_image = kmalloc(bgrt_image_size, GFP_KERNEL);
-	if (!bgrt_image)
+	bgrt_image = kmalloc(bgrt_image_size, GFP_KERNEL | __GFP_NOWARN);
+	if (!bgrt_image) {
+		pr_err("Ignoring BGRT: failed to allocate memory for image (wanted %zu bytes)\n",
+		       bgrt_image_size);
 		return;
+	}
 
 	if (ioremapped) {
 		image = early_memremap(bgrt_tab->image_address,
 				       bmp_header.size);
 		if (!image) {
+			pr_err("Ignoring BGRT: failed to map image memory\n");
 			kfree(bgrt_image);
 			bgrt_image = NULL;
 			return;
-- 
2.0.1

Re: [PATCH] efi-bgrt: Add error handling; inform the user when ignoring the BGRT

[Matt Fleming]

On Wed, 30 Jul, at 12:23:32PM, Josh Triplett wrote:
> Gracefully handle failures to allocate memory for the image, which might
> be arbitrarily large.
> 
> efi_bgrt_init can fail in various ways as well, usually because the
> BIOS-provided BGRT structure does not match expectations.  Add
> appropriate error messages rather than failing silently.
> 
> Reported-by: Srihari Vijayaraghavan

This could really do with an email address appending.

> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=81321
> Signed-off-by: Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org>
> ---
>  arch/x86/platform/efi/efi-bgrt.c | 36 ++++++++++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 6 deletions(-)

[...]

> @@ -61,14 +81,18 @@ void __init efi_bgrt_init(void)
>  		early_iounmap(image, sizeof(bmp_header));
>  	bgrt_image_size = bmp_header.size;
>  
> -	bgrt_image = kmalloc(bgrt_image_size, GFP_KERNEL);
> -	if (!bgrt_image)
> +	bgrt_image = kmalloc(bgrt_image_size, GFP_KERNEL | __GFP_NOWARN);
> +	if (!bgrt_image) {
> +		pr_err("Ignoring BGRT: failed to allocate memory for image (wanted %zu bytes)\n",
> +		       bgrt_image_size);
>  		return;

I'm not sure that using __GFP_NOWARN is the right thing to do here. If
for some reason we can't handle the BGRT image we should include checks
in the BGRT code, rather than relying on the page-allocation machinery
to save us.

Let's just use an explicit limit on the size of the BGRT image we're
willing to handle.

Either way, this hunk shouldn't be mixed in with the improved error
logging changes, it needs to be a separate patch.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] efi-bgrt: Add error handling; inform the user when ignoring the BGRT

[josh]

On Thu, Jul 31, 2014 at 11:31:10AM +0100, Matt Fleming wrote:
> On Wed, 30 Jul, at 12:23:32PM, Josh Triplett wrote:
> > @@ -61,14 +81,18 @@ void __init efi_bgrt_init(void)
> >  		early_iounmap(image, sizeof(bmp_header));
> >  	bgrt_image_size = bmp_header.size;
> >  
> > -	bgrt_image = kmalloc(bgrt_image_size, GFP_KERNEL);
> > -	if (!bgrt_image)
> > +	bgrt_image = kmalloc(bgrt_image_size, GFP_KERNEL | __GFP_NOWARN);
> > +	if (!bgrt_image) {
> > +		pr_err("Ignoring BGRT: failed to allocate memory for image (wanted %zu bytes)\n",
> > +		       bgrt_image_size);
> >  		return;
> 
> I'm not sure that using __GFP_NOWARN is the right thing to do here. If
> for some reason we can't handle the BGRT image we should include checks
> in the BGRT code, rather than relying on the page-allocation machinery
> to save us.
> 
> Let's just use an explicit limit on the size of the BGRT image we're
> willing to handle.

I started to add an explicit limit, but any reasonable limit (large
enough for modern screens) would be large enough that there's still a
non-trivial possibility of allocation failure.  And I think it makes
sense for BGRT image allocation to be non-fatal and minimally noisy
(just a single-line error, not a scary-looking allocation warning),
considering the highly optional and cosmetic nature of BGRT.  So, I
believe __GFP_NOWARN makes sense.

- Josh Triplett

Re: [PATCH] efi-bgrt: Add error handling; inform the user when ignoring the BGRT

[Matt Fleming]

(Including akpm, the __GFP_NOWARN police)

On Thu, 31 Jul, at 09:11:33AM, Josh Triplett wrote:
> 
> I started to add an explicit limit, but any reasonable limit (large
> enough for modern screens) would be large enough that there's still a
> non-trivial possibility of allocation failure.  And I think it makes
> sense for BGRT image allocation to be non-fatal and minimally noisy
> (just a single-line error, not a scary-looking allocation warning),
> considering the highly optional and cosmetic nature of BGRT.  So, I
> believe __GFP_NOWARN makes sense.

Yes, I agree that we don't want to trigger the page allocator warning,
but I don't agree that passing __GFP_NOWARN is OK, which is why I'm
advocating some size limit checks.

We need to distinguish between "Your BGRT image size is huge, and
assumed buggy" and "Your BGRT looks valid, but we ran out of memory".

We've already got enough problems with the EFI code because we silently
paper over bugs, and using the page allocator's failure path as a way to
check for buggy BGRT images just doesn't make any sense to me at all.

If we get the limit wrong, it's not the end of the world, we can change
it later, but it's a safe bet that if the firmware engineers start
seeing "BGRT is buggy" in the kernel log they're going to start a
dialogue with us.

Original patch,

  https://lkml.org/lkml/2014/7/30/543

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] efi-bgrt: Add error handling; inform the user when ignoring the BGRT

[Josh Triplett]

On Fri, Aug 01, 2014 at 10:19:49AM +0100, Matt Fleming wrote:
> (Including akpm, the __GFP_NOWARN police)

Andrew suggested __GFP_NOWARN here in the first place.

> On Thu, 31 Jul, at 09:11:33AM, Josh Triplett wrote:
> > 
> > I started to add an explicit limit, but any reasonable limit (large
> > enough for modern screens) would be large enough that there's still a
> > non-trivial possibility of allocation failure.  And I think it makes
> > sense for BGRT image allocation to be non-fatal and minimally noisy
> > (just a single-line error, not a scary-looking allocation warning),
> > considering the highly optional and cosmetic nature of BGRT.  So, I
> > believe __GFP_NOWARN makes sense.
> 
> Yes, I agree that we don't want to trigger the page allocator warning,
> but I don't agree that passing __GFP_NOWARN is OK, which is why I'm
> advocating some size limit checks.
> 
> We need to distinguish between "Your BGRT image size is huge, and
> assumed buggy" and "Your BGRT looks valid, but we ran out of memory".
> 
> We've already got enough problems with the EFI code because we silently
> paper over bugs, and using the page allocator's failure path as a way to
> check for buggy BGRT images just doesn't make any sense to me at all.
> 
> If we get the limit wrong, it's not the end of the world, we can change
> it later, but it's a safe bet that if the firmware engineers start
> seeing "BGRT is buggy" in the kernel log they're going to start a
> dialogue with us.

The original bug report was about an allocation failure for a fairly
reasonable BGRT size.  We can certainly prohibit absurdly huge ones (for
instance, bigger than the maximum likely screen resolution times 4 bytes
per pixel), but allocation failures may well occur for smaller sizes,
and I don't think we want to spew a massive warning for that either.

- Josh Triplett

Re: [PATCH] efi-bgrt: Add error handling; inform the user when ignoring the BGRT

[Matt Fleming]

On Fri, 01 Aug, at 09:11:54AM, Josh Triplett wrote:
> 
> The original bug report was about an allocation failure for a fairly
> reasonable BGRT size.  We can certainly prohibit absurdly huge ones (for
> instance, bigger than the maximum likely screen resolution times 4 bytes
> per pixel), but allocation failures may well occur for smaller sizes,
> and I don't think we want to spew a massive warning for that either.

Oh, dammit, that's my bad. I misread the allocation size and thought it
was huge, but now realise it was only 6MB or so. Sorry Josh.

I was worried that this was the first reported instance of a BGRT
claiming to be valid but with a bogusly large image size. I've never
been so happy to be wrong.

However, the fact that the allocation failed is worth investigating -
this machine appears to have GBs of ram. Perhaps we should switch to
requesting pages directly instead of relying on kmalloc()? 

I appreciate that the BGRT code isn't mission critical or anything like
that, and that failing the alloc isn't the end of the world, but if we
have code in the kernel it should really be as robust as possible. I
don't think trying to kmalloc() ~6MB can claim to be robust.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] efi-bgrt: Add error handling; inform the user when ignoring the BGRT

[Josh Triplett]

On Mon, Aug 04, 2014 at 01:19:59PM +0100, Matt Fleming wrote:
> On Fri, 01 Aug, at 09:11:54AM, Josh Triplett wrote:
> > 
> > The original bug report was about an allocation failure for a fairly
> > reasonable BGRT size.  We can certainly prohibit absurdly huge ones (for
> > instance, bigger than the maximum likely screen resolution times 4 bytes
> > per pixel), but allocation failures may well occur for smaller sizes,
> > and I don't think we want to spew a massive warning for that either.
> 
> Oh, dammit, that's my bad. I misread the allocation size and thought it
> was huge, but now realise it was only 6MB or so. Sorry Josh.
> 
> I was worried that this was the first reported instance of a BGRT
> claiming to be valid but with a bogusly large image size. I've never
> been so happy to be wrong.

:)

> However, the fact that the allocation failed is worth investigating -
> this machine appears to have GBs of ram. Perhaps we should switch to
> requesting pages directly instead of relying on kmalloc()? 
> 
> I appreciate that the BGRT code isn't mission critical or anything like
> that, and that failing the alloc isn't the end of the world, but if we
> have code in the kernel it should really be as robust as possible. I
> don't think trying to kmalloc() ~6MB can claim to be robust.

vmalloc or flex_array could potentially help here.  However, I'd suggest
we go ahead and merge this patch to improve the existing error handling
before doing a more extensive rewrite to use one of those.

Would anything go horrifically wrong if this allocation used vmalloc?
We really don't care deeply about the performance of this memory; it
just needs a single copy in and a small number of copies out in the
lifetime of a system.

- Josh Triplett