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: <87jzyfrba7.fsf@minerva.mail-host-address-is-not-set>
Date:   Thu, 13 Apr 2023 18:34:40 +0200
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Pierre Asselin <pa@...ix.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

"Pierre Asselin" <pa@...ix.com> writes:

>> pa@...ix.com (Pierre Asselin) writes:

[...]

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

Yes, I agree that is a regression and has to be fixed. I'm just arguing
against this particular fix.

> 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 (mis?)understood that it could be too small as well. But that's why I
prefer Thomas to chime in before agreeing on any path forward. But he is
in vacation this week I believe.

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

I see.

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

That a reasonable approach too. But better if we can make simpledrm work
too since many distros have dropped all the fbdev drivers in favor of it.

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

Thanks for all your information and testing with this bug!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat