Messages in this thread | | | Date | Tue, 26 Oct 2021 09:36:29 -0400 | Subject | Re: [PATCH] video: fbdev: cirrusfb: check pixclock to avoid divide by zero | From | George Kennedy <> |
| |
Hi Geert,
On 10/26/2021 4:30 AM, Geert Uytterhoeven wrote: > Hi George, > > 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> >>>> --- >>>> drivers/video/fbdev/cirrusfb.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/drivers/video/fbdev/cirrusfb.c b/drivers/video/fbdev/cirrusfb.c >>>> index 93802ab..099ddcb 100644 >>>> --- 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
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.
I would think we would want to keep this driver consistent with the other fb drivers that return failure with the incorrect value.
Thank you, George > >>> 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@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
| |