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

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

[Sebastian Andrzej Siewior]

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)

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
…

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

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---

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.

 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;
+	}
 	bgrt_image_size = bmp_header.size;
 
 	bgrt_image = kmalloc(bgrt_image_size, GFP_KERNEL | __GFP_NOWARN);
-- 
2.1.4

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

[Matt Fleming]

(Pulling in Josh)

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)
> 
> 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
> …
> 
> now and then. The data behind that pointer changes on each boot because
> nobody preserves the content across kexec.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> 
> 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.

>  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;
> +	}
>  	bgrt_image_size = bmp_header.size;
>  
>  	bgrt_image = kmalloc(bgrt_image_size, GFP_KERNEL | __GFP_NOWARN);

I'm confused, is the BMP image valid on your machine or not? You add a
validity check but talk about compressing it above.

-- 
Matt Fleming, Intel Open Source Technology Center

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

[josh]

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@linutronix.de>
> > ---
> > 
> > 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

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

[Sebastian Andrzej Siewior]

On 07/29/2015 02:10 AM, josh@joshtriplett.org wrote:
>> On Wed, 22 Jul, at 05:32:44PM, Sebastian Andrzej Siewior wrote:
>>> 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?)

>From what I know kexec entry is transparent. Could you remove the table
from ACPI? There is no point on having this bitmap information now,
right? The last way out would be to patch kexec and let it read the
table from /sys and put it at the spot mentioned in the ACPI table.

>>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>>> ---
>>>
>>> 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.

This is correct. However I miss the point of saving the image in the
first place. From what I see is that I have now 272 KiB in memory which
are never used again. Is there a usecase why we have it? From the code
it looks like we save it during boot and make it available later via
sysfs.

>>>  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.

But it is one additional check to make sure we look at proper data. The
(ACPI-table) header has an image type which is to BMP (the only
currently support image type) so this is the double check. And the
kernel should be able to read from any address as long as it is within
its DRAM.

>  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.

So you assume that the information from ACPI is always correct then?
The pointer is correct, the information it points to is no longer.

If we run always under EFI then it looks like the variable efi_setup
which is checked in efi_enter_virtual_mode() is 0 during normal boot
and != 0 on kexec entry. This hint is set via setup_data / SETUP_EFI
since commit 1fec053369 ("x86/efi: Pass necessary EFI data for kexec
via setup_data"). So maybe we could use this to check if we run under
kexec or not.

> - Josh Triplett

Sebastian

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

[Dave Young]

Hi,

> 
> >  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.
> 
> So you assume that the information from ACPI is always correct then?
> The pointer is correct, the information it points to is no longer.
> 
> If we run always under EFI then it looks like the variable efi_setup
> which is checked in efi_enter_virtual_mode() is 0 during normal boot
> and != 0 on kexec entry. This hint is set via setup_data / SETUP_EFI
> since commit 1fec053369 ("x86/efi: Pass necessary EFI data for kexec
> via setup_data"). So maybe we could use this to check if we run under
> kexec or not.

Not sure if BGRT is useful in kexec kernel, it seems not worth to copy
it between kernels.

IMO just return in case if (efi_setup) is true make sense.

Thanks
Dave

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

[Josh Triplett]

On Wed, Jul 29, 2015 at 10:30:51AM +0200, Sebastian Andrzej Siewior wrote:
> On 07/29/2015 02:10 AM, josh@joshtriplett.org wrote:
> >> On Wed, 22 Jul, at 05:32:44PM, Sebastian Andrzej Siewior wrote:
> >>> 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?)
> 
> From what I know kexec entry is transparent. Could you remove the table
> from ACPI? There is no point on having this bitmap information now,
> right? The last way out would be to patch kexec and let it read the
> table from /sys and put it at the spot mentioned in the ACPI table.

Assuming that memory isn't already in use.  Seems non-trivial.  Another
alternative would be to patch the address in the ACPI table.  But for a
first pass, I think it'd suffice to just stop attempting to parse the
BGRT at all from the kexec'd kernel.

> >>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> >>> ---
> >>>
> >>> 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.
> 
> This is correct. However I miss the point of saving the image in the
> first place. From what I see is that I have now 272 KiB in memory which
> are never used again. Is there a usecase why we have it? From the code
> it looks like we save it during boot and make it available later via
> sysfs.

That's the point, yes.  A splash screen or an about box can then read it
from there later and display it to the user.

> >>>  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.
> 
> But it is one additional check to make sure we look at proper data. The
> (ACPI-table) header has an image type which is to BMP (the only
> currently support image type) so this is the double check.

I agree completely with adding the check; I'm just saying it isn't a
complete fix.

> And the
> kernel should be able to read from any address as long as it is within
> its DRAM.

Then what caused the oops on early_ioremap?

> >  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.
> 
> So you assume that the information from ACPI is always correct then?
> The pointer is correct, the information it points to is no longer.
> 
> If we run always under EFI then it looks like the variable efi_setup
> which is checked in efi_enter_virtual_mode() is 0 during normal boot
> and != 0 on kexec entry. This hint is set via setup_data / SETUP_EFI
> since commit 1fec053369 ("x86/efi: Pass necessary EFI data for kexec
> via setup_data"). So maybe we could use this to check if we run under
> kexec or not.

That sounds like a sensible fix; don't try to parse the BGRT if
efi_setup.

- Josh Triplett

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

[Matt Fleming]

On Wed, 29 Jul, at 05:37:52PM, Dave Young wrote:
> 
> Not sure if BGRT is useful in kexec kernel, it seems not worth to copy
> it between kernels.
> 
> IMO just return in case if (efi_setup) is true make sense.

Yes, I think it makes sense to skip touching the BGRT on kexec boot.

As a side note, we should really look at using one of the efi_enabled()
bits to signal when we're booting a kexec'd kernel because it is not at
all obvious that 'efi_setup != 0' means we're in kexec kernel.

But that wants to be a separate patch from this issue.

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Sebastian Andrzej Siewior]

On 07/29/2015 06:41 PM, Josh Triplett wrote:

>> This is correct. However I miss the point of saving the image in the
>> first place. From what I see is that I have now 272 KiB in memory which
>> are never used again. Is there a usecase why we have it? From the code
>> it looks like we save it during boot and make it available later via
>> sysfs.
> 
> That's the point, yes.  A splash screen or an about box can then read it
> from there later and display it to the user.
> 
>>>>>  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.
>>
>> But it is one additional check to make sure we look at proper data. The
>> (ACPI-table) header has an image type which is to BMP (the only
>> currently support image type) so this is the double check.
> 
> I agree completely with adding the check; I'm just saying it isn't a
> complete fix.

okay, so how do we continue here? You ack that one  and send the other
patch on top or do you want first that kexec patch and then the BMP
check?

>> And the
>> kernel should be able to read from any address as long as it is within
>> its DRAM.
> 
> Then what caused the oops on early_ioremap?

from the WARN_ON() in ioremap:

         /*
          * Mappings have to fit in the FIX_BTMAP area.
          */
         nrpages = size >> PAGE_SHIFT;
         if (WARN_ON(nrpages > NR_FIX_BTMAPS))
                 return NULL;

This means the size of the ioremap is limited to 256KiB. So lets say we
get 300KiB as the image size: we managed to allocate say 300KiB via
kmalloc() and later we get the warn_on() here because we can't remap
more than 256KiB.

So we can ignore this until a BIOS includes a real image with a size >
256KiB. Another way around it would be get memblock to ignore this
region and give it back later.

> That sounds like a sensible fix; don't try to parse the BGRT if
> efi_setup.

okay.

> - Josh Triplett

Sebastian

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

[Josh Triplett]

On Thu, Jul 30, 2015 at 06:33:41PM +0200, Sebastian Andrzej Siewior wrote:
> On 07/29/2015 06:41 PM, Josh Triplett wrote:
> 
> >> This is correct. However I miss the point of saving the image in the
> >> first place. From what I see is that I have now 272 KiB in memory which
> >> are never used again. Is there a usecase why we have it? From the code
> >> it looks like we save it during boot and make it available later via
> >> sysfs.
> > 
> > That's the point, yes.  A splash screen or an about box can then read it
> > from there later and display it to the user.
> > 
> >>>>>  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.
> >>
> >> But it is one additional check to make sure we look at proper data. The
> >> (ACPI-table) header has an image type which is to BMP (the only
> >> currently support image type) so this is the double check.
> > 
> > I agree completely with adding the check; I'm just saying it isn't a
> > complete fix.
> 
> okay, so how do we continue here? You ack that one  and send the other
> patch on top or do you want first that kexec patch and then the BMP
> check?

I don't see any problem with the BMP format patch, other than that I'd
suggest clarifying the scope of the fix in the commit message.

You should then also submit a separate patch to check efi_setup.

> >> And the
> >> kernel should be able to read from any address as long as it is within
> >> its DRAM.
> > 
> > Then what caused the oops on early_ioremap?
> 
> from the WARN_ON() in ioremap:
> 
>          /*
>           * Mappings have to fit in the FIX_BTMAP area.
>           */
>          nrpages = size >> PAGE_SHIFT;
>          if (WARN_ON(nrpages > NR_FIX_BTMAPS))
>                  return NULL;
> 
> This means the size of the ioremap is limited to 256KiB. So lets say we
> get 300KiB as the image size: we managed to allocate say 300KiB via
> kmalloc() and later we get the warn_on() here because we can't remap
> more than 256KiB.
> 
> So we can ignore this until a BIOS includes a real image with a size >
> 256KiB. Another way around it would be get memblock to ignore this
> region and give it back later.

Ah, I see; sorry, I thought this was an access violation of some kind
(poking at memory that doesn't want poking), rather than a size issue.
Thanks for clarifying.

- Josh Triplett