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

[Geert Uytterhoeven]

Hi George,

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.

> We can plug in a default value, but I believe it is just covering up the
> fact that an incorrect value has been copied in.

Passing zero is not incorrect.  The driver is supposed to round it
up to a valid value.

> I would think we would want to keep this driver consistent with the
> other fb drivers that return failure with the incorrect value.

I disagree: non-conformant behavior should be fixed, not copied.

> >>> Shouldn't you be checking further up the call chain where this got set
> >>> to 0?
> >> The same pixclock check is done in these fb drivers:
> >>
> >> arch/arm/mach-rpc/include/mach/acornfb.h:    if (!var->pixclock)
> >> drivers/video/fbdev/asiliantfb.c:    if (!var->pixclock)
> >> drivers/video/fbdev/clps711x-fb.c:    if (!var->pixclock)
> >> drivers/video/fbdev/core/fbmon.c:    if (!var->pixclock)
> >> drivers/video/fbdev/core/modedb.c:    if (!var->pixclock)
> >> drivers/video/fbdev/cirrusfb.c:    if (!var->pixclock)
> >> drivers/video/fbdev/kyro/fbdev.c:    if (!var->pixclock)
> >> drivers/video/fbdev/riva/fbdev.c:    if (!var->pixclock)
> >> drivers/video/fbdev/uvesafb.c:    if (!var->pixclock)
> >>
> >>> What logic allows this to be a valid value?  What about all other fb
> >>> drivers?
> >> The "check_var" function, which is set into the ".fb_check_var" element
> >> of the fb_ops struct, should do the check, but in the case of cirrusfb,
> >> that is not being done.
> >>
> >> All this patch does is add the same pixclock check that the other above
> >> fb drivers do.
> > Indeed, several drivers are not following the rounding rules.

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