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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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