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

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

[Zheyu Ma]

Hi,

I found a bug in the function i740fb_set_par().

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.

The following log reveals it:

divide error: 0000 [#1] PREEMPT SMP KASAN PTI
RIP: 0010:i740fb_decode_var drivers/video/fbdev/i740fb.c:444 [inline]
RIP: 0010:i740fb_set_par+0x272f/0x3bb0 drivers/video/fbdev/i740fb.c:739
Call Trace:
 <TASK>
 fb_set_var+0x604/0xeb0 drivers/video/fbdev/core/fbmem.c:1036
 do_fb_ioctl+0x234/0x670 drivers/video/fbdev/core/fbmem.c:1112
 fb_ioctl+0xdd/0x130 drivers/video/fbdev/core/fbmem.c:1191
 vfs_ioctl fs/ioctl.c:51 [inline]
 __do_sys_ioctl fs/ioctl.c:874 [inline]

Regards,
Zheyu Ma

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

[Helge Deller]

On 4/3/22 13:26, Zheyu Ma wrote:
> Hi,
>
> 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 ?

Helge


> The following log reveals it:
>
> divide error: 0000 [#1] PREEMPT SMP KASAN PTI
> RIP: 0010:i740fb_decode_var drivers/video/fbdev/i740fb.c:444 [inline]
> RIP: 0010:i740fb_set_par+0x272f/0x3bb0 drivers/video/fbdev/i740fb.c:739
> Call Trace:
>  <TASK>
>  fb_set_var+0x604/0xeb0 drivers/video/fbdev/core/fbmem.c:1036
>  do_fb_ioctl+0x234/0x670 drivers/video/fbdev/core/fbmem.c:1112
>  fb_ioctl+0xdd/0x130 drivers/video/fbdev/core/fbmem.c:1191
>  vfs_ioctl fs/ioctl.c:51 [inline]
>  __do_sys_ioctl fs/ioctl.c:874 [inline]
>
> Regards,
> Zheyu Ma

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

[Geert Uytterhoeven]

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

Commit b36b242d4b8ea178 ("video: fbdev: asiliantfb: Error out if
'pixclock' equals zero") does not do that.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Helge Deller]

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

That said, currently I'd prefer to apply the zero-checks patches over
any untested patches. It's easy to revert such checks if a better solution
becomes available.

Thoughts?

> Commit b36b242d4b8ea178 ("video: fbdev: asiliantfb: Error out if
> 'pixclock' equals zero") does not do that.

Helge

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

[Geert Uytterhoeven]

Hi Helge,

On Tue, Apr 5, 2022 at 8:34 AM Helge Deller <deller@gmx.de> wrote:
> On 4/4/22 13:46, Geert Uytterhoeven wrote:
> > 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 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.
>
> That said, currently I'd prefer to apply the zero-checks patches over
> any untested patches. It's easy to revert such checks if a better solution
> becomes available.
>
> Thoughts?

Fair enough. And you're the maintainer ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Ondrej Zary]

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

> That said, currently I'd prefer to apply the zero-checks patches over
> any untested patches. It's easy to revert such checks if a better solution
> becomes available.
> 
> Thoughts?
> 
> > Commit b36b242d4b8ea178 ("video: fbdev: asiliantfb: Error out if
> > 'pixclock' equals zero") does not do that.
> 
> Helge
> 


-- 
Ondrej Zary

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

[Helge Deller]

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

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

[Zheyu Ma]

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

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/

Zheyu Ma

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

[Helge Deller]

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