BGRT warns again on my system

BGRT warns again on my system

[Andy Lutomirski]

My garbage workstation splats like this on boot:

[    0.160523] ioremap: invalid physical address d0c801800000001

This was fixed for a while, but I think the problem was reintroduced by:

commit 66dbe99cfe30e113d2e571e68b9b6a1a8985a157
Author: Môshe van der Sterre <me-A/3C56C7qwM@public.gmane.org>
Date:   Mon Feb 1 22:07:03 2016 +0000

    x86/efi/bgrt: Don't ignore the BGRT if the 'valid' bit is 0

Is there any good way to fix this again?

Re: BGRT warns again on my system

[Josh Triplett]

On Sat, May 28, 2016 at 01:46:07PM -0700, Andy Lutomirski wrote:
> My garbage workstation splats like this on boot:
> 
> [    0.160523] ioremap: invalid physical address d0c801800000001
> 
> This was fixed for a while, but I think the problem was reintroduced by:
> 
> commit 66dbe99cfe30e113d2e571e68b9b6a1a8985a157
> Author: Môshe van der Sterre <me-A/3C56C7qwM@public.gmane.org>
> Date:   Mon Feb 1 22:07:03 2016 +0000
> 
>     x86/efi/bgrt: Don't ignore the BGRT if the 'valid' bit is 0
> 
> Is there any good way to fix this again?

This was the kind of concern I had about ignoring the valid bit.  I
supported this patch based on the statement that Windows 10 ignores the
valid bit as well.  Presumably either Windows has some way of detecting
this kind of problem, or it would break on that system.  If the former,
does anyone know what algorithm Windows uses to safely check for BGRT?

I can see two possible ways to fix this.  If we can detect in advance
that the address in the BGRT looks bogus (pointing to physical memory
that doesn't exist, for instance), then we could ignore it entirely
(without any warning if valid==0, or with a warning if valid==1).  If we
can't detect that in advance, then we need to revert this patch and go
back to respecting the valid bit.  (However, trying to check the address
in advance seems preferable given that some BIOS might have a bogus
address even with valid==1.)

Re: BGRT warns again on my system

[Môshe van der Sterre]

On 05/29/2016 12:25 AM, Josh Triplett wrote:
> On Sat, May 28, 2016 at 01:46:07PM -0700, Andy Lutomirski wrote:
>> My garbage workstation splats like this on boot:
>>
>> [    0.160523] ioremap: invalid physical address d0c801800000001
>>
>> This was fixed for a while, but I think the problem was reintroduced by:
>>
>> commit 66dbe99cfe30e113d2e571e68b9b6a1a8985a157
>> Author: Môshe van der Sterre <me-A/3C56C7qwM@public.gmane.org>
>> Date:   Mon Feb 1 22:07:03 2016 +0000
>>
>>     x86/efi/bgrt: Don't ignore the BGRT if the 'valid' bit is 0
>>
>> Is there any good way to fix this again?

Is it sensible for efi-bgrt.c to use phys_addr_valid from arch/x86/mm?
That way efi-bgrt.c can choose a noise level on its own. I added a patch
to show what I mean, but is this even allowed by kernel coding
conventions?

> This was the kind of concern I had about ignoring the valid bit.  I
> supported this patch based on the statement that Windows 10 ignores the
> valid bit as well.  Presumably either Windows has some way of detecting
> this kind of problem, or it would break on that system.  If the former,
> does anyone know what algorithm Windows uses to safely check for BGRT?

I don't know the exact steps Windows takes here, but I would also like
to point to the ACPI specification. The way I read it, the valid bit
really is not any indication about having a valid physical address. It
is about having a valid screen state and a better name would perhaps
have been 'onscreen' or 'screenstatenotchanged' etc. To my eyes this is
unintuitive, but it is actually documented this way.
Quoting from the specification:

"It should be set to 1 when the table is written, and invalidated if
there is reason to expect that the screen state has been changed."

The BGRT introduction text also gives a bit more information:

"The table is written when the image is drawn on the screen. This
should be done after it is expected that any firmware components that
may write to the screen are done doing so and it is known that the image
is the only thing on the screen. If the boot path is interrupted (e.g.
by a key press), the valid bit within the status field should be changed
to 0 to indicate to the OS that the current image is invalidated."

On 2 machines I was able to test this on, this matches the behavior I
observed: A normal boot provides valid==1, but booting with a bootmenu
provides valid==0. On the third machine I tested the bit was simply
always 0, with or without bootmenu. On those 3 machines the physical
address was valid and usable in any situation.

> I can see two possible ways to fix this.  If we can detect in advance
> that the address in the BGRT looks bogus (pointing to physical memory
> that doesn't exist, for instance), then we could ignore it entirely
> (without any warning if valid==0, or with a warning if valid==1).  If we
> can't detect that in advance, then we need to revert this patch and go
> back to respecting the valid bit.  (However, trying to check the address
> in advance seems preferable given that some BIOS might have a bogus
> address even with valid==1.)

How do you interpret those words from the specification? I think the
absence of a valid physical address should give the same warning,
unrelated to the valid bit, because I don't think the bit is about that
fact at all.

---
 arch/x86/platform/efi/efi-bgrt.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
index 6a2f569..a7fdb61 100644
--- a/arch/x86/platform/efi/efi-bgrt.c
+++ b/arch/x86/platform/efi/efi-bgrt.c
@@ -19,6 +19,8 @@
 #include <linux/efi.h>
 #include <linux/efi-bgrt.h>
 
+#include "../../mm/physaddr.h"
+
 struct acpi_table_bgrt *bgrt_tab;
 void *__initdata bgrt_image;
 size_t __initdata bgrt_image_size;
@@ -67,6 +69,11 @@ void __init efi_bgrt_init(void)
 		return;
 	}
 
+	if (!phys_addr_valid(bgrt_tab->image_address)) {
+		pr_notice("Ignoring BGRT: image address is bogus\n");
+		return;
+	}
+
 	image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB);
 	if (!image) {
 		pr_notice("Ignoring BGRT: failed to map image header memory\n");
-- 
2.4.3

Re: BGRT warns again on my system

[Matt Fleming]

On Sun, 29 May, at 06:09:01AM, Môshe van der Sterre wrote:
> ---
>  arch/x86/platform/efi/efi-bgrt.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
> index 6a2f569..a7fdb61 100644
> --- a/arch/x86/platform/efi/efi-bgrt.c
> +++ b/arch/x86/platform/efi/efi-bgrt.c
> @@ -19,6 +19,8 @@
>  #include <linux/efi.h>
>  #include <linux/efi-bgrt.h>
>  
> +#include "../../mm/physaddr.h"
> +
>  struct acpi_table_bgrt *bgrt_tab;
>  void *__initdata bgrt_image;
>  size_t __initdata bgrt_image_size;
> @@ -67,6 +69,11 @@ void __init efi_bgrt_init(void)
>  		return;
>  	}
>  
> +	if (!phys_addr_valid(bgrt_tab->image_address)) {
> +		pr_notice("Ignoring BGRT: image address is bogus\n");
> +		return;
> +	}
> +
>  	image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB);
>  	if (!image) {
>  		pr_notice("Ignoring BGRT: failed to map image header memory\n");
> -- 
> 2.4.3
> 

If this does indeed fix Andy's immediate issue (and it looks like
it would) I'm happy to apply this as an interim solution.

Once we have a persistent EFI memmap available at runtime on x86 we
should attempt to see whether the physical address in ->image_address
is covered by some region in that table. The persistent EFI memmap
patches should be posted in the coming weeks.

Re: BGRT warns again on my system

[Môshe van der Sterre]

On 06/02/2016 11:24 AM, Matt Fleming wrote:
> On Sun, 29 May, at 06:09:01AM, Môshe van der Sterre wrote:
>> ---
>>   arch/x86/platform/efi/efi-bgrt.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
>> index 6a2f569..a7fdb61 100644
>> --- a/arch/x86/platform/efi/efi-bgrt.c
>> +++ b/arch/x86/platform/efi/efi-bgrt.c
>> @@ -19,6 +19,8 @@
>>   #include <linux/efi.h>
>>   #include <linux/efi-bgrt.h>
>>   
>> +#include "../../mm/physaddr.h"
>> +
>>   struct acpi_table_bgrt *bgrt_tab;
>>   void *__initdata bgrt_image;
>>   size_t __initdata bgrt_image_size;
>> @@ -67,6 +69,11 @@ void __init efi_bgrt_init(void)
>>   		return;
>>   	}
>>   
>> +	if (!phys_addr_valid(bgrt_tab->image_address)) {
>> +		pr_notice("Ignoring BGRT: image address is bogus\n");
>> +		return;
>> +	}
>> +
>>   	image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB);
>>   	if (!image) {
>>   		pr_notice("Ignoring BGRT: failed to map image header memory\n");
>> -- 
>> 2.4.3
>>
> If this does indeed fix Andy's immediate issue (and it looks like
> it would) I'm happy to apply this as an interim solution.
I have tested that this does indeed fix the warning for Andy's value of 
->image_address, but off course I can't test this with the actual 
hardware that Andy has. Do you want me to resend this with a commit 
message so it can be applied?
> Once we have a persistent EFI memmap available at runtime on x86 we
> should attempt to see whether the physical address in ->image_address
> is covered by some region in that table. The persistent EFI memmap
> patches should be posted in the coming weeks.

Re: BGRT warns again on my system

[Josh Triplett]

On Sun, Jun 19, 2016 at 03:14:40PM +0200, Môshe van der Sterre wrote:
> On 06/02/2016 11:24 AM, Matt Fleming wrote:
> > On Sun, 29 May, at 06:09:01AM, Môshe van der Sterre wrote:
> > > ---
> > >   arch/x86/platform/efi/efi-bgrt.c | 7 +++++++
> > >   1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
> > > index 6a2f569..a7fdb61 100644
> > > --- a/arch/x86/platform/efi/efi-bgrt.c
> > > +++ b/arch/x86/platform/efi/efi-bgrt.c
> > > @@ -19,6 +19,8 @@
> > >   #include <linux/efi.h>
> > >   #include <linux/efi-bgrt.h>
> > > +#include "../../mm/physaddr.h"
> > > +
> > >   struct acpi_table_bgrt *bgrt_tab;
> > >   void *__initdata bgrt_image;
> > >   size_t __initdata bgrt_image_size;
> > > @@ -67,6 +69,11 @@ void __init efi_bgrt_init(void)
> > >   		return;
> > >   	}
> > > +	if (!phys_addr_valid(bgrt_tab->image_address)) {
> > > +		pr_notice("Ignoring BGRT: image address is bogus\n");
> > > +		return;
> > > +	}
> > > +
> > >   	image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB);
> > >   	if (!image) {
> > >   		pr_notice("Ignoring BGRT: failed to map image header memory\n");
> > > -- 
> > > 2.4.3
> > > 
> > If this does indeed fix Andy's immediate issue (and it looks like
> > it would) I'm happy to apply this as an interim solution.
> I have tested that this does indeed fix the warning for Andy's value of
> ->image_address, but off course I can't test this with the actual hardware
> that Andy has. Do you want me to resend this with a commit message so it can
> be applied?

With a commit message and a clearer pr_notice than "is bogus", yes. How
about "image address not a valid physical memory address"?

Re: BGRT warns again on my system

[Môshe van der Sterre]

On 06/19/2016 04:38 PM, Josh Triplett wrote:
> On Sun, Jun 19, 2016 at 03:14:40PM +0200, Môshe van der Sterre wrote:
>> On 06/02/2016 11:24 AM, Matt Fleming wrote:
>>
>>> If this does indeed fix Andy's immediate issue (and it looks like
>>> it would) I'm happy to apply this as an interim solution.
>> I have tested that this does indeed fix the warning for Andy's value of
>> ->image_address, but off course I can't test this with the actual hardware
>> that Andy has. Do you want me to resend this with a commit message so it can
>> be applied?
> With a commit message and a clearer pr_notice than "is bogus", yes. How
> about "image address not a valid physical memory address"?

The sub-par notice text crossed my mind when I originally mailed it, but 
I forgot about it today. I like your suggestion so I'll use that one.

Re: BGRT warns again on my system

[Dave Young]

On 06/02/16 at 10:24am, Matt Fleming wrote:
> On Sun, 29 May, at 06:09:01AM, Môshe van der Sterre wrote:
> > ---
> >  arch/x86/platform/efi/efi-bgrt.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
> > index 6a2f569..a7fdb61 100644
> > --- a/arch/x86/platform/efi/efi-bgrt.c
> > +++ b/arch/x86/platform/efi/efi-bgrt.c
> > @@ -19,6 +19,8 @@
> >  #include <linux/efi.h>
> >  #include <linux/efi-bgrt.h>
> >  
> > +#include "../../mm/physaddr.h"
> > +
> >  struct acpi_table_bgrt *bgrt_tab;
> >  void *__initdata bgrt_image;
> >  size_t __initdata bgrt_image_size;
> > @@ -67,6 +69,11 @@ void __init efi_bgrt_init(void)
> >  		return;
> >  	}
> >  
> > +	if (!phys_addr_valid(bgrt_tab->image_address)) {
> > +		pr_notice("Ignoring BGRT: image address is bogus\n");
> > +		return;
> > +	}
> > +
> >  	image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB);
> >  	if (!image) {
> >  		pr_notice("Ignoring BGRT: failed to map image header memory\n");
> > -- 
> > 2.4.3
> > 
> 
> If this does indeed fix Andy's immediate issue (and it looks like
> it would) I'm happy to apply this as an interim solution.
> 
> Once we have a persistent EFI memmap available at runtime on x86 we
> should attempt to see whether the physical address in ->image_address
> is covered by some region in that table. The persistent EFI memmap
> patches should be posted in the coming weeks.

I may missed the background but I worry about who knows if it is meant
to be used for BGRT image. Some vendors could respect the BGRT valid
bit and just leave the address as a random address. And the random
address could be a valid physical address though. It is possible to
leak memory infomation. 

Could we reconsider about below commit? It may be a Windows 10 bug?

commit 66dbe99cfe30e113d2e571e68b9b6a1a8985a157
Author: Môshe van der Sterre <me-A/3C56C7qwM@public.gmane.org>
Date:   Mon Feb 1 22:07:03 2016 +0000

    x86/efi/bgrt: Don't ignore the BGRT if the 'valid' bit is 0

Thanks
Dave

Re: BGRT warns again on my system

[Josh Triplett]

On Mon, Jun 20, 2016 at 01:38:19PM +0800, Dave Young wrote:
> On 06/02/16 at 10:24am, Matt Fleming wrote:
> > On Sun, 29 May, at 06:09:01AM, Môshe van der Sterre wrote:
> > > ---
> > >  arch/x86/platform/efi/efi-bgrt.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
> > > index 6a2f569..a7fdb61 100644
> > > --- a/arch/x86/platform/efi/efi-bgrt.c
> > > +++ b/arch/x86/platform/efi/efi-bgrt.c
> > > @@ -19,6 +19,8 @@
> > >  #include <linux/efi.h>
> > >  #include <linux/efi-bgrt.h>
> > >  
> > > +#include "../../mm/physaddr.h"
> > > +
> > >  struct acpi_table_bgrt *bgrt_tab;
> > >  void *__initdata bgrt_image;
> > >  size_t __initdata bgrt_image_size;
> > > @@ -67,6 +69,11 @@ void __init efi_bgrt_init(void)
> > >  		return;
> > >  	}
> > >  
> > > +	if (!phys_addr_valid(bgrt_tab->image_address)) {
> > > +		pr_notice("Ignoring BGRT: image address is bogus\n");
> > > +		return;
> > > +	}
> > > +
> > >  	image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB);
> > >  	if (!image) {
> > >  		pr_notice("Ignoring BGRT: failed to map image header memory\n");
> > > -- 
> > > 2.4.3
> > > 
> > 
> > If this does indeed fix Andy's immediate issue (and it looks like
> > it would) I'm happy to apply this as an interim solution.
> > 
> > Once we have a persistent EFI memmap available at runtime on x86 we
> > should attempt to see whether the physical address in ->image_address
> > is covered by some region in that table. The persistent EFI memmap
> > patches should be posted in the coming weeks.
> 
> I may missed the background but I worry about who knows if it is meant
> to be used for BGRT image. Some vendors could respect the BGRT valid
> bit and just leave the address as a random address. And the random
> address could be a valid physical address though. It is possible to
> leak memory infomation. 

Not unless the random address they point to happens to work as a valid
BMP image.  And even then, we figure all this out and copy the BGRT out
of that memory before we actually create the non-early memory map.

> Could we reconsider about below commit? It may be a Windows 10 bug?
> 
> commit 66dbe99cfe30e113d2e571e68b9b6a1a8985a157
> Author: Môshe van der Sterre <me-A/3C56C7qwM@public.gmane.org>
> Date:   Mon Feb 1 22:07:03 2016 +0000
> 
>     x86/efi/bgrt: Don't ignore the BGRT if the 'valid' bit is 0

I'd certainly welcome further investigation there. But if it is a bug,
it's one that BIOSes take advantage of.

Re: BGRT warns again on my system

[Matt Fleming]

On Mon, 20 Jun, at 01:38:19PM, Dave Young wrote:
> 
> I may missed the background but I worry about who knows if it is meant
> to be used for BGRT image. Some vendors could respect the BGRT valid
> bit and just leave the address as a random address. And the random
> address could be a valid physical address though. It is possible to
> leak memory infomation. 
> 
> Could we reconsider about below commit? It may be a Windows 10 bug?

Valid concerns, but we can only go on anecdotal evidence from the
behaviour of the machines we see.

Even if it is a bug in Windows, if the firmware developers are writing
code that expects the bug, and users see pretty BIOS screens with
Windows but not in Linux, we usually try to be bug-compatible.

All of that is a long way of saying: It doesn't matter if it's a bug
or not, we should provide the same behaviour.