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

[Daniel Vetter]

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

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

> 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