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

[George Kennedy]

Hi Geert,

On 10/26/2021 1:12 PM, Geert Uytterhoeven wrote:
> Hi George,
>
> On Tue, Oct 26, 2021 at 5:48 PM George Kennedy
> <george.kennedy at 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 at 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 at 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 at 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".

Thank you,
George
>
> Gr{oetje,eeting}s,
>
>                          Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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