Re: [PATCH] x86/mm, efi: Check for valid image type

[josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org]

On Tue, Jul 28, 2015 at 09:51:57PM +0100, Matt Fleming wrote:
> (Pulling in Josh)

Thanks, Matt.

> On Wed, 22 Jul, at 05:32:44PM, Sebastian Andrzej Siewior wrote:
> > I usually see
> > |Ignoring BGRT: failed to allocate memory for image (wanted 264301314 bytes)
> > |Ignoring BGRT: failed to allocate memory for image (wanted 3925872891 bytes)

Yikes.  258MB or 3.9GB are completely bogus, yes.

> > sometimes I get
> > 
> > |------------[ cut here ]------------
> > |WARNING: CPU: 0 PID: 0 at mm/early_ioremap.c:136 __early_ioremap.constprop.0+0x113/0x1d3()
> > …
> > | [<ffffffff81b3de8c>] __early_ioremap.constprop.0+0x113/0x1d3
> > | [<ffffffff81b3e106>] early_ioremap+0x13/0x15
> > | [<ffffffff81b2c4a9>] efi_bgrt_init+0x1e2/0x27d
> > …

That should definitely never happen.

> > now and then. The data behind that pointer changes on each boot because
> > nobody preserves the content across kexec.

Right.  The kernel copies this image precisely because it may go away at
ExitBootServices or similar, or when ACPI reclaim space is freed.  This
ties into the various work about trying to make kexec handle EFI and
ACPI.  Is there some way we can indicate to the kexec kernel that this
won't work?  (Or fix it so it will?)

> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> > ---
> > 
> > I don't know much about the requirement of having the .bmp in memory all the
> > time. Would it be a bad thing to compress the bmp and uncompress on cat from
> > userland? In my case the bmp has 272 KiB and LZO gets it down to 12KiB,
> > XZ 7.4KiB.
> 
> The usual use for BGRT is to display it during kernel boot, so
> interacting with userland doesn't help you there.

If we're going to be storing a large image, applying simple in-kernel
compression doesn't seem unreasonable, if we then decompress it when
read from the BGRT sysfs file.  That's entirely separate from this issue
though.

> >  arch/x86/platform/efi/efi-bgrt.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
> > index d7f997f7c26d..59710f0875bb 100644
> > --- a/arch/x86/platform/efi/efi-bgrt.c
> > +++ b/arch/x86/platform/efi/efi-bgrt.c
> > @@ -79,6 +79,10 @@ void __init efi_bgrt_init(void)
> >  	memcpy_fromio(&bmp_header, image, sizeof(bmp_header));
> >  	if (ioremapped)
> >  		early_iounmap(image, sizeof(bmp_header));
> > +	if (bmp_header.id != 0x4d42) {
> > +		pr_err("BGRT: Not a valid BMP file.\n");
> > +		return;
> > +	}

That's a good idea as an additional cross-check, but not a complete fix
for the problem.  As you pointed out above, a wild pointer could cause a
WARN from early_ioremap.  We need to never follow the pointer in the
first place after a kexec, unless we have some way to know that it's
actually valid.

- Josh Triplett