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