diff mbox series

[v2] fbmem: pull fbcon_update_vcs() out of fb_set_var()

Message ID 075b7e37-3278-cd7d-31ab-c5073cfa8e92@i-love.sakura.ne.jp (mailing list archive)
State Superseded, archived
Headers show
Series [v2] fbmem: pull fbcon_update_vcs() out of fb_set_var() | expand

Commit Message

Tetsuo Handa July 30, 2020, 10:47 a.m. UTC 
syzbot is reporting OOB read bug in vc_do_resize() [1] caused by memcpy()
based on outdated old_{rows,row_size} values, for resize_screen() can
recurse into vc_do_resize() which changes vc->vc_{cols,rows} that outdates
old_{rows,row_size} values which were saved before calling resize_screen().

Daniel Vetter explained that resize_screen() should not recurse into
fbcon_update_vcs() path due to FBINFO_MISC_USEREVENT being still set
when calling resize_screen().

Instead of masking FBINFO_MISC_USEREVENT before calling fbcon_update_vcs(),
we can remove FBINFO_MISC_USEREVENT by calling fbcon_update_vcs() only if
fb_set_var() returned 0. This change assumes that it is harmless to call
fbcon_update_vcs() when fb_set_var() returned 0 without reaching
fb_notifier_call_chain().

[1] https://syzkaller.appspot.com/bug?id=c70c88cfd16dcf6e1d3c7f0ab8648b3144b5b25e

Reported-and-tested-by: syzbot <syzbot+c37a14770d51a085a520@syzkaller.appspotmail.com>
Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: kernel test robot <lkp@intel.com> for missing #include
---
 drivers/video/fbdev/core/fbmem.c   | 8 ++------
 drivers/video/fbdev/core/fbsysfs.c | 4 ++--
 drivers/video/fbdev/ps3fb.c        | 5 +++--
 include/linux/fb.h                 | 2 --
 4 files changed, 7 insertions(+), 12 deletions(-)

Comments

Daniel Vetter Aug. 4, 2020, 5:38 a.m. UTC | #1 
On Thu, Jul 30, 2020 at 07:47:14PM +0900, Tetsuo Handa wrote:
> syzbot is reporting OOB read bug in vc_do_resize() [1] caused by memcpy()
> based on outdated old_{rows,row_size} values, for resize_screen() can
> recurse into vc_do_resize() which changes vc->vc_{cols,rows} that outdates
> old_{rows,row_size} values which were saved before calling resize_screen().
> 
> Daniel Vetter explained that resize_screen() should not recurse into
> fbcon_update_vcs() path due to FBINFO_MISC_USEREVENT being still set
> when calling resize_screen().
> 
> Instead of masking FBINFO_MISC_USEREVENT before calling fbcon_update_vcs(),
> we can remove FBINFO_MISC_USEREVENT by calling fbcon_update_vcs() only if
> fb_set_var() returned 0. This change assumes that it is harmless to call
> fbcon_update_vcs() when fb_set_var() returned 0 without reaching
> fb_notifier_call_chain().
> 
> [1] https://syzkaller.appspot.com/bug?id=c70c88cfd16dcf6e1d3c7f0ab8648b3144b5b25e
> 
> Reported-and-tested-by: syzbot <syzbot+c37a14770d51a085a520@syzkaller.appspotmail.com>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reported-by: kernel test robot <lkp@intel.com> for missing #include

Thanks a lot for your patch, queued up to hopefully still make it for
5.9-rc1.

Cheers, Daniel

> ---
>  drivers/video/fbdev/core/fbmem.c   | 8 ++------
>  drivers/video/fbdev/core/fbsysfs.c | 4 ++--
>  drivers/video/fbdev/ps3fb.c        | 5 +++--
>  include/linux/fb.h                 | 2 --
>  4 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 30e73ec..da7c88f 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -957,7 +957,6 @@ static int fb_check_caps(struct fb_info *info, struct fb_var_screeninfo *var,
>  int
>  fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>  {
> -	int flags = info->flags;
>  	int ret = 0;
>  	u32 activate;
>  	struct fb_var_screeninfo old_var;
> @@ -1052,9 +1051,6 @@ static int fb_check_caps(struct fb_info *info, struct fb_var_screeninfo *var,
>  	event.data = &mode;
>  	fb_notifier_call_chain(FB_EVENT_MODE_CHANGE, &event);
>  
> -	if (flags & FBINFO_MISC_USEREVENT)
> -		fbcon_update_vcs(info, activate & FB_ACTIVATE_ALL);
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL(fb_set_var);
> @@ -1105,9 +1101,9 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>  			return -EFAULT;
>  		console_lock();
>  		lock_fb_info(info);
> -		info->flags |= FBINFO_MISC_USEREVENT;
>  		ret = fb_set_var(info, &var);
> -		info->flags &= ~FBINFO_MISC_USEREVENT;
> +		if (!ret)
> +			fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
>  		unlock_fb_info(info);
>  		console_unlock();
>  		if (!ret && copy_to_user(argp, &var, sizeof(var)))
> diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
> index d54c88f..65dae05 100644
> --- a/drivers/video/fbdev/core/fbsysfs.c
> +++ b/drivers/video/fbdev/core/fbsysfs.c
> @@ -91,9 +91,9 @@ static int activate(struct fb_info *fb_info, struct fb_var_screeninfo *var)
>  
>  	var->activate |= FB_ACTIVATE_FORCE;
>  	console_lock();
> -	fb_info->flags |= FBINFO_MISC_USEREVENT;
>  	err = fb_set_var(fb_info, var);
> -	fb_info->flags &= ~FBINFO_MISC_USEREVENT;
> +	if (!err)
> +		fbcon_update_vcs(fb_info, var->activate & FB_ACTIVATE_ALL);
>  	console_unlock();
>  	if (err)
>  		return err;
> diff --git a/drivers/video/fbdev/ps3fb.c b/drivers/video/fbdev/ps3fb.c
> index 9df78fb..203c254 100644
> --- a/drivers/video/fbdev/ps3fb.c
> +++ b/drivers/video/fbdev/ps3fb.c
> @@ -29,6 +29,7 @@
>  #include <linux/freezer.h>
>  #include <linux/uaccess.h>
>  #include <linux/fb.h>
> +#include <linux/fbcon.h>
>  #include <linux/init.h>
>  
>  #include <asm/cell-regs.h>
> @@ -824,12 +825,12 @@ static int ps3fb_ioctl(struct fb_info *info, unsigned int cmd,
>  				var = info->var;
>  				fb_videomode_to_var(&var, vmode);
>  				console_lock();
> -				info->flags |= FBINFO_MISC_USEREVENT;
>  				/* Force, in case only special bits changed */
>  				var.activate |= FB_ACTIVATE_FORCE;
>  				par->new_mode_id = val;
>  				retval = fb_set_var(info, &var);
> -				info->flags &= ~FBINFO_MISC_USEREVENT;
> +				if (!retval)
> +					fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
>  				console_unlock();
>  			}
>  			break;
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index 3b4b2f0..b11eb02 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -400,8 +400,6 @@ struct fb_tile_ops {
>  #define FBINFO_HWACCEL_YPAN		0x2000 /* optional */
>  #define FBINFO_HWACCEL_YWRAP		0x4000 /* optional */
>  
> -#define FBINFO_MISC_USEREVENT          0x10000 /* event request
> -						  from userspace */
>  #define FBINFO_MISC_TILEBLITTING       0x20000 /* use tile blitting */
>  
>  /* A driver may set this flag to indicate that it does want a set_par to be
> -- 
> 1.8.3.1
>
diff mbox series

Patch

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 30e73ec..da7c88f 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -957,7 +957,6 @@  static int fb_check_caps(struct fb_info *info, struct fb_var_screeninfo *var,
 int
 fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
 {
-	int flags = info->flags;
 	int ret = 0;
 	u32 activate;
 	struct fb_var_screeninfo old_var;
@@ -1052,9 +1051,6 @@  static int fb_check_caps(struct fb_info *info, struct fb_var_screeninfo *var,
 	event.data = &mode;
 	fb_notifier_call_chain(FB_EVENT_MODE_CHANGE, &event);
 
-	if (flags & FBINFO_MISC_USEREVENT)
-		fbcon_update_vcs(info, activate & FB_ACTIVATE_ALL);
-
 	return 0;
 }
 EXPORT_SYMBOL(fb_set_var);
@@ -1105,9 +1101,9 @@  static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
 			return -EFAULT;
 		console_lock();
 		lock_fb_info(info);
-		info->flags |= FBINFO_MISC_USEREVENT;
 		ret = fb_set_var(info, &var);
-		info->flags &= ~FBINFO_MISC_USEREVENT;
+		if (!ret)
+			fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
 		unlock_fb_info(info);
 		console_unlock();
 		if (!ret && copy_to_user(argp, &var, sizeof(var)))
diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
index d54c88f..65dae05 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -91,9 +91,9 @@  static int activate(struct fb_info *fb_info, struct fb_var_screeninfo *var)
 
 	var->activate |= FB_ACTIVATE_FORCE;
 	console_lock();
-	fb_info->flags |= FBINFO_MISC_USEREVENT;
 	err = fb_set_var(fb_info, var);
-	fb_info->flags &= ~FBINFO_MISC_USEREVENT;
+	if (!err)
+		fbcon_update_vcs(fb_info, var->activate & FB_ACTIVATE_ALL);
 	console_unlock();
 	if (err)
 		return err;
diff --git a/drivers/video/fbdev/ps3fb.c b/drivers/video/fbdev/ps3fb.c
index 9df78fb..203c254 100644
--- a/drivers/video/fbdev/ps3fb.c
+++ b/drivers/video/fbdev/ps3fb.c
@@ -29,6 +29,7 @@ 
 #include <linux/freezer.h>
 #include <linux/uaccess.h>
 #include <linux/fb.h>
+#include <linux/fbcon.h>
 #include <linux/init.h>
 
 #include <asm/cell-regs.h>
@@ -824,12 +825,12 @@  static int ps3fb_ioctl(struct fb_info *info, unsigned int cmd,
 				var = info->var;
 				fb_videomode_to_var(&var, vmode);
 				console_lock();
-				info->flags |= FBINFO_MISC_USEREVENT;
 				/* Force, in case only special bits changed */
 				var.activate |= FB_ACTIVATE_FORCE;
 				par->new_mode_id = val;
 				retval = fb_set_var(info, &var);
-				info->flags &= ~FBINFO_MISC_USEREVENT;
+				if (!retval)
+					fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);
 				console_unlock();
 			}
 			break;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 3b4b2f0..b11eb02 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -400,8 +400,6 @@  struct fb_tile_ops {
 #define FBINFO_HWACCEL_YPAN		0x2000 /* optional */
 #define FBINFO_HWACCEL_YWRAP		0x4000 /* optional */
 
-#define FBINFO_MISC_USEREVENT          0x10000 /* event request
-						  from userspace */
 #define FBINFO_MISC_TILEBLITTING       0x20000 /* use tile blitting */
 
 /* A driver may set this flag to indicate that it does want a set_par to be