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 > > > 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
