| Message ID | 1635188490-15082-1-git-send-email-george.kennedy@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | video: fbdev: cirrusfb: check pixclock to avoid divide by zero | expand |
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; Shouldn't you be checking further up the call chain where this got set to 0? What logic allows this to be a valid value? What about all other fb drivers? thanks, greg k-h
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; > 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. BTW, this patch should also go into stable (specifically, 5.4.y). Thank you, George > > thanks, > > greg k-h
Hi George, On Mon, Oct 25, 2021 at 03:33:43PM -0400, George Kennedy 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; > > Shouldn't you be checking further up the call chain where this got set > > to 0? > I recall we had a similar discussion when Zheyu Ma <zheyuma97@gmail.com> added the same checks to at least 3 fbdev drivers. The conclusion was that the check belongs in the individual check functions - IIRC. Maybe Zheyu or google can help find the original thread. Sam
On Tue, Oct 26, 2021 at 5:45 AM Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi George, > > On Mon, Oct 25, 2021 at 03:33:43PM -0400, George Kennedy 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; > > > Shouldn't you be checking further up the call chain where this got set > > > to 0? > > > I recall we had a similar discussion when Zheyu Ma <zheyuma97@gmail.com> > added the same checks to at least 3 fbdev drivers. > The conclusion was that the check belongs in the individual check > functions - IIRC. > > Maybe Zheyu or google can help find the original thread. Related discussions are here: https://lore.kernel.org/all/1627179054-29903-1-git-send-email-zheyuma97@gmail.com/ <https://lore.kernel.org/all/1627179054-29903-1-git-send-email-zheyuma97@gmail.com/#t> https://lore.kernel.org/all/1626871424-27708-1-git-send-email-zheyuma97@gmail.com/ https://lore.kernel.org/all/1627293835-17441-1-git-send-email-zheyuma97@gmail.com/ Regards, Zheyu Ma
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. > > 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
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
Hi George, On Tue, Oct 26, 2021 at 3:38 PM George Kennedy <george.kennedy@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@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> > >>>> --- 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@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
Hi Geert, On 10/26/2021 10:11 AM, Geert Uytterhoeven wrote: > Hi George, > > On Tue, Oct 26, 2021 at 3:38 PM George Kennedy > <george.kennedy@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@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> >>>>>> --- 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)? Thank you, George > >> 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@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
Hi George, On Tue, Oct 26, 2021 at 5:48 PM George Kennedy <george.kennedy@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@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@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> > >>>>>> --- 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@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
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@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@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@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> >>>>>>>> --- 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@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
Hi George, On Wed, Oct 27, 2021 at 3:13 AM George Kennedy <george.kennedy@oracle.com> wrote: > On 10/26/2021 1:12 PM, Geert Uytterhoeven wrote: > > On Tue, Oct 26, 2021 at 5:48 PM George Kennedy > > <george.kennedy@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@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@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> > >>>>>>>> --- 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". Perhaps the const should be dropped from the var parameter, so the rounding can be done in the function where it makes most sense, and where most of the above operations are already done? Then, you can simplify: - freq = PICOS2KHZ(var->pixclock); + freq = PICOS2KHZ(var->pixclock ? : 1); and change the "if (freq > maxclock) return -EINVAL" to use maxclock instead. 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
On 10/27/2021 2:53 AM, Geert Uytterhoeven wrote: > Hi George, > > On Wed, Oct 27, 2021 at 3:13 AM George Kennedy > <george.kennedy@oracle.com> wrote: >> On 10/26/2021 1:12 PM, Geert Uytterhoeven wrote: >>> On Tue, Oct 26, 2021 at 5:48 PM George Kennedy >>> <george.kennedy@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@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@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> >>>>>>>>>> --- 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". > Perhaps the const should be dropped from the var parameter, so the > rounding can be done in the function where it makes most sense, > and where most of the above operations are already done? > > Then, you can simplify: > > - freq = PICOS2KHZ(var->pixclock); > + freq = PICOS2KHZ(var->pixclock ? : 1); > > and change the "if (freq > maxclock) return -EINVAL" to use maxclock > instead. Thanks Geert, Will make the suggested changes and send out a v2 review. George > 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
On 10/27/2021 2:53 AM, Geert Uytterhoeven wrote: > Hi George, > > On Wed, Oct 27, 2021 at 3:13 AM George Kennedy > <george.kennedy@oracle.com> wrote: >> On 10/26/2021 1:12 PM, Geert Uytterhoeven wrote: >>> On Tue, Oct 26, 2021 at 5:48 PM George Kennedy >>> <george.kennedy@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@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@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> >>>>>>>>>> --- 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". > Perhaps the const should be dropped from the var parameter, so the > rounding can be done in the function where it makes most sense, > and where most of the above operations are already done? > > Then, you can simplify: > > - freq = PICOS2KHZ(var->pixclock); > + freq = PICOS2KHZ(var->pixclock ? : 1); > > and change the "if (freq > maxclock) return -EINVAL" to use maxclock > instead. Geert, Does this look ok? diff --git a/drivers/video/fbdev/cirrusfb.c b/drivers/video/fbdev/cirrusfb.c index 93802ab..3d47c34 100644 --- a/drivers/video/fbdev/cirrusfb.c +++ b/drivers/video/fbdev/cirrusfb.c @@ -469,7 +469,7 @@ static int cirrusfb_check_mclk(struct fb_info *info, long freq) return 0; } -static int cirrusfb_check_pixclock(const struct fb_var_screeninfo *var, +static int cirrusfb_check_pixclock(struct fb_var_screeninfo *var, struct fb_info *info) { long freq; @@ -478,9 +478,7 @@ static int cirrusfb_check_pixclock(const struct fb_var_screeninfo *var, unsigned maxclockidx = var->bits_per_pixel >> 3; /* convert from ps to kHz */ - freq = PICOS2KHZ(var->pixclock); - - dev_dbg(info->device, "desired pixclock: %ld kHz\n", freq); + freq = PICOS2KHZ(var->pixclock ? : 1); maxclock = cirrusfb_board_info[cinfo->btype].maxclock[maxclockidx]; cinfo->multiplexing = 0; @@ -488,11 +486,13 @@ static int cirrusfb_check_pixclock(const struct fb_var_screeninfo *var, /* If the frequency is greater than we can support, we might be able * to use multiplexing for the video mode */ if (freq > maxclock) { - dev_err(info->device, - "Frequency greater than maxclock (%ld kHz)\n", - maxclock); - return -EINVAL; + var->pixclock = KHZ2PICOS(maxclock); + + while ((freq = PICOS2KHZ(var->pixclock)) > maxclock) + var->pixclock++; } + dev_dbg(info->device, "desired pixclock: %ld kHz\n", freq); + /* * Additional constraint: 8bpp uses DAC clock doubling to allow maximum * pixel clock Is the pixclock round-up still needed? Without it the frequency may be slightly above maxclock in some cases. Thank you, George > > 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
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; + /* convert from ps to kHz */ freq = PICOS2KHZ(var->pixclock);
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(+)