dri-devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] vt: Handle recursion in vc_do_resize().
@ 2020-07-29  5:30 Tetsuo Handa
  2020-07-29  8:18 ` Daniel Vetter
  0 siblings, 1 reply; 8+ messages in thread
From: Tetsuo Handa @ 2020-07-29  5:30 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, Tetsuo Handa,
	linux-kernel, dri-devel, syzbot

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 read before calling resize_screen().

Minimal fix might be to read vc->vc_{rows,size_row} after resize_screen().
A different fix might be to forbid recursive vc_do_resize() request.
I can't tell which fix is the better.

But since I guess that new_cols == vc->vc_cols && new_rows == vc->vc_rows
check could become true after returning from resize_screen(), and I assume
that not calling clear_selection() when resize_screen() will return error
is harmless, let's redo the check by moving resize_screen() earlier.

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

Reported-by: syzbot <syzbot+c37a14770d51a085a520@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 drivers/tty/vt/vt.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 42d8c67..952a067 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -1217,7 +1217,24 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc,
 
 	if (new_cols == vc->vc_cols && new_rows == vc->vc_rows)
 		return 0;
+	if (new_screen_size > KMALLOC_MAX_SIZE || !new_screen_size)
+		return -EINVAL;
 
+	/*
+	 * Since fbcon_resize() from resize_screen() can recurse into
+	 * this function via fb_set_var(), handle recursion now.
+	 */
+	err = resize_screen(vc, new_cols, new_rows, user);
+	if (err)
+		return err;
+	/* Reload values in case recursion changed vc->vc_{cols,rows}. */
+	new_cols = (cols ? cols : vc->vc_cols);
+	new_rows = (lines ? lines : vc->vc_rows);
+	new_row_size = new_cols << 1;
+	new_screen_size = new_row_size * new_rows;
+
+	if (new_cols == vc->vc_cols && new_rows == vc->vc_rows)
+		return 0;
 	if (new_screen_size > KMALLOC_MAX_SIZE || !new_screen_size)
 		return -EINVAL;
 	newscreen = kzalloc(new_screen_size, GFP_USER);
@@ -1238,13 +1255,6 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc,
 	old_rows = vc->vc_rows;
 	old_row_size = vc->vc_size_row;
 
-	err = resize_screen(vc, new_cols, new_rows, user);
-	if (err) {
-		kfree(newscreen);
-		vc_uniscr_free(new_uniscr);
-		return err;
-	}
-
 	vc->vc_rows = new_rows;
 	vc->vc_cols = new_cols;
 	vc->vc_size_row = new_row_size;
-- 
1.8.3.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] vt: Handle recursion in vc_do_resize().
  2020-07-29  5:30 [PATCH] vt: Handle recursion in vc_do_resize() Tetsuo Handa
@ 2020-07-29  8:18 ` Daniel Vetter
  2020-07-29 22:46   ` [PATCH] fbmem: pull fbcon_update_vcs() out of fb_set_var() Tetsuo Handa
  0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2020-07-29  8:18 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: Linux Fbdev development list, Bartlomiej Zolnierkiewicz,
	Greg Kroah-Hartman, Linux Kernel Mailing List, dri-devel,
	Jiri Slaby, syzbot

On Wed, Jul 29, 2020 at 8:58 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> 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 read before calling resize_screen().
>
> Minimal fix might be to read vc->vc_{rows,size_row} after resize_screen().
> A different fix might be to forbid recursive vc_do_resize() request.
> I can't tell which fix is the better.
>
> But since I guess that new_cols == vc->vc_cols && new_rows == vc->vc_rows
> check could become true after returning from resize_screen(), and I assume
> that not calling clear_selection() when resize_screen() will return error
> is harmless, let's redo the check by moving resize_screen() earlier.
>
> [1] https://syzkaller.appspot.com/bug?id=c70c88cfd16dcf6e1d3c7f0ab8648b3144b5b25e
>
> Reported-by: syzbot <syzbot+c37a14770d51a085a520@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Ok, I have actual insight on this one here, and I'm pretty sure this
isn't the fix. Looking at the syzkaller splat we have a recursion of
the form

fb_ioctl -> fb_set_var -> fbcon_update_vcs -> fbcon_resize -> fb_set_var

Which isn't supposed to be happening. I've dug around recently in
fbcon code, and this is a fairly common issue: You can update fbcon
state both from fb_ioctl, but also from the vc side. To avoid the
above recursion problems the code is using FBINFO_MISC_USEREVENT, and
should only set that from fb_ioctl entry points. That's all fairly
fragile, so I've done a bit of reworking, e.g.

commit de29ae5c092bd9a5360cfabf174b0f783248d278
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Tue May 28 11:02:56 2019 +0200

    fbmem: pull fbcon_fb_blanked out of fb_blank

as an example.

I think doing the same for fb_set_var, i.e. only calling
fbcon_update_vcs for the 3 callers that want it, should fix this
recursion. I think that's the much more robust fix instead of trying
to paper over the fallout of this recursion here and everywhere else.

Can you look into reworking your patch like that?

Cheers, Daniel

> ---
>  drivers/tty/vt/vt.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 42d8c67..952a067 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -1217,7 +1217,24 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc,
>
>         if (new_cols == vc->vc_cols && new_rows == vc->vc_rows)
>                 return 0;
> +       if (new_screen_size > KMALLOC_MAX_SIZE || !new_screen_size)
> +               return -EINVAL;
>
> +       /*
> +        * Since fbcon_resize() from resize_screen() can recurse into
> +        * this function via fb_set_var(), handle recursion now.
> +        */
> +       err = resize_screen(vc, new_cols, new_rows, user);
> +       if (err)
> +               return err;
> +       /* Reload values in case recursion changed vc->vc_{cols,rows}. */
> +       new_cols = (cols ? cols : vc->vc_cols);
> +       new_rows = (lines ? lines : vc->vc_rows);
> +       new_row_size = new_cols << 1;
> +       new_screen_size = new_row_size * new_rows;
> +
> +       if (new_cols == vc->vc_cols && new_rows == vc->vc_rows)
> +               return 0;
>         if (new_screen_size > KMALLOC_MAX_SIZE || !new_screen_size)
>                 return -EINVAL;
>         newscreen = kzalloc(new_screen_size, GFP_USER);
> @@ -1238,13 +1255,6 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc,
>         old_rows = vc->vc_rows;
>         old_row_size = vc->vc_size_row;
>
> -       err = resize_screen(vc, new_cols, new_rows, user);
> -       if (err) {
> -               kfree(newscreen);
> -               vc_uniscr_free(new_uniscr);
> -               return err;
> -       }
> -
>         vc->vc_rows = new_rows;
>         vc->vc_cols = new_cols;
>         vc->vc_size_row = new_row_size;
> --
> 1.8.3.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] fbmem: pull fbcon_update_vcs() out of fb_set_var()
  2020-07-29  8:18 ` Daniel Vetter
@ 2020-07-29 22:46   ` Tetsuo Handa
  2020-07-30 10:47     ` [PATCH v2] " Tetsuo Handa
  2020-07-30 11:16     ` [PATCH] " Daniel Vetter
  0 siblings, 2 replies; 8+ messages in thread
From: Tetsuo Handa @ 2020-07-29 22:46 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Linux Fbdev development list, Bartlomiej Zolnierkiewicz,
	Greg Kroah-Hartman, Linux Kernel Mailing List, dri-devel,
	Jiri Slaby, syzbot

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>
---
 drivers/video/fbdev/core/fbmem.c   | 8 ++------
 drivers/video/fbdev/core/fbsysfs.c | 4 ++--
 drivers/video/fbdev/ps3fb.c        | 4 ++--
 include/linux/fb.h                 | 2 --
 4 files changed, 6 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..4b4a99f 100644
--- a/drivers/video/fbdev/ps3fb.c
+++ b/drivers/video/fbdev/ps3fb.c
@@ -824,12 +824,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


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] fbmem: pull fbcon_update_vcs() out of fb_set_var()
  2020-07-29 22:46   ` [PATCH] fbmem: pull fbcon_update_vcs() out of fb_set_var() Tetsuo Handa
@ 2020-07-30 10:47     ` Tetsuo Handa
  2020-08-04  5:38       ` daniel
  2020-07-30 11:16     ` [PATCH] " Daniel Vetter
  1 sibling, 1 reply; 8+ messages in thread
From: Tetsuo Handa @ 2020-07-30 10:47 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Linux Fbdev development list, Bartlomiej Zolnierkiewicz,
	Greg Kroah-Hartman, Linux Kernel Mailing List, dri-devel,
	Jiri Slaby, syzbot

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(-)

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fbmem: pull fbcon_update_vcs() out of fb_set_var()
  2020-07-29 22:46   ` [PATCH] fbmem: pull fbcon_update_vcs() out of fb_set_var() Tetsuo Handa
  2020-07-30 10:47     ` [PATCH v2] " Tetsuo Handa
@ 2020-07-30 11:16     ` Daniel Vetter
  2020-07-30 11:27       ` Tetsuo Handa
  1 sibling, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2020-07-30 11:16 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: Linux Fbdev development list, Bartlomiej Zolnierkiewicz,
	Greg Kroah-Hartman, Linux Kernel Mailing List, dri-devel,
	Jiri Slaby, syzbot

On Thu, Jul 30, 2020 at 12:47 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> 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>
> ---
>  drivers/video/fbdev/core/fbmem.c   | 8 ++------
>  drivers/video/fbdev/core/fbsysfs.c | 4 ++--
>  drivers/video/fbdev/ps3fb.c        | 4 ++--
>  include/linux/fb.h                 | 2 --
>  4 files changed, 6 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..4b4a99f 100644
> --- a/drivers/video/fbdev/ps3fb.c
> +++ b/drivers/video/fbdev/ps3fb.c
> @@ -824,12 +824,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);

Patch looks good, except ... does this compile? fbcon_update_vcs is
defined in fbcon.h, and that doesn't seem to be included here ...
Maybe what we want is an fb_set_var_ioctl in fbmem.c so that the fbcon
interaction is a bit better hidden (but that's a bikeshed, feel free
to ignore). Also I have no idea what trickery you need to compile-test
ps3fb, that's why I'm asking :-)
-Daniel

>                                 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
>
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fbmem: pull fbcon_update_vcs() out of fb_set_var()
  2020-07-30 11:16     ` [PATCH] " Daniel Vetter
@ 2020-07-30 11:27       ` Tetsuo Handa
  2020-07-30 11:35         ` Daniel Vetter
  0 siblings, 1 reply; 8+ messages in thread
From: Tetsuo Handa @ 2020-07-30 11:27 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Linux Fbdev development list, Bartlomiej Zolnierkiewicz,
	Greg Kroah-Hartman, Linux Kernel Mailing List, dri-devel,
	Jiri Slaby, syzbot

On 2020/07/30 20:16, Daniel Vetter wrote:
> Patch looks good, except ... does this compile? fbcon_update_vcs is
> defined in fbcon.h, and that doesn't seem to be included here ...
> Maybe what we want is an fb_set_var_ioctl in fbmem.c so that the fbcon
> interaction is a bit better hidden (but that's a bikeshed, feel free
> to ignore). Also I have no idea what trickery you need to compile-test
> ps3fb, that's why I'm asking :-)

Right. I didn't prepare environment for compiling powerpc kernel.
Kernel test robot found it and I already posted V2 patch as
https://lkml.kernel.org/r/075b7e37-3278-cd7d-31ab-c5073cfa8e92@i-love.sakura.ne.jp .
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fbmem: pull fbcon_update_vcs() out of fb_set_var()
  2020-07-30 11:27       ` Tetsuo Handa
@ 2020-07-30 11:35         ` Daniel Vetter
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2020-07-30 11:35 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: Linux Fbdev development list, Bartlomiej Zolnierkiewicz,
	Greg Kroah-Hartman, Linux Kernel Mailing List, dri-devel,
	Jiri Slaby, syzbot

On Thu, Jul 30, 2020 at 1:27 PM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> On 2020/07/30 20:16, Daniel Vetter wrote:
> > Patch looks good, except ... does this compile? fbcon_update_vcs is
> > defined in fbcon.h, and that doesn't seem to be included here ...
> > Maybe what we want is an fb_set_var_ioctl in fbmem.c so that the fbcon
> > interaction is a bit better hidden (but that's a bikeshed, feel free
> > to ignore). Also I have no idea what trickery you need to compile-test
> > ps3fb, that's why I'm asking :-)
>
> Right. I didn't prepare environment for compiling powerpc kernel.
> Kernel test robot found it and I already posted V2 patch as
> https://lkml.kernel.org/r/075b7e37-3278-cd7d-31ab-c5073cfa8e92@i-love.sakura.ne.jp .

Excellent. It's still stuck in a queue somewhere and hasn't reached my
inbox, I'll queue it up as soon as I have it.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] fbmem: pull fbcon_update_vcs() out of fb_set_var()
  2020-07-30 10:47     ` [PATCH v2] " Tetsuo Handa
@ 2020-08-04  5:38       ` daniel
  0 siblings, 0 replies; 8+ messages in thread
From: daniel @ 2020-08-04  5:38 UTC (permalink / raw)
  Cc: Linux Fbdev development list, Bartlomiej Zolnierkiewicz,
	Greg Kroah-Hartman, Linux Kernel Mailing List, dri-devel,
	Jiri Slaby, syzbot

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-29  5:30 [PATCH] vt: Handle recursion in vc_do_resize() Tetsuo Handa
2020-07-29  8:18 ` Daniel Vetter
2020-07-29 22:46   ` [PATCH] fbmem: pull fbcon_update_vcs() out of fb_set_var() Tetsuo Handa
2020-07-30 10:47     ` [PATCH v2] " Tetsuo Handa
2020-08-04  5:38       ` daniel
2020-07-30 11:16     ` [PATCH] " Daniel Vetter
2020-07-30 11:27       ` Tetsuo Handa
2020-07-30 11:35         ` Daniel Vetter

dri-devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dri-devel/0 dri-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dri-devel dri-devel/ https://lore.kernel.org/dri-devel \
		dri-devel@lists.freedesktop.org
	public-inbox-index dri-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.freedesktop.lists.dri-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git