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

["Môshe van der Sterre" <me-A/3C56C7qwM@public.gmane.org>]

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