Messages in this thread | | | Date | Wed, 27 Oct 2021 08:05:56 -0400 | Subject | Re: [PATCH] video: fbdev: cirrusfb: check pixclock to avoid divide by zero | From | George Kennedy <> |
| |
On 10/27/2021 2:53 AM, Geert Uytterhoeven wrote: > Hi George, > > On Wed, Oct 27, 2021 at 3:13 AM George Kennedy > <george.kennedy@oracle.com> wrote: >> On 10/26/2021 1:12 PM, Geert Uytterhoeven wrote: >>> On Tue, Oct 26, 2021 at 5:48 PM George Kennedy >>> <george.kennedy@oracle.com> wrote: >>>> On 10/26/2021 10:11 AM, Geert Uytterhoeven wrote: >>>>> On Tue, Oct 26, 2021 at 3:38 PM George Kennedy >>>>> <george.kennedy@oracle.com> wrote: >>>>>> On 10/26/2021 4:30 AM, Geert Uytterhoeven wrote: >>>>>>> On Mon, Oct 25, 2021 at 9:37 PM George Kennedy >>>>>>> <george.kennedy@oracle.com> wrote: >>>>>>>> On 10/25/2021 3:07 PM, Greg KH wrote: >>>>>>>>> On Mon, Oct 25, 2021 at 02:01:30PM -0500, George Kennedy wrote: >>>>>>>>>> Do a sanity check on pixclock value before using it as a divisor. >>>>>>>>>> >>>>>>>>>> Syzkaller reported a divide error in cirrusfb_check_pixclock. >>>>>>>>>> >>>>>>>>>> divide error: 0000 [#1] SMP KASAN PTI >>>>>>>>>> CPU: 0 PID: 14938 Comm: cirrusfb_test Not tainted 5.15.0-rc6 #1 >>>>>>>>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-2 >>>>>>>>>> RIP: 0010:cirrusfb_check_var+0x6f1/0x1260 >>>>>>>>>> >>>>>>>>>> Call Trace: >>>>>>>>>> fb_set_var+0x398/0xf90 >>>>>>>>>> do_fb_ioctl+0x4b8/0x6f0 >>>>>>>>>> fb_ioctl+0xeb/0x130 >>>>>>>>>> __x64_sys_ioctl+0x19d/0x220 >>>>>>>>>> do_syscall_64+0x3a/0x80 >>>>>>>>>> entry_SYSCALL_64_after_hwframe+0x44/0xae >>>>>>>>>> >>>>>>>>>> Signed-off-by: George Kennedy <george.kennedy@oracle.com> >>>>>>>>>> --- a/drivers/video/fbdev/cirrusfb.c >>>>>>>>>> +++ b/drivers/video/fbdev/cirrusfb.c >>>>>>>>>> @@ -477,6 +477,9 @@ static int cirrusfb_check_pixclock(const struct fb_var_screeninfo *var, >>>>>>>>>> struct cirrusfb_info *cinfo = info->par; >>>>>>>>>> unsigned maxclockidx = var->bits_per_pixel >> 3; >>>>>>>>>> >>>>>>>>>> + if (!var->pixclock) >>>>>>>>>> + return -EINVAL; >>>>>>> This is not correct: fbdev drivers should round up invalid values, >>>>>>> and only return an error if rounding up cannot yield a valid value. >>>>>> What default value would you recommend? Here are examples of some of the >>>>>> possible cirrusfb pixclock values: >>>>>> 40000: 25MHz >>>>>> 20000: 50Mhz >>>>>> 12500: 80Mhz >>>>> You should pick the lowest supported value. >>>> In bestclock() the frequency value ("freq") is not allowed to go below 8000. >>>> >>>> if (freq < 8000) >>>> freq = 8000; >>>> >>>> If pixclock is passed in as zero to cirrusfb_check_pixclock(), is it ok >>>> to then set the value of pixclock to 125000, which will result in "freq" >>>> being set to 8000 (or adjust the passed in pixclock value to make sure >>>> "freq" does not get below 8000)? >>> No, clock rate is the inverse of clock period. >>> So the smallest clock period (fb_var_screeninfo.pixclock) corresponds >>> to the largest clock rate (freq in bestclock()). >> How about this? >> >> This gets the frequency derived from pixclock to maxclock or rounds up >> pixclock to get the frequency as close to maxclock as possible. >> >> diff --git a/drivers/video/fbdev/cirrusfb.c b/drivers/video/fbdev/cirrusfb.c >> index 93802ab..2e8e620 100644 >> --- a/drivers/video/fbdev/cirrusfb.c >> +++ b/drivers/video/fbdev/cirrusfb.c >> @@ -620,6 +620,18 @@ static int cirrusfb_check_var(struct >> fb_var_screeninfo *var, >> return -EINVAL; >> } >> >> + if (!var->pixclock) { >> + long maxclock; >> + unsigned maxclockidx = var->bits_per_pixel >> 3; >> + >> + maxclock = >> cirrusfb_board_info[cinfo->btype].maxclock[maxclockidx]; >> + >> + var->pixclock = KHZ2PICOS(maxclock); >> + while (PICOS2KHZ(var->pixclock) > maxclock) { >> + var->pixclock++; >> + } >> + } >> + >> if (cirrusfb_check_pixclock(var, info)) >> return -EINVAL; >> >> The work can't be done in cirrusfb_check_pixclock() as var->pixclock is >> read-only because "var" is "const struct fb_var_screeninfo *var". > Perhaps the const should be dropped from the var parameter, so the > rounding can be done in the function where it makes most sense, > and where most of the above operations are already done? > > Then, you can simplify: > > - freq = PICOS2KHZ(var->pixclock); > + freq = PICOS2KHZ(var->pixclock ? : 1); > > and change the "if (freq > maxclock) return -EINVAL" to use maxclock > instead.
Thanks Geert,
Will make the suggested changes and send out a v2 review.
George > 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
| |