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

From: Sam Ravnborg <sam@ravnborg.org>
To: Zheyu Ma <zheyuma97@gmail.com>
Cc: linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org,
	tzimmermann@suse.de, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] video: fbdev: neofb: add a check against divide error
Date: Wed, 21 Jul 2021 15:03:24 +0200	[thread overview]
Message-ID: <YPgbHMtLQqb1kP0l@ravnborg.org> (raw)
In-Reply-To: <1626871424-27708-1-git-send-email-zheyuma97@gmail.com>

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