Re: [PATCH] video: fbdev: cirrusfb: check pixclock to avoid divide by zero

[Geert Uytterhoeven]

Hi George,

On Tue, Oct 26, 2021 at 5:48 PM George Kennedy
<george.kennedy@xxxxxxxxxx> wrote:
> On 10/26/2021 10:11 AM, Geert Uytterhoeven wrote:
> > On Tue, Oct 26, 2021 at 3:38 PM George Kennedy
> > <george.kennedy@xxxxxxxxxx> wrote:
> >> On 10/26/2021 4:30 AM, Geert Uytterhoeven wrote:
> >>> On Mon, Oct 25, 2021 at 9:37 PM George Kennedy
> >>> <george.kennedy@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>>>>> --- 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()).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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