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

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

[Môshe van der Sterre]

Unintuitively, the BGRT graphic is apparently meant to be usable if
the valid bit in not set. The valid bit only conveys uncertainty
about the validity in relation to the screen state.

Windows 10 actually uses the BGRT image for its boot screen even if
not 'valid', for example when the user triggered the boot menu.
Because it is unclear if all firmwares will provide a usable graphic
in this case, we now look at the BMP magic number as an additional
check.
---
 arch/x86/platform/efi/efi-bgrt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
index b097066..a243381 100644
--- a/arch/x86/platform/efi/efi-bgrt.c
+++ b/arch/x86/platform/efi/efi-bgrt.c
@@ -57,11 +57,6 @@ void __init efi_bgrt_init(void)
 		       bgrt_tab->status);
 		return;
 	}
-	if (bgrt_tab->status != 1) {
-		pr_debug("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);
@@ -80,6 +75,11 @@ void __init efi_bgrt_init(void)
 
 	memcpy(&bmp_header, image, sizeof(bmp_header));
 	memunmap(image);
+	if (bmp_header.id != 0x4d42) {
+		pr_err("Ignoring BGRT: Incorrect BMP magic number 0x%x (expected 0x4d42)\n",
+			bmp_header.id);
+		return;
+	}
 	bgrt_image_size = bmp_header.size;
 
 	bgrt_image = kmalloc(bgrt_image_size, GFP_KERNEL | __GFP_NOWARN);
-- 
2.5.0

Re: [PATCH] x86/efi-bgrt: Don't ignore the BGRT if the 'valid' bit is 0

[Matt Fleming]

On Sun, 20 Dec, at 10:53:11PM, Môshe van der Sterre wrote:
> Unintuitively, the BGRT graphic is apparently meant to be usable if
> the valid bit in not set. The valid bit only conveys uncertainty
> about the validity in relation to the screen state.
> 
> Windows 10 actually uses the BGRT image for its boot screen even if
> not 'valid', for example when the user triggered the boot menu.
> Because it is unclear if all firmwares will provide a usable graphic
> in this case, we now look at the BMP magic number as an additional
> check.
> ---
>  arch/x86/platform/efi/efi-bgrt.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
> index b097066..a243381 100644
> --- a/arch/x86/platform/efi/efi-bgrt.c
> +++ b/arch/x86/platform/efi/efi-bgrt.c
> @@ -57,11 +57,6 @@ void __init efi_bgrt_init(void)
>  		       bgrt_tab->status);
>  		return;
>  	}
> -	if (bgrt_tab->status != 1) {
> -		pr_debug("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);
> @@ -80,6 +75,11 @@ void __init efi_bgrt_init(void)
>  
>  	memcpy(&bmp_header, image, sizeof(bmp_header));
>  	memunmap(image);
> +	if (bmp_header.id != 0x4d42) {
> +		pr_err("Ignoring BGRT: Incorrect BMP magic number 0x%x (expected 0x4d42)\n",
> +			bmp_header.id);
> +		return;
> +	}
>  	bgrt_image_size = bmp_header.size;
>  
>  	bgrt_image = kmalloc(bgrt_image_size, GFP_KERNEL | __GFP_NOWARN);


Shouldn't this be pr_debug() instead of pr_err()?

Re: [PATCH] x86/efi-bgrt: Don't ignore the BGRT if the 'valid' bit is 0

[Môshe van der Sterre]

On 01/05/2016 04:46 PM, Matt Fleming wrote:
> On Sun, 20 Dec, at 10:53:11PM, Môshe van der Sterre wrote:
>> Unintuitively, the BGRT graphic is apparently meant to be usable if
>> the valid bit in not set. The valid bit only conveys uncertainty
>> about the validity in relation to the screen state.
>>
>> Windows 10 actually uses the BGRT image for its boot screen even if
>> not 'valid', for example when the user triggered the boot menu.
>> Because it is unclear if all firmwares will provide a usable graphic
>> in this case, we now look at the BMP magic number as an additional
>> check.
>> ---
>>   arch/x86/platform/efi/efi-bgrt.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
>> index b097066..a243381 100644
>> --- a/arch/x86/platform/efi/efi-bgrt.c
>> +++ b/arch/x86/platform/efi/efi-bgrt.c
>> @@ -57,11 +57,6 @@ void __init efi_bgrt_init(void)
>>   		       bgrt_tab->status);
>>   		return;
>>   	}
>> -	if (bgrt_tab->status != 1) {
>> -		pr_debug("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);
>> @@ -80,6 +75,11 @@ void __init efi_bgrt_init(void)
>>   
>>   	memcpy(&bmp_header, image, sizeof(bmp_header));
>>   	memunmap(image);
>> +	if (bmp_header.id != 0x4d42) {
>> +		pr_err("Ignoring BGRT: Incorrect BMP magic number 0x%x (expected 0x4d42)\n",
>> +			bmp_header.id);
>> +		return;
>> +	}
>>   	bgrt_image_size = bmp_header.size;
>>   
>>   	bgrt_image = kmalloc(bgrt_image_size, GFP_KERNEL | __GFP_NOWARN);
>
> Shouldn't this be pr_debug() instead of pr_err()?

It is an actual error for the image to be anything else. I did look at 
your commit "x86/efi-bgrt: Switch pr_err() to pr_debug() for invalid 
BGRT", and actually intentionally chose pr_err() instead of pr_debug() 
because of it.

 From that commit message:
     However, Josh points that out it still makes sense to test the
     validity of the upper 7 bits of the 'status' field, since
     they're marked as "reserved" in the spec and must be zero. If
     firmware violates this it really *is* an error.

 From the ACPI spec:
     If the value is 0, the Image Type is Bitmap. The format for a 
Bitmap is defined at the reference located in the ACPI Link Document 
under the heading "Types of Bitmaps". All other values not defined in 
the table are reserved for future use.
and also:
     Implementations must present the image in a 24 bit bitmap with 
pixel format 0xRRGGBB, or a 32-bit bitmap with the pixel format 
0xrrRRGGBB, where ‘rr’ is reserved.
Following the link to MSDN, the header id is definitely also required to 
be "BM" for those types,


That said, I'm not 100% sure if pr_err() is right for any of the errors 
in efi-bgrt.c. I don't really feel knowledgeable enough to suggest 
anything about that, so I tried to follow the previous discussion 
outcome as closely as possible.

By the way... Now I'm thinking about the ID, what about endianness? This 
code is under "arch/x86", does this always imply a little-endian 
architecture? The new check will probably fail on big-endian but that 
should never happen, right?

Greetings,
Môshe van der Sterre

Re: [PATCH] x86/efi-bgrt: Don't ignore the BGRT if the 'valid' bit is 0

[Matt Fleming]

On Tue, 05 Jan, at 05:38:54PM, Môshe van der Sterre wrote:
> On 01/05/2016 04:46 PM, Matt Fleming wrote:
> >Shouldn't this be pr_debug() instead of pr_err()?
> 
> It is an actual error for the image to be anything else. I did look at your
> commit "x86/efi-bgrt: Switch pr_err() to pr_debug() for invalid BGRT", and
> actually intentionally chose pr_err() instead of pr_debug() because of it.
> 
> From that commit message:
>     However, Josh points that out it still makes sense to test the
>     validity of the upper 7 bits of the 'status' field, since
>     they're marked as "reserved" in the spec and must be zero. If
>     firmware violates this it really *is* an error.
> 
> From the ACPI spec:
>     If the value is 0, the Image Type is Bitmap. The format for a Bitmap is
> defined at the reference located in the ACPI Link Document under the heading
> "Types of Bitmaps". All other values not defined in the table are reserved
> for future use.
> and also:
>     Implementations must present the image in a 24 bit bitmap with pixel
> format 0xRRGGBB, or a 32-bit bitmap with the pixel format 0xrrRRGGBB, where
> ‘rr’ is reserved.
> Following the link to MSDN, the header id is definitely also required to be
> "BM" for those types,
> That said, I'm not 100% sure if pr_err() is right for any of the errors in

OK, I'll queue this up. Josh, does this look OK to you?

> efi-bgrt.c. I don't really feel knowledgeable enough to suggest anything
> about that, so I tried to follow the previous discussion outcome as closely
> as possible.
> 
> By the way... Now I'm thinking about the ID, what about endianness? This
> code is under "arch/x86", does this always imply a little-endian
> architecture? The new check will probably fail on big-endian but that should
> never happen, right?

Yes, we can ignore big-endian.

Re: [PATCH] x86/efi-bgrt: Don't ignore the BGRT if the 'valid' bit is 0

[Matt Fleming]

On Fri, 08 Jan, at 11:30:47AM, Matt Fleming wrote:
> 
> OK, I'll queue this up. Josh, does this look OK to you?

Actually Môshe, could you resend the patch with the correct
Signed-off-by line?

Re: [PATCH] x86/efi-bgrt: Don't ignore the BGRT if the 'valid' bit is 0

[Josh Triplett]

On Fri, Jan 08, 2016 at 11:30:47AM +0000, Matt Fleming wrote:
> On Tue, 05 Jan, at 05:38:54PM, Môshe van der Sterre wrote:
> > On 01/05/2016 04:46 PM, Matt Fleming wrote:
> > >Shouldn't this be pr_debug() instead of pr_err()?
> > 
> > It is an actual error for the image to be anything else. I did look at your
> > commit "x86/efi-bgrt: Switch pr_err() to pr_debug() for invalid BGRT", and
> > actually intentionally chose pr_err() instead of pr_debug() because of it.
> > 
> > From that commit message:
> >     However, Josh points that out it still makes sense to test the
> >     validity of the upper 7 bits of the 'status' field, since
> >     they're marked as "reserved" in the spec and must be zero. If
> >     firmware violates this it really *is* an error.
> > 
> > From the ACPI spec:
> >     If the value is 0, the Image Type is Bitmap. The format for a Bitmap is
> > defined at the reference located in the ACPI Link Document under the heading
> > "Types of Bitmaps". All other values not defined in the table are reserved
> > for future use.
> > and also:
> >     Implementations must present the image in a 24 bit bitmap with pixel
> > format 0xRRGGBB, or a 32-bit bitmap with the pixel format 0xrrRRGGBB, where
> > ‘rr’ is reserved.
> > Following the link to MSDN, the header id is definitely also required to be
> > "BM" for those types,
> > That said, I'm not 100% sure if pr_err() is right for any of the errors in
> 
> OK, I'll queue this up. Josh, does this look OK to you?

Yes, checking the bitmap file header seems like a good additional
cross-check.  And since the spec says that the image must be a bitmap
(given the one possible value for the format field), and since we can't
reliably know *how much* data to read without reading the bitmap header,
we should indeed error out if the data doesn't look like a bitmap.

Reviewed-by: Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org>

Re: [PATCH] x86/efi-bgrt: Don't ignore the BGRT if the 'valid' bit is 0

[Matt Fleming]

On Fri, 08 Jan, at 11:42:35AM, Matt Fleming wrote:
> On Fri, 08 Jan, at 11:30:47AM, Matt Fleming wrote:
> > 
> > OK, I'll queue this up. Josh, does this look OK to you?
> 
> Actually Môshe, could you resend the patch with the correct
> Signed-off-by line?

Ping?

Re: [PATCH] x86/efi-bgrt: Don't ignore the BGRT if the 'valid' bit is 0

[Môshe van der Sterre]

On 01/14/2016 01:29 PM, Matt Fleming wrote:
> On Fri, 08 Jan, at 11:42:35AM, Matt Fleming wrote:
>> On Fri, 08 Jan, at 11:30:47AM, Matt Fleming wrote:
>>> OK, I'll queue this up. Josh, does this look OK to you?
>> Actually Môshe, could you resend the patch with the correct
>> Signed-off-by line?
> Ping?
Sorry for the late reply, I'll try to send it later today.

Re: [PATCH] x86/efi-bgrt: Don't ignore the BGRT if the 'valid' bit is 0

[Matt Fleming]

On Thu, 14 Jan, at 11:05:07PM, Môshe van der Sterre wrote:
> Unintuitively, the BGRT graphic is apparently meant to be usable if
> the valid bit in not set. The valid bit only conveys uncertainty
> about the validity in relation to the screen state.
> 
> Windows 10 actually uses the BGRT image for its boot screen even if
> not 'valid', for example when the user triggered the boot menu.
> Because it is unclear if all firmwares will provide a usable graphic
> in this case, we now look at the BMP magic number as an additional
> check.
> 
> Reviewed-by: Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org>
> Signed-off-by: Môshe van der Sterre <me-A/3C56C7qwM@public.gmane.org>
> ---
>  arch/x86/platform/efi/efi-bgrt.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Thanks, applied!