Re: [PATCH] fbmem: prevent potential use-after-free issues with console_lock()

[Hang Zhang]

On Thu, Jan 5, 2023 at 5:25 AM Daniel Vetter <daniel@xxxxxxxx> wrote:
>
> On Thu, 5 Jan 2023 at 11:21, Daniel Vetter <daniel@xxxxxxxx> wrote:
> >
> > Hi Helge
> >
> > On Mon, 2 Jan 2023 at 16:28, Helge Deller <deller@xxxxxx> wrote:
> > >
> > > On 12/30/22 07:35, Hang Zhang wrote:
> > > > In do_fb_ioctl(), user specified "fb_info" can be freed in the callee
> > > > fbcon_get_con2fb_map_ioctl() -> set_con2fb_map() ->
> > > > con2fb_release_oldinfo(), this free operation is protected by
> > > > console_lock() in fbcon_set_con2fb_map_ioctl(), it also results in
> > > > the change of certain states such as "minfo->dead" in matroxfb_remove(),
> > > > so that it can be checked to avoid use-after-free before the use sites
> > > > (e.g., the check at the beginning of matroxfb_ioctl()). However,
> > > > the problem is that the use site is not protected by the same locks
> > > > as for the free operation, e.g., "default" case in do_fb_ioctl()
> > > > can lead to "matroxfb_ioctl()" but it's not protected by console_lock(),
> > > > which can invalidate the aforementioned state set and check in a
> > > > concurrent setting.
> > > >
> > > > Prevent the potential use-after-free issues by protecting the "default"
> > > > case in do_fb_ioctl() with console_lock(), similarly as for many other
> > > > cases like "case FBIOBLANK" and "case FBIOPAN_DISPLAY".
> > > >
> > > > Signed-off-by: Hang Zhang <zh.nvgt@xxxxxxxxx>
> > >
> > > applied to fbdev git tree.
> >
> > The patch above makes no sense at all to me:
> >
> > - fb_info is protected by lock_fb_info and
> > - the lifetime of fb_info is protected by the get/put functions
> > - yes there's the interaction with con2fb, which is protected by
> > console_lock, but the lifetime guarantees are ensured by the device
> > removal
> > - which means any stuff happening in matroxfb_remove is also not a
> > concern here (unless matroxfb completely gets all the device lifetime
> > stuff wrong, but it doesn't look like it's any worse than any of the
> > other fbdev drivers that we haven't recently fixed up due to the
> > takeover issues with firmware drivers
>
> I have also a really hard timing finding the con2fb map use in the
> matroxfb ioctl code, but that just might be that I didn't look
> carefully enough. Maybe that would shed some light on this.
> -Daniel
>
>
> >
> > On the very clear downside this now means we take console_lock for the
> > vblank ioctl (which is a device driver extension for reasons, despite
> > that it's a standard fbdev ioctl), which is no good at all given how
> > console_lock() is a really expensive lock.
> >
> > Unless I'm massively missing something, can you pls push the revert
> > before this lands in Linus' tree?
> >
> > Thanks, Daniel

Hi, Daniel. Thank you for your feedback. We're not developers of the
video subsystem and thus may be short in domain knowledge (e.g., the
performance of console_lock() and the complex lifetime management).
This patch initially intended to bring up the potential use-after-free
issues here to the community - we have performed a best-effort code
review and cannot exclude the possibility based on our understanding.

What we have observed is that the call chain leading to the free site
(do_fb_ioctl()->fbcon_set_con2fb_map_ioctl()->set_con2fb_map()->
con2fb_release_oldinfo()-> ... ->matroxfb_remove()) is only protected
by console_lock() but not lock_fb_info(), while the potential use
site (call chain starts from the default case in do_fb_ioctl()) is
only protected by lock_fb_info() but not console_lock().
We thus propose to add this extra console_lock() to the default case,
which is inspired by the lock protection of many other existing
switch-case terms in the same function.

Since we do not have deep domain knowledge of this subsystem, we will
rely on the developers to make a decision regarding the patch. Thank
you again for your review and help!

Best,
Hang

> >
> > > Thanks,
> > > Helge
> > >
> > > > ---
> > > >   drivers/video/fbdev/core/fbmem.c | 2 ++
> > > >   1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> > > > index 1e70d8c67653..8b1a1527d18a 100644
> > > > --- a/drivers/video/fbdev/core/fbmem.c
> > > > +++ b/drivers/video/fbdev/core/fbmem.c
> > > > @@ -1182,6 +1182,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
> > > >               console_unlock();
> > > >               break;
> > > >       default:
> > > > +             console_lock();
> > > >               lock_fb_info(info);
> > > >               fb = info->fbops;
> > > >               if (fb->fb_ioctl)
> > > > @@ -1189,6 +1190,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
> > > >               else
> > > >                       ret = -ENOTTY;
> > > >               unlock_fb_info(info);
> > > > +             console_unlock();
> > > >       }
> > > >       return ret;
> > > >   }
> > >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch