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

[Hang Zhang]

On Fri, Jan 6, 2023 at 1:59 PM Daniel Vetter <daniel@xxxxxxxx> wrote:
>
> On Thu, Jan 05, 2023 at 01:38:54PM -0500, Hang Zhang wrote:
> > 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!
>
> Can you please elaborate where you've found this use-after-free and how?
> I'm still not understanding how you even got here - this is orthogonal to
> whether the patch is the right fix or not.
> -Daniel

Hi, Daniel. Sure. This issue was initially flagged by our experimental static
code analyzer aiming to find use-after-free issues in the kernel - that's why
we don't have PoC or execution traces here. We deeply understand that
static analyzer can generate false alarms, so we have tried our best and
spent a good amount of time carefully reviewing the related code. We
eventually found that we could not exclude this potential issue based on our
study, so we decided to report this to the community with this tentative fix. As
mentioned, we may be short in domain knowledge, so your input is
highly appreciated. We respect the developer's decision about whether
this is really a problem and whether/how to fix it. However, if you think the
use-after-free is actually not possible, it will be very helpful if you can
elaborate on the reasoning since it will greatly help us improve our
analyzer. Thank you very much for your help!

BTW, if this is worthed a fix and the performance of console_lock() is a
major concern, then I think there may be alternative solutions like adding
a lock_fb_info() to the free call chain - if that's better in performance,
or maybe selectively protect the matroxfb ioctl but not vblank ioctl as you
mentioned.

Thanks,
Hang

>
> >
> > 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
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch