Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1190063 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] x86/mm, efi: Check for valid image type Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2015-07-22 17:40 +0200
    Re: [PATCH] x86/mm, efi: Check for valid image type Matt Fleming <matt@codeblueprint.co.uk> - 2015-07-28 23:00 +0200
      Re: [PATCH] x86/mm, efi: Check for valid image type Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2015-07-29 10:40 +0200
        Re: [PATCH] x86/mm, efi: Check for valid image type Dave Young <dyoung@redhat.com> - 2015-07-29 11:40 +0200
        Re: [PATCH] x86/mm, efi: Check for valid image type Josh Triplett <josh@joshtriplett.org> - 2015-07-30 21:10 +0200

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1194665

(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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1194942

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1194996

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1196379

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web