[PATCH] video: fbdev: neofb: add a check against divide error

[PATCH] video: fbdev: neofb: add a check against divide error

[Zheyu Ma]

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 because of the 'PICOS2KHZ' macro.

Fix this by checking whether 'pixclock' is zero first.

The following log reveals it:

[   53.093806] divide error: 0000 [#1] PREEMPT SMP KASAN PTI
[   53.093838] CPU: 3 PID: 11763 Comm: hang Not tainted 5.14.0-rc2-00478-g2734d6c1b1a0 #215
[   53.093859] RIP: 0010:neofb_check_var+0x80/0xe50
[   53.093951] Call Trace:
[   53.093956]  ? neofb_setcolreg+0x2b0/0x2b0
[   53.093968]  fb_set_var+0x2e4/0xeb0
[   53.093977]  ? fb_blank+0x1a0/0x1a0
[   53.093984]  ? lock_acquire+0x1ef/0x530
[   53.093996]  ? lock_release+0x810/0x810
[   53.094005]  ? lock_is_held_type+0x100/0x140
[   53.094016]  ? ___might_sleep+0x1ee/0x2d0
[   53.094028]  ? __mutex_lock+0x620/0x1190
[   53.094036]  ? do_fb_ioctl+0x313/0x700
[   53.094044]  ? mutex_lock_io_nested+0xfa0/0xfa0
[   53.094051]  ? __this_cpu_preempt_check+0x1d/0x30
[   53.094060]  ? _raw_spin_unlock_irqrestore+0x46/0x60
[   53.094069]  ? lockdep_hardirqs_on+0x59/0x100
[   53.094076]  ? _raw_spin_unlock_irqrestore+0x46/0x60
[   53.094085]  ? trace_hardirqs_on+0x6a/0x1c0
[   53.094096]  do_fb_ioctl+0x31e/0x700

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 c0f4f402da3f..966df2a07360 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 (PICOS2KHZ(var->pixclock) > par->maxClock)
+	if (var->pixclock && PICOS2KHZ(var->pixclock) > par->maxClock)
 		return -EINVAL;
 
 	/* Is the mode larger than the LCD panel? */
-- 
2.17.6

Re: [PATCH] video: fbdev: neofb: add a check against divide error

[Sam Ravnborg]

Hi Zheyu,
On Wed, Jul 21, 2021 at 12:43:44PM +0000, Zheyu Ma wrote:
> 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 because of the 'PICOS2KHZ' macro.
> 
> Fix this by checking whether 'pixclock' is zero first.
> 
> The following log reveals it:
> 
> [   53.093806] divide error: 0000 [#1] PREEMPT SMP KASAN PTI
> [   53.093838] CPU: 3 PID: 11763 Comm: hang Not tainted 5.14.0-rc2-00478-g2734d6c1b1a0 #215
> [   53.093859] RIP: 0010:neofb_check_var+0x80/0xe50
> [   53.093951] Call Trace:
> [   53.093956]  ? neofb_setcolreg+0x2b0/0x2b0
> [   53.093968]  fb_set_var+0x2e4/0xeb0
> [   53.093977]  ? fb_blank+0x1a0/0x1a0
> [   53.093984]  ? lock_acquire+0x1ef/0x530
> [   53.093996]  ? lock_release+0x810/0x810
> [   53.094005]  ? lock_is_held_type+0x100/0x140
> [   53.094016]  ? ___might_sleep+0x1ee/0x2d0
> [   53.094028]  ? __mutex_lock+0x620/0x1190
> [   53.094036]  ? do_fb_ioctl+0x313/0x700
> [   53.094044]  ? mutex_lock_io_nested+0xfa0/0xfa0
> [   53.094051]  ? __this_cpu_preempt_check+0x1d/0x30
> [   53.094060]  ? _raw_spin_unlock_irqrestore+0x46/0x60
> [   53.094069]  ? lockdep_hardirqs_on+0x59/0x100
> [   53.094076]  ? _raw_spin_unlock_irqrestore+0x46/0x60
> [   53.094085]  ? trace_hardirqs_on+0x6a/0x1c0
> [   53.094096]  do_fb_ioctl+0x31e/0x700
> 
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>

I looked if we could move this check up to the caller, but it seems
better to keep it per driver.
Added the patch to drm-misc-next, it will appera in -next in around one
week.

	Sam

> ---
>  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 c0f4f402da3f..966df2a07360 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 (PICOS2KHZ(var->pixclock) > par->maxClock)
> +	if (var->pixclock && PICOS2KHZ(var->pixclock) > par->maxClock)
>  		return -EINVAL;
>  
>  	/* Is the mode larger than the LCD panel? */
> -- 
> 2.17.6