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

From: Zheyu Ma <zheyuma97@gmail.com>
To: Helge Deller <deller@gmx.de>
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>
Subject: Re: [BUG] fbdev: i740fb: Divide error when ‘var->pixclock’ is zero
Date: Wed, 6 Apr 2022 09:24:20 +0800	[thread overview]
Message-ID: <CAMhUBjmBm3=CY=cCZuH0+ZeemNVT=9XywSoYiR7WLcYOUGu9VQ@mail.gmail.com> (raw)
In-Reply-To: <527a8c23-609f-5f8a-e076-a8a59da59865@gmx.de>

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