From: George Kennedy <hidden> Date: 2021-10-25 19:01:10
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(+)
On Mon, Oct 25, 2021 at 02:01:30PM -0500, George Kennedy wrote:
quoted hunk
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(+)
@@ -477,6 +477,9 @@ static int cirrusfb_check_pixclock(const struct fb_var_screeninfo *var,structcirrusfb_info*cinfo=info->par;unsignedmaxclockidx=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
From: George Kennedy <hidden> Date: 2021-10-25 19:42:46
On 10/25/2021 3:07 PM, Greg KH wrote:
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(+)
@@ -477,6 +477,9 @@ static int cirrusfb_check_pixclock(const struct fb_var_screeninfo *var,structcirrusfb_info*cinfo=info->par;unsignedmaxclockidx=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
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(+)
@@ -477,6 +477,9 @@ static int cirrusfb_check_pixclock(const struct fb_var_screeninfo *var,structcirrusfb_info*cinfo=info->par;unsignedmaxclockidx=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
From: George Kennedy <hidden> Date: 2021-10-26 13:39:01
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
[off-list ref] wrote:
quoted
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(+)
@@ -477,6 +477,9 @@ static int cirrusfb_check_pixclock(const struct fb_var_screeninfo *var,structcirrusfb_info*cinfo=info->par;unsignedmaxclockidx=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
quoted
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
Hi George,
On Tue, Oct 26, 2021 at 3:38 PM George Kennedy
[off-list ref] wrote:
On 10/26/2021 4:30 AM, Geert Uytterhoeven wrote:
quoted
On Mon, Oct 25, 2021 at 9:37 PM George Kennedy
[off-list ref] wrote:
quoted
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>
@@ -477,6 +477,9 @@ static int cirrusfb_check_pixclock(const struct fb_var_screeninfo *var,structcirrusfb_info*cinfo=info->par;unsignedmaxclockidx=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.
quoted
quoted
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
From: George Kennedy <hidden> Date: 2021-10-26 15:48:18
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
[off-list ref] wrote:
quoted
On 10/26/2021 4:30 AM, Geert Uytterhoeven wrote:
quoted
On Mon, Oct 25, 2021 at 9:37 PM George Kennedy
[off-list ref] wrote:
quoted
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>
@@ -477,6 +477,9 @@ static int cirrusfb_check_pixclock(const struct fb_var_screeninfo *var,structcirrusfb_info*cinfo=info->par;unsignedmaxclockidx=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
quoted
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.
quoted
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.
quoted
quoted
quoted
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
Hi George,
On Tue, Oct 26, 2021 at 5:48 PM George Kennedy
[off-list ref] wrote:
On 10/26/2021 10:11 AM, Geert Uytterhoeven wrote:
quoted
On Tue, Oct 26, 2021 at 3:38 PM George Kennedy
[off-list ref] wrote:
quoted
On 10/26/2021 4:30 AM, Geert Uytterhoeven wrote:
quoted
On Mon, Oct 25, 2021 at 9:37 PM George Kennedy
[off-list ref] wrote:
quoted
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>
@@ -477,6 +477,9 @@ static int cirrusfb_check_pixclock(const struct fb_var_screeninfo *var,structcirrusfb_info*cinfo=info->par;unsignedmaxclockidx=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
From: George Kennedy <hidden> Date: 2021-10-27 01:13:22
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
[off-list ref] wrote:
quoted
On 10/26/2021 10:11 AM, Geert Uytterhoeven wrote:
quoted
On Tue, Oct 26, 2021 at 3:38 PM George Kennedy
[off-list ref] wrote:
quoted
On 10/26/2021 4:30 AM, Geert Uytterhoeven wrote:
quoted
On Mon, Oct 25, 2021 at 9:37 PM George Kennedy
[off-list ref] wrote:
quoted
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>
@@ -477,6 +477,9 @@ static int cirrusfb_check_pixclock(const struct fb_var_screeninfo *var,structcirrusfb_info*cinfo=info->par;unsignedmaxclockidx=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.
@@ -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
[off-list ref] wrote:
quoted hunk
On 10/26/2021 1:12 PM, Geert Uytterhoeven wrote:
quoted
On Tue, Oct 26, 2021 at 5:48 PM George Kennedy
[off-list ref] wrote:
quoted
On 10/26/2021 10:11 AM, Geert Uytterhoeven wrote:
quoted
On Tue, Oct 26, 2021 at 3:38 PM George Kennedy
[off-list ref] wrote:
quoted
On 10/26/2021 4:30 AM, Geert Uytterhoeven wrote:
quoted
On Mon, Oct 25, 2021 at 9:37 PM George Kennedy
[off-list ref] wrote:
quoted
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>
@@ -477,6 +477,9 @@ static int cirrusfb_check_pixclock(const struct fb_var_screeninfo *var,structcirrusfb_info*cinfo=info->par;unsignedmaxclockidx=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.
@@ -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
From: George Kennedy <hidden> Date: 2021-10-27 12:06:34
On 10/27/2021 2:53 AM, Geert Uytterhoeven wrote:
Hi George,
On Wed, Oct 27, 2021 at 3:13 AM George Kennedy
[off-list ref] wrote:
quoted
On 10/26/2021 1:12 PM, Geert Uytterhoeven wrote:
quoted
On Tue, Oct 26, 2021 at 5:48 PM George Kennedy
[off-list ref] wrote:
quoted
On 10/26/2021 10:11 AM, Geert Uytterhoeven wrote:
quoted
On Tue, Oct 26, 2021 at 3:38 PM George Kennedy
[off-list ref] wrote:
quoted
On 10/26/2021 4:30 AM, Geert Uytterhoeven wrote:
quoted
On Mon, Oct 25, 2021 at 9:37 PM George Kennedy
[off-list ref] wrote:
quoted
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>
@@ -477,6 +477,9 @@ static int cirrusfb_check_pixclock(const struct fb_var_screeninfo *var,structcirrusfb_info*cinfo=info->par;unsignedmaxclockidx=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.
@@ -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
From: George Kennedy <hidden> Date: 2021-10-27 15:22:34
On 10/27/2021 2:53 AM, Geert Uytterhoeven wrote:
Hi George,
On Wed, Oct 27, 2021 at 3:13 AM George Kennedy
[off-list ref] wrote:
quoted
On 10/26/2021 1:12 PM, Geert Uytterhoeven wrote:
quoted
On Tue, Oct 26, 2021 at 5:48 PM George Kennedy
[off-list ref] wrote:
quoted
On 10/26/2021 10:11 AM, Geert Uytterhoeven wrote:
quoted
On Tue, Oct 26, 2021 at 3:38 PM George Kennedy
[off-list ref] wrote:
quoted
On 10/26/2021 4:30 AM, Geert Uytterhoeven wrote:
quoted
On Mon, Oct 25, 2021 at 9:37 PM George Kennedy
[off-list ref] wrote:
quoted
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>
@@ -477,6 +477,9 @@ static int cirrusfb_check_pixclock(const struct fb_var_screeninfo *var,structcirrusfb_info*cinfo=info->par;unsignedmaxclockidx=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.
@@ -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.
@@ -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