Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <9f694b7e-4cd1-9682-76bd-d534b8b2a485@gmx.de>
Date:   Thu, 7 Apr 2022 17:50:35 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: =?UTF-8?Q?Re=3a_=5bBUG=5d_fbdev=3a_i740fb=3a_Divide_error_when_?=
 =?UTF-8?B?4oCYdmFyLT5waXhjbG9ja+KAmSBpcyB6ZXJv?=
Content-Language: en-US
To:     Zheyu Ma <zheyuma97@gmail.com>
Cc:     Ondrej Zary <linux@zary.sk>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        Linux Fbdev development list <linux-fbdev@vger.kernel.org>,
        DRI Development <dri-devel@lists.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <CAMhUBjmFhqTLBscHHVZ1VTSqrJBT1VEevA+KkjY+y9_ZtdRkMg@mail.gmail.com>
 <CAMuHMdUiEo8q9x0C0x5zOM=ax1=S06=s0JjcJvZYD4aMGLmEaQ@mail.gmail.com>
 <a564f6af-31fa-79a2-72c3-578f2c095b23@gmx.de>
 <202204051946.43277.linux@zary.sk>
 <527a8c23-609f-5f8a-e076-a8a59da59865@gmx.de>
 <CAMhUBjmBm3=CY=cCZuH0+ZeemNVT=9XywSoYiR7WLcYOUGu9VQ@mail.gmail.com>
From:   Helge Deller <deller@gmx.de>
In-Reply-To: <CAMhUBjmBm3=CY=cCZuH0+ZeemNVT=9XywSoYiR7WLcYOUGu9VQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On 4/6/22 03:24, Zheyu Ma wrote:
> On Wed, Apr 6, 2022 at 2:23 AM Helge Deller <deller@gmx.de> wrote:
>>
>> 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@gmx.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 valu=
e to
>>>>>>> 'var->pixclock', the driver will throw a divide error.
>>>>>>>
>>>>>>> This bug occurs because the driver uses the value of 'var->pixcloc=
k'
>>>>>>> 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 low=
est
>>>>>>> supported value when this field is zero.
>>>>>>> I have no idea about which solution is better.
>>>>>>
>>>>>> Me neither.
>>>>>> I think a solution like commit b36b242d4b8ea178f7fd038965e3cac7f30c=
3f09
>>>>>> 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 b36b242d4b8ea178f7fd038965e3cac=
7f30c3f09 ?
>>>>>
>>>>> 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?
>
> I have submitted patches for the i740fb driver and other drivers which
> have similar bugs as follows:
> https://lore.kernel.org/all/20220404084723.79089-1-zheyuma97@gmail.com/

Yes, I know.
But Ondrej offered to test a patch which would round an invalid pixclock u=
p
instead of just returning EINVAL (which is what your patch does).
So, if someone comes up with such a patch it'd be the preferred solution.

Helge