linux-kernel - Re: [PATCH] firmware/sysfb: Fix wrong stride when bits-per-pixel is calculated

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: <ae5f6784c72e1b31cdf7766d3c6dbe0c.squirrel@mail.panix.com>
Date:   Thu, 13 Apr 2023 12:24:12 -0400
From:   "Pierre Asselin" <pa@...ix.com>
To:     "Javier Martinez Canillas" <javierm@...hat.com>
Cc:     "Pierre Asselin" <pa@...ix.com>, tzimmermann@...e.de,
        linux-kernel@...r.kernel.org, jfalempe@...hat.com,
        hdegoede@...hat.com, dri-devel@...ts.freedesktop.org,
        daniel.vetter@...ll.ch, ardb@...nel.org
Subject: Re: [PATCH] firmware/sysfb: Fix wrong stride when bits-per-pixel is
 calculated

> pa@...ix.com (Pierre Asselin) writes:
>
>> After careful reading of the comments in f35cd3fa7729, would this
>> be an appropriate fix ?  Does it still address all the issues raised
>> by Thomas ?
>>
>> (not tested)
>>
>> diff --git a/drivers/firmware/sysfb_simplefb.c
>> b/drivers/firmware/sysfb_simplefb.c
>> index 82c64cb9f531..9f5299d54732 100644
>> --- a/drivers/firmware/sysfb_simplefb.c
>> +++ b/drivers/firmware/sysfb_simplefb.c
>> @@ -56,10 +56,11 @@ __init bool sysfb_parse_mode(const struct
>> screen_info *si,
>>  	 * don't specify alpha channels.
>>  	 */
>>  	if (si->lfb_depth > 8) {
>> -		bits_per_pixel = max(max3(si->red_size + si->red_pos,
>> +		bits_per_pixel = max3(max3(si->red_size + si->red_pos,
>>  					  si->green_size + si->green_pos,
>>  					  si->blue_size + si->blue_pos),
>> -				     si->rsvd_size + si->rsvd_pos);
>> +				     si->rsvd_size + si->rsvd_pos,
>> +				     si->lfb_depth);


> I would defer to Thomas but personally I don't like it. Seems to me that
> this is getting too complicated just to workaround buggy BIOS that are not
> reporting consistent information about their firmware-provided
> framebuffer.

Okay, but remember, this is a regression.  The buggy BIOSes were there
the whole time and the old code that matched f->bits_per_pixel against
si->lfb_depth used to work against these buggy BIOSes.

And is it a bug, or is it an underspecified standard ?  "These bits aren't
reserved, we just ignore them" or some such.

My reading of Thomas' comments in the code was that sometimes lfb_depth
was reported too small but never too large ?  But I'm not sure.  It's
true in my two cases.

> I would either trust the pixel channel information (what Thomas patch did)
> + my patch to calculate the stride (since we can't trust the line lenght
> which is based on the reported depth) + a DMI table for broken machines.

> But that will only work if your systems are the exception and not a common
> issue, otherwise then Thomas' approach won't work if there are too many
> buggy BIOS out there.

The laptop is ancient but the Dell tower is maybe 4 years old.  The
BIOS is 09/11/2019 according to dmidecode, and the most recent for
this box.

> Another option is to do the opposite, not calculate a BPP using the pixel
> and then use that value to calculate a new stride, but instead assume that
> the lfb_linelength is correct and use that to calculate the BPP.

Or reject (some) inconsistencies in the struct screen_info and return
false, so the kernel falls back to e.g. vesa ?

> Something like the following patch, which should also fix your regression
> and may be enough to address Thomas' concerns of inconsistent depths too?

I'll try that patch.