Hi Helge,
On Sun, Apr 3, 2022 at 5:41 PM Helge Deller <[email protected]> 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 -- [email protected]
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
Hello Geert,
On 4/4/22 13:46, Geert Uytterhoeven wrote:
> Hi Helge,
>
> On Sun, Apr 3, 2022 at 5:41 PM Helge Deller <[email protected]> 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
Hi Helge,
On Tue, Apr 5, 2022 at 8:34 AM Helge Deller <[email protected]> wrote:
> On 4/4/22 13:46, Geert Uytterhoeven wrote:
> > On Sun, Apr 3, 2022 at 5:41 PM Helge Deller <[email protected]> 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 -- [email protected]
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
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 <[email protected]> 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
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 <[email protected]> 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
On Wed, Apr 6, 2022 at 2:23 AM Helge Deller <[email protected]> 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 <[email protected]> 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/[email protected]/
Zheyu Ma
On 4/6/22 03:24, Zheyu Ma wrote:
> On Wed, Apr 6, 2022 at 2:23 AM Helge Deller <[email protected]> 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 <[email protected]> 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/[email protected]/
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