| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5720D365.5080601@moshe.nl>
Date: Wed, 27 Apr 2016 16:57:41 +0200
From: Môshe van der Sterre <me@...he.nl>
To: Josh Boyer <jwboyer@...oraproject.org>
Cc: Matt Fleming <matt@...eblueprint.co.uk>,
"linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
"Linux-Kernel@...r. Kernel. Org" <linux-kernel@...r.kernel.org>,
Josh Triplett <josh@...htriplett.org>
Subject: Re: [PATCH] x86/efi-bgrt: Switch all pr_err() to pr_debug() for
invalid BGRT
On 04/27/2016 03:56 PM, Josh Boyer wrote:
> On Wed, Apr 27, 2016 at 9:26 AM, Môshe van der Sterre <me@...he.nl> wrote:
>> (additionally CC-ing Josh Triplett)
> Thanks for doing so. I completely forgot.
>
>> On 04/27/2016 02:50 PM, Josh Boyer wrote:
>>> The promise of pretty boot splashes from firmware via BGRT was at
>>> best only that; a promise. The kernel diligently checks to make
>>> sure the BGRT data firmware gives it is valid, and dutifully warns
>>> the user when it isn't. However, it does so via the pr_err log
>>> level which seems unnecessary. The user cannot do anything about
>>> this and there really isn't an error on the part of Linux to
>>> correct.
>>>
>>> This lowers the log level by using pr_debug instead. Users will
>>> no longer have their boot process uglified by the kernel reminding
>>> us that firmware can and often is broken. Ironic, considering
>>> BGRT is supposed to make boot pretty to begin with.
>> Hi Josh Boyer,
>>
>> Are you seeing these errors somewhere? I recently fixed the error "Ignoring
> We have a user that reports seeing:
>
> "Ignoring BGRT: Invalid version 0 (expected 1)"
>
> on a Lenovo T430 machine. We've had a few other scattered reports on
> various machine types since BGRT went into the kernel as well.
Ok. With this information, I think pr_debug is indeed better.
>> BGRT: invalid status 0 (expected 1)" because Linux apparently interpreted
>> that part of the specification differently than others.
>> If that's the error you are seeing, perhaps your problem is already solved
>> in recent kernels? (fixed in commit 66dbe99)
>>
>> Personally I agree that BGRT messages should not annoy actual users of
>> production firmwares.
>> However I also agree with the previous consensus that these checks (for
>> actual spec violations) should remain pr_err unless some production firmware
>> is triggering them. What do you think?
> Production firmware is literally the only firmware end users will ever
> see. I don't see much point in leaving scary error messages in the
> kernel to complain about things the user has no chance of fixing or in
> almost all cases even reporting to people who could fix it.
In principle I can understand the wish to show big scary error messages
to firmware developers doing it wrong.
With that said:
The patch looks good to me, but Josh Triplett and Matt Fleming their
opinions might be better informed than mine.
> josh
>
>>> Signed-off-by: Josh Boyer <jwboyer@...oraproject.org>
>>> ---
>>> arch/x86/platform/efi/efi-bgrt.c | 18 +++++++++---------
>>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/arch/x86/platform/efi/efi-bgrt.c
>>> b/arch/x86/platform/efi/efi-bgrt.c
>>> index a2433817c987..6f70d2ac8029 100644
>>> --- a/arch/x86/platform/efi/efi-bgrt.c
>>> +++ b/arch/x86/platform/efi/efi-bgrt.c
>>> @@ -43,40 +43,40 @@ void __init efi_bgrt_init(void)
>>> return;
>>> if (bgrt_tab->header.length < sizeof(*bgrt_tab)) {
>>> - pr_err("Ignoring BGRT: invalid length %u (expected
>>> %zu)\n",
>>> + pr_debug("Ignoring BGRT: invalid length %u (expected
>>> %zu)\n",
>>> bgrt_tab->header.length, sizeof(*bgrt_tab));
>>> return;
>>> }
>>> if (bgrt_tab->version != 1) {
>>> - pr_err("Ignoring BGRT: invalid version %u (expected 1)\n",
>>> + pr_debug("Ignoring BGRT: invalid version %u (expected
>>> 1)\n",
>>> bgrt_tab->version);
>>> return;
>>> }
>>> if (bgrt_tab->status & 0xfe) {
>>> - pr_err("Ignoring BGRT: reserved status bits are non-zero
>>> %u\n",
>>> + pr_debug("Ignoring BGRT: reserved status bits are non-zero
>>> %u\n",
>>> bgrt_tab->status);
>>> return;
>>> }
>>> if (bgrt_tab->image_type != 0) {
>>> - pr_err("Ignoring BGRT: invalid image type %u (expected
>>> 0)\n",
>>> + pr_debug("Ignoring BGRT: invalid image type %u (expected
>>> 0)\n",
>>> bgrt_tab->image_type);
>>> return;
>>> }
>>> if (!bgrt_tab->image_address) {
>>> - pr_err("Ignoring BGRT: null image address\n");
>>> + pr_debug("Ignoring BGRT: null image address\n");
>>> return;
>>> }
>>> image = memremap(bgrt_tab->image_address, sizeof(bmp_header),
>>> MEMREMAP_WB);
>>> if (!image) {
>>> - pr_err("Ignoring BGRT: failed to map image header
>>> memory\n");
>>> + pr_debug("Ignoring BGRT: failed to map image header
>>> memory\n");
>>> return;
>>> }
>>> 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",
>>> + pr_debug("Ignoring BGRT: Incorrect BMP magic number 0x%x
>>> (expected 0x4d42)\n",
>>> bmp_header.id);
>>> return;
>>> }
>>> @@ -84,14 +84,14 @@ void __init efi_bgrt_init(void)
>>> bgrt_image = kmalloc(bgrt_image_size, GFP_KERNEL | __GFP_NOWARN);
>>> if (!bgrt_image) {
>>> - pr_err("Ignoring BGRT: failed to allocate memory for image
>>> (wanted %zu bytes)\n",
>>> + pr_debug("Ignoring BGRT: failed to allocate memory for
>>> image (wanted %zu bytes)\n",
>>> bgrt_image_size);
>>> return;
>>> }
>>> image = memremap(bgrt_tab->image_address, bmp_header.size,
>>> MEMREMAP_WB);
>>> if (!image) {
>>> - pr_err("Ignoring BGRT: failed to map image memory\n");
>>> + pr_debug("Ignoring BGRT: failed to map image memory\n");
>>> kfree(bgrt_image);
>>> bgrt_image = NULL;
>>> return;
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists