Thread (8 messages) 8 messages, 2 authors, 2020-08-04

Re: [PATCH] fbmem: pull fbcon_update_vcs() out of fb_set_var()

flat view

From: Daniel Vetter <hidden>
Date: 2020-07-30 11:16:35
Also in: dri-devel, lkml

On Thu, Jul 30, 2020 at 12:47 AM Tetsuo Handa
[off-list ref] wrote:
quoted hunk ↗ jump to hunk
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Ç0c88cfd16dcf6e1d3c7f0ab8648b3144b5b25e

Reported-and-tested-by: syzbot <redacted>
Suggested-by: Daniel Vetter <redacted>
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
quoted hunk ↗ jump to hunk
                                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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help