From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Date: Thu, 30 Jul 2020 11:16:19 +0000 Subject: Re: [PATCH] fbmem: pull fbcon_update_vcs() out of fb_set_var() Message-Id: List-Id: References: <1596000620-4075-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable 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 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=C70c88cfd16dcf6e1d3c7f0ab8648b31= 44b5b25e > > Reported-and-tested-by: syzbot > Suggested-by: Daniel Vetter > Signed-off-by: Tetsuo Handa > --- > 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 =3D info->flags; > int ret =3D 0; > u32 activate; > struct fb_var_screeninfo old_var; > @@ -1052,9 +1051,6 @@ static int fb_check_caps(struct fb_info *info, stru= ct fb_var_screeninfo *var, > event.data =3D &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, unsig= ned int cmd, > return -EFAULT; > console_lock(); > lock_fb_info(info); > - info->flags |=3D FBINFO_MISC_USEREVENT; > ret =3D fb_set_var(info, &var); > - info->flags &=3D ~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/cor= e/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 |=3D FB_ACTIVATE_FORCE; > console_lock(); > - fb_info->flags |=3D FBINFO_MISC_USEREVENT; > err =3D fb_set_var(fb_info, var); > - fb_info->flags &=3D ~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, unsign= ed int cmd, > var =3D info->var; > fb_videomode_to_var(&var, vmode); > console_lock(); > - info->flags |=3D FBINFO_MISC_USEREVENT; > /* Force, in case only special bits chang= ed */ > var.activate |=3D FB_ACTIVATE_FORCE; > par->new_mode_id =3D val; > retval =3D fb_set_var(info, &var); > - info->flags &=3D ~FBINFO_MISC_USEREVENT; > + if (!retval) > + fbcon_update_vcs(info, var.activa= te & 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 > > --=20 Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch