linux-kernel - Re: [BUG] fbdev: i740fb: Divide error when ‘var->pixclock’ is zero

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: <527a8c23-609f-5f8a-e076-a8a59da59865@gmx.de>
Date:   Tue, 5 Apr 2022 20:23:52 +0200
From:   Helge Deller <deller@....de>
To:     Ondrej Zary <linux@...y.sk>
Cc:     Geert Uytterhoeven <geert@...ux-m68k.org>,
        Zheyu Ma <zheyuma97@...il.com>,
        Linux Fbdev development list <linux-fbdev@...r.kernel.org>,
        DRI Development <dri-devel@...ts.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [BUG] fbdev: i740fb: Divide error when ‘var->pixclock’ is zero

On 4/5/22 19:46, Ondrej Zary wrote:
> On Tuesday 05 April 2022 08:33:57 Helge Deller wrote:
>> Hello Geert,
>>
>> On 4/4/22 13:46, Geert Uytterhoeven wrote:
>>> Hi Helge,
>>>
>>> On Sun, Apr 3, 2022 at 5:41 PM Helge Deller <deller@....de> wrote:
>>>> On 4/3/22 13:26, Zheyu Ma wrote:
>>>>> I found a bug in the function i740fb_set_par().
>>>>
>>>> Nice catch!
>>>>
>>>>> When the user calls the ioctl system call without setting the value to
>>>>> 'var->pixclock', the driver will throw a divide error.
>>>>>
>>>>> This bug occurs because the driver uses the value of 'var->pixclock'
>>>>> without checking it, as the following code snippet show:
>>>>>
>>>>> if ((1000000 / var->pixclock) > DACSPEED8) {
>>>>>      dev_err(info->device, "requested pixclock %i MHz out of range
>>>>> (max. %i MHz at 8bpp)\n",
>>>>>          1000000 / var->pixclock, DACSPEED8);
>>>>>     return -EINVAL;x
>>>>> }
>>>>>
>>>>> We can fix this by checking the value of 'var->pixclock' in the
>>>>> function i740fb_check_var() similar to commit
>>>>> b36b242d4b8ea178f7fd038965e3cac7f30c3f09, or we should set the lowest
>>>>> supported value when this field is zero.
>>>>> I have no idea about which solution is better.
>>>>
>>>> Me neither.
>>>> I think a solution like commit b36b242d4b8ea178f7fd038965e3cac7f30c3f09
>>>> is sufficient.
>>>>
>>>> Note that i740fb_set_par() is called in i740fb_resume() as well.
>>>> Since this doesn't comes form userspace I think adding a check for
>>>> the return value there isn't necessary.
>>>>
>>>> Would you mind sending a patch like b36b242d4b8ea178f7fd038965e3cac7f30c3f09 ?
>>>
>>> When passed an invalid value, .check_var() is supposed to
>>> round up the invalid to a valid value, if possible.
>>
>> I don't disagree.
>> The main problem probably is: what is the next valid value?
>> This needs to be analyzed on a per-driver base and ideally tested.
>> Right now a division-by-zero is tiggered which is probably more worse.
>
> I still have an i740 card so I can test it.

Good. Someone wants to come up with a proposed patch?

Helge