None of these framebuffer drivers checks for 'pixclock', leading to many divide errors, which we fix by checking the value of 'pixclock' in *_check_var(). As discussed before, it is better to keep the check per driver rather than in the caller.
https://lore.kernel.org/all/YPgbHMtLQqb1kP0l@ravnborg.org/
Zheyu Ma (7): video: fbdev: i740fb: Error out if 'pixclock' equals zero video: fbdev: neofb: Fix the check of 'var->pixclock' video: fbdev: kyro: Error out if 'lineclock' equals zero video: fbdev: vt8623fb: Error out if 'pixclock' equals zero video: fbdev: tridentfb: Error out if 'pixclock' equals zero video: fbdev: arkfb: Error out if 'pixclock' equals zero video: fbdev: s3fb: Error out if 'pixclock' equals zero
drivers/video/fbdev/arkfb.c | 3 +++ drivers/video/fbdev/i740fb.c | 3 +++ drivers/video/fbdev/kyro/fbdev.c | 2 ++ drivers/video/fbdev/neofb.c | 2 +- drivers/video/fbdev/s3fb.c | 3 +++ drivers/video/fbdev/tridentfb.c | 3 +++ drivers/video/fbdev/vt8623fb.c | 3 +++ 7 files changed, 18 insertions(+), 1 deletion(-)
The userspace program could pass any values to the driver through ioctl() interface. If the driver doesn't check the value of 'pixclock', it may cause divide error.
Fix this by checking whether 'pixclock' is zero in the function i740fb_check_var().
The following log reveals it:
divide error: 0000 [#1] PREEMPT SMP KASAN PTI RIP: 0010:i740fb_decode_var drivers/video/fbdev/i740fb.c:444 [inline] RIP: 0010:i740fb_set_par+0x272f/0x3bb0 drivers/video/fbdev/i740fb.c:739 Call Trace: fb_set_var+0x604/0xeb0 drivers/video/fbdev/core/fbmem.c:1036 do_fb_ioctl+0x234/0x670 drivers/video/fbdev/core/fbmem.c:1112 fb_ioctl+0xdd/0x130 drivers/video/fbdev/core/fbmem.c:1191 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:874 [inline]
Signed-off-by: Zheyu Ma zheyuma97@gmail.com --- drivers/video/fbdev/i740fb.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/video/fbdev/i740fb.c b/drivers/video/fbdev/i740fb.c index 52cce0db8bd3..b595437a5752 100644 --- a/drivers/video/fbdev/i740fb.c +++ b/drivers/video/fbdev/i740fb.c @@ -657,6 +657,9 @@ static int i740fb_decode_var(const struct fb_var_screeninfo *var,
static int i740fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info) { + if (!var->pixclock) + return -EINVAL; + switch (var->bits_per_pixel) { case 8: var->red.offset = var->green.offset = var->blue.offset = 0;
On 4/4/22 10:47, Zheyu Ma wrote:
Hello Zheyu,
I've applied the patches #2-#7 of this series, but left out this specific patch (for now). As discussed on the mailing list we can try to come up with a better fix (to round up the pixclock when it's invalid). If not, I will apply this one later.
Thanks! Helge
On Fri, Apr 8, 2022 at 3:50 AM Helge Deller deller@gmx.de wrote:
I'm also looking forward to a more appropriate patch for this driver!
Thanks, Zheyu Ma
On Friday 08 April 2022 03:58:10 Zheyu Ma wrote:
I was not able to reproduce it at first but finally found it: the monitor must be unplugged. If a valid EDID is present, fb_validate_mode() call in i740fb_check_var() will refuse zero pixclock.
Haven't found any obvious way to correct zero pixclock value. Most other drivers simply return -EINVAL.
Thanks, Zheyu Ma
The previous check against 'var->pixclock' doesn't return -EINVAL when it equals zero, but the driver uses it again, causing the divide error.
Fix this by returning when 'var->pixclock' is zero.
The following log reveals it:
[ 49.704574] divide error: 0000 [#1] PREEMPT SMP KASAN PTI [ 49.704593] RIP: 0010:neofb_set_par+0x190f/0x49a0 [ 49.704635] Call Trace: [ 49.704636] <TASK> [ 49.704650] fb_set_var+0x604/0xeb0 [ 49.704702] do_fb_ioctl+0x234/0x670 [ 49.704745] fb_ioctl+0xdd/0x130 [ 49.704753] do_syscall_64+0x3b/0x90
Signed-off-by: Zheyu Ma zheyuma97@gmail.com --- drivers/video/fbdev/neofb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/neofb.c b/drivers/video/fbdev/neofb.c index 966df2a07360..28d32cbf496b 100644 --- a/drivers/video/fbdev/neofb.c +++ b/drivers/video/fbdev/neofb.c @@ -585,7 +585,7 @@ neofb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
DBG("neofb_check_var");
- if (var->pixclock && PICOS2KHZ(var->pixclock) > par->maxClock) + if (!var->pixclock || PICOS2KHZ(var->pixclock) > par->maxClock) return -EINVAL;
/* Is the mode larger than the LCD panel? */
The userspace program could pass any values to the driver through ioctl() interface. If the driver doesn't check the value of 'lineclock', it may cause divide error.
Fix this by checking whether 'lineclock' is zero.
The following log reveals it:
[ 33.404918] divide error: 0000 [#1] PREEMPT SMP KASAN PTI [ 33.404932] RIP: 0010:kyrofb_set_par+0x30d/0xd80 [ 33.404976] Call Trace: [ 33.404978] <TASK> [ 33.404987] fb_set_var+0x604/0xeb0 [ 33.405038] do_fb_ioctl+0x234/0x670 [ 33.405083] fb_ioctl+0xdd/0x130 [ 33.405091] do_syscall_64+0x3b/0x90
Signed-off-by: Zheyu Ma zheyuma97@gmail.com --- drivers/video/fbdev/kyro/fbdev.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/video/fbdev/kyro/fbdev.c b/drivers/video/fbdev/kyro/fbdev.c index 25801e8e3f74..d57772f96ad2 100644 --- a/drivers/video/fbdev/kyro/fbdev.c +++ b/drivers/video/fbdev/kyro/fbdev.c @@ -494,6 +494,8 @@ static int kyrofb_set_par(struct fb_info *info) info->var.hsync_len + info->var.left_margin)) / 1000;
+ if (!lineclock) + return -EINVAL;
/* time for a frame in ns (precision in 32bpp) */ frameclock = lineclock * (info->var.yres +
The userspace program could pass any values to the driver through ioctl() interface. If the driver doesn't check the value of 'pixclock', it may cause divide error.
Fix this by checking whether 'pixclock' is zero in the function vt8623fb_check_var().
The following log reveals it:
[ 47.778727] divide error: 0000 [#1] PREEMPT SMP KASAN PTI [ 47.778803] RIP: 0010:vt8623fb_set_par+0xecd/0x2210 [ 47.778870] Call Trace: [ 47.778872] <TASK> [ 47.778909] fb_set_var+0x604/0xeb0 [ 47.778995] do_fb_ioctl+0x234/0x670 [ 47.779041] fb_ioctl+0xdd/0x130 [ 47.779048] do_syscall_64+0x3b/0x90
Signed-off-by: Zheyu Ma zheyuma97@gmail.com --- drivers/video/fbdev/vt8623fb.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/video/fbdev/vt8623fb.c b/drivers/video/fbdev/vt8623fb.c index 7a959e5ba90b..a92a8c670cf0 100644 --- a/drivers/video/fbdev/vt8623fb.c +++ b/drivers/video/fbdev/vt8623fb.c @@ -321,6 +321,9 @@ static int vt8623fb_check_var(struct fb_var_screeninfo *var, struct fb_info *inf { int rv, mem, step;
+ if (!var->pixclock) + return -EINVAL; + /* Find appropriate format */ rv = svga_match_format (vt8623fb_formats, var, NULL); if (rv < 0)
Hi Zheyu,
On Mon, Apr 4, 2022 at 1:07 PM Zheyu Ma zheyuma97@gmail.com wrote:
Thanks for your patch!
When passed an invalid value, .check_var() is supposed to round up the invalid to a valid value, if possible.
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
The userspace program could pass any values to the driver through ioctl() interface. If the driver doesn't check the value of 'pixclock', it may cause divide error.
Fix this by checking whether 'pixclock' is zero.
The following log reveals it:
[ 38.260715] divide error: 0000 [#1] PREEMPT SMP KASAN PTI [ 38.260733] RIP: 0010:tridentfb_check_var+0x853/0xe60 [ 38.260791] Call Trace: [ 38.260793] <TASK> [ 38.260796] fb_set_var+0x367/0xeb0 [ 38.260879] do_fb_ioctl+0x234/0x670 [ 38.260922] fb_ioctl+0xdd/0x130 [ 38.260930] do_syscall_64+0x3b/0x90
Signed-off-by: Zheyu Ma zheyuma97@gmail.com --- drivers/video/fbdev/tridentfb.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/video/fbdev/tridentfb.c b/drivers/video/fbdev/tridentfb.c index 4d20cb557ff0..319131bd72cf 100644 --- a/drivers/video/fbdev/tridentfb.c +++ b/drivers/video/fbdev/tridentfb.c @@ -996,6 +996,9 @@ static int tridentfb_check_var(struct fb_var_screeninfo *var, int ramdac = 230000; /* 230MHz for most 3D chips */ debug("enter\n");
+ if (!var->pixclock) + return -EINVAL; + /* check color depth */ if (bpp == 24) bpp = var->bits_per_pixel = 32;
The userspace program could pass any values to the driver through ioctl() interface. If the driver doesn't check the value of 'pixclock', it may cause divide error.
Fix this by checking whether 'pixclock' is zero.
The following log reveals it:
[ 76.603696] divide error: 0000 [#1] PREEMPT SMP KASAN PTI [ 76.603712] RIP: 0010:arkfb_set_par+0x10fc/0x24f0 [ 76.603762] Call Trace: [ 76.603764] <TASK> [ 76.603773] fb_set_var+0x604/0xeb0 [ 76.603827] do_fb_ioctl+0x234/0x670 [ 76.603873] fb_ioctl+0xdd/0x130 [ 76.603881] do_syscall_64+0x3b/0x90
Signed-off-by: Zheyu Ma zheyuma97@gmail.com --- drivers/video/fbdev/arkfb.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c index edf169d0816e..eb3e47c58c5f 100644 --- a/drivers/video/fbdev/arkfb.c +++ b/drivers/video/fbdev/arkfb.c @@ -566,6 +566,9 @@ static int arkfb_check_var(struct fb_var_screeninfo *var, struct fb_info *info) { int rv, mem, step;
+ if (!var->pixclock) + return -EINVAL; + /* Find appropriate format */ rv = svga_match_format (arkfb_formats, var, NULL); if (rv < 0)
On Mon, Apr 4, 2022 at 3:10 PM Zheyu Ma zheyuma97@gmail.com wrote:
Thanks for your patch!
When passed an invalid value, .check_var() is supposed to round up the invalid to a valid value, if possible.
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
The userspace program could pass any values to the driver through ioctl() interface. If the driver doesn't check the value of 'pixclock', it may cause divide error.
Fix this by checking whether 'pixclock' is zero in s3fb_check_var().
The following log reveals it:
[ 511.141561] divide error: 0000 [#1] PREEMPT SMP KASAN PTI [ 511.141607] RIP: 0010:s3fb_check_var+0x3f3/0x530 [ 511.141693] Call Trace: [ 511.141695] <TASK> [ 511.141716] fb_set_var+0x367/0xeb0 [ 511.141815] do_fb_ioctl+0x234/0x670 [ 511.141876] fb_ioctl+0xdd/0x130 [ 511.141888] do_syscall_64+0x3b/0x90
Signed-off-by: Zheyu Ma zheyuma97@gmail.com --- drivers/video/fbdev/s3fb.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/video/fbdev/s3fb.c b/drivers/video/fbdev/s3fb.c index 5c74253e7b2c..b93c8eb02336 100644 --- a/drivers/video/fbdev/s3fb.c +++ b/drivers/video/fbdev/s3fb.c @@ -549,6 +549,9 @@ static int s3fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info) int rv, mem, step; u16 m, n, r;
+ if (!var->pixclock) + return -EINVAL; + /* Find appropriate format */ rv = svga_match_format (s3fb_formats, var, NULL);
On Mon, Apr 4, 2022 at 3:50 PM Zheyu Ma zheyuma97@gmail.com wrote:
When passed an invalid value, .check_var() is supposed to round up the invalid value to a valid value, if possible.
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
dri-devel@lists.freedesktop.org
-
Geert Uytterhoeven -
Helge Deller -
Ondrej Zary -
Zheyu Ma