linux-kernel - Re: [PATCH] x86/efi-bgrt: Switch all pr_err() to pr

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5720BE0B.8080605@moshe.nl>
Date:	Wed, 27 Apr 2016 15:26:35 +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-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

(additionally CC-ing Josh Triplett)

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 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?

Greetings,
Môshe van der Sterre

> 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;