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

Hi George,

On Mon, Oct 25, 2021 at 9:37 PM George Kennedy
[off-list ref] wrote:

On 10/25/2021 3:07 PM, Greg KH wrote:

quoted

On Mon, Oct 25, 2021 at 02:01:30PM -0500, George Kennedy wrote:

quoted

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 <redacted>
---
  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.

quoted

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)

quoted

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