| Message ID | 20221021011544.GA339803@ubuntu (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v2] video: fbdev: smscufx: Fixed several use-after-free bugs | expand |
On 10/21/22 03:15, Hyunwoo Kim wrote: > Several types of UAFs can occur when physically removing a USB device. > > Adds ufx_ops_destroy() function to .fb_destroy of fb_ops, and > in this function, there is kref_put() that finally calls ufx_free(). > > This fix prevents multiple UAFs. > > Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com> > Link: https://lore.kernel.org/linux-fbdev/20221011153436.GA4446@ubuntu/ applied. Thanks! Helge > --- > v2: > The v1 patch fixed several UAFs, but "info" was not kfree()d after the device > was removed by calling the framebuffer_release() function from > ufx_free_framebuffer(). > This is because fb_info->count was not 0 at the time the > framebuffer_release() function was called. > > Moved the framebuffer_release() function to the ufx_ops_destory() function. > The ufx_ops_destory() function is a .fb_destory ops that is called > after fb_info->count goes to zero. > --- > drivers/video/fbdev/smscufx.c | 55 +++++++++++++++++++---------------- > 1 file changed, 30 insertions(+), 25 deletions(-) > > diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c > index e65bdc499c23..9343b7a4ac89 100644 > --- a/drivers/video/fbdev/smscufx.c > +++ b/drivers/video/fbdev/smscufx.c > @@ -97,7 +97,6 @@ struct ufx_data { > struct kref kref; > int fb_count; > bool virtualized; /* true when physical usb device not present */ > - struct delayed_work free_framebuffer_work; > atomic_t usb_active; /* 0 = update virtual buffer, but no usb traffic */ > atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */ > u8 *edid; /* null until we read edid from hw or get from sysfs */ > @@ -1117,15 +1116,24 @@ static void ufx_free(struct kref *kref) > { > struct ufx_data *dev = container_of(kref, struct ufx_data, kref); > > - /* this function will wait for all in-flight urbs to complete */ > - if (dev->urbs.count > 0) > - ufx_free_urb_list(dev); > + kfree(dev); > +} > > - pr_debug("freeing ufx_data %p", dev); > +static void ufx_ops_destory(struct fb_info *info) > +{ > + struct ufx_data *dev = info->par; > + int node = info->node; > > - kfree(dev); > + /* Assume info structure is freed after this point */ > + framebuffer_release(info); > + > + pr_debug("fb_info for /dev/fb%d has been freed", node); > + > + /* release reference taken by kref_init in probe() */ > + kref_put(&dev->kref, ufx_free); > } > > + > static void ufx_release_urb_work(struct work_struct *work) > { > struct urb_node *unode = container_of(work, struct urb_node, > @@ -1134,14 +1142,9 @@ static void ufx_release_urb_work(struct work_struct *work) > up(&unode->dev->urbs.limit_sem); > } > > -static void ufx_free_framebuffer_work(struct work_struct *work) > +static void ufx_free_framebuffer(struct ufx_data *dev) > { > - struct ufx_data *dev = container_of(work, struct ufx_data, > - free_framebuffer_work.work); > struct fb_info *info = dev->info; > - int node = info->node; > - > - unregister_framebuffer(info); > > if (info->cmap.len != 0) > fb_dealloc_cmap(&info->cmap); > @@ -1153,11 +1156,6 @@ static void ufx_free_framebuffer_work(struct work_struct *work) > > dev->info = NULL; > > - /* Assume info structure is freed after this point */ > - framebuffer_release(info); > - > - pr_debug("fb_info for /dev/fb%d has been freed", node); > - > /* ref taken in probe() as part of registering framebfufer */ > kref_put(&dev->kref, ufx_free); > } > @@ -1169,11 +1167,13 @@ static int ufx_ops_release(struct fb_info *info, int user) > { > struct ufx_data *dev = info->par; > > + mutex_lock(&disconnect_mutex); > + > dev->fb_count--; > > /* We can't free fb_info here - fbmem will touch it when we return */ > if (dev->virtualized && (dev->fb_count == 0)) > - schedule_delayed_work(&dev->free_framebuffer_work, HZ); > + ufx_free_framebuffer(dev); > > if ((dev->fb_count == 0) && (info->fbdefio)) { > fb_deferred_io_cleanup(info); > @@ -1186,6 +1186,8 @@ static int ufx_ops_release(struct fb_info *info, int user) > > kref_put(&dev->kref, ufx_free); > > + mutex_unlock(&disconnect_mutex); > + > return 0; > } > > @@ -1292,6 +1294,7 @@ static const struct fb_ops ufx_ops = { > .fb_blank = ufx_ops_blank, > .fb_check_var = ufx_ops_check_var, > .fb_set_par = ufx_ops_set_par, > + .fb_destroy = ufx_ops_destory, > }; > > /* Assumes &info->lock held by caller > @@ -1673,9 +1676,6 @@ static int ufx_usb_probe(struct usb_interface *interface, > goto destroy_modedb; > } > > - INIT_DELAYED_WORK(&dev->free_framebuffer_work, > - ufx_free_framebuffer_work); > - > retval = ufx_reg_read(dev, 0x3000, &id_rev); > check_warn_goto_error(retval, "error %d reading 0x3000 register from device", retval); > dev_dbg(dev->gdev, "ID_REV register value 0x%08x", id_rev); > @@ -1748,10 +1748,12 @@ static int ufx_usb_probe(struct usb_interface *interface, > static void ufx_usb_disconnect(struct usb_interface *interface) > { > struct ufx_data *dev; > + struct fb_info *info; > > mutex_lock(&disconnect_mutex); > > dev = usb_get_intfdata(interface); > + info = dev->info; > > pr_debug("USB disconnect starting\n"); > > @@ -1765,12 +1767,15 @@ static void ufx_usb_disconnect(struct usb_interface *interface) > > /* if clients still have us open, will be freed on last close */ > if (dev->fb_count == 0) > - schedule_delayed_work(&dev->free_framebuffer_work, 0); > + ufx_free_framebuffer(dev); > > - /* release reference taken by kref_init in probe() */ > - kref_put(&dev->kref, ufx_free); > + /* this function will wait for all in-flight urbs to complete */ > + if (dev->urbs.count > 0) > + ufx_free_urb_list(dev); > > - /* consider ufx_data freed */ > + pr_debug("freeing ufx_data %p", dev); > + > + unregister_framebuffer(info); > > mutex_unlock(&disconnect_mutex); > }
diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c index e65bdc499c23..9343b7a4ac89 100644 --- a/drivers/video/fbdev/smscufx.c +++ b/drivers/video/fbdev/smscufx.c @@ -97,7 +97,6 @@ struct ufx_data { struct kref kref; int fb_count; bool virtualized; /* true when physical usb device not present */ - struct delayed_work free_framebuffer_work; atomic_t usb_active; /* 0 = update virtual buffer, but no usb traffic */ atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */ u8 *edid; /* null until we read edid from hw or get from sysfs */ @@ -1117,15 +1116,24 @@ static void ufx_free(struct kref *kref) { struct ufx_data *dev = container_of(kref, struct ufx_data, kref); - /* this function will wait for all in-flight urbs to complete */ - if (dev->urbs.count > 0) - ufx_free_urb_list(dev); + kfree(dev); +} - pr_debug("freeing ufx_data %p", dev); +static void ufx_ops_destory(struct fb_info *info) +{ + struct ufx_data *dev = info->par; + int node = info->node; - kfree(dev); + /* Assume info structure is freed after this point */ + framebuffer_release(info); + + pr_debug("fb_info for /dev/fb%d has been freed", node); + + /* release reference taken by kref_init in probe() */ + kref_put(&dev->kref, ufx_free); } + static void ufx_release_urb_work(struct work_struct *work) { struct urb_node *unode = container_of(work, struct urb_node, @@ -1134,14 +1142,9 @@ static void ufx_release_urb_work(struct work_struct *work) up(&unode->dev->urbs.limit_sem); } -static void ufx_free_framebuffer_work(struct work_struct *work) +static void ufx_free_framebuffer(struct ufx_data *dev) { - struct ufx_data *dev = container_of(work, struct ufx_data, - free_framebuffer_work.work); struct fb_info *info = dev->info; - int node = info->node; - - unregister_framebuffer(info); if (info->cmap.len != 0) fb_dealloc_cmap(&info->cmap); @@ -1153,11 +1156,6 @@ static void ufx_free_framebuffer_work(struct work_struct *work) dev->info = NULL; - /* Assume info structure is freed after this point */ - framebuffer_release(info); - - pr_debug("fb_info for /dev/fb%d has been freed", node); - /* ref taken in probe() as part of registering framebfufer */ kref_put(&dev->kref, ufx_free); } @@ -1169,11 +1167,13 @@ static int ufx_ops_release(struct fb_info *info, int user) { struct ufx_data *dev = info->par; + mutex_lock(&disconnect_mutex); + dev->fb_count--; /* We can't free fb_info here - fbmem will touch it when we return */ if (dev->virtualized && (dev->fb_count == 0)) - schedule_delayed_work(&dev->free_framebuffer_work, HZ); + ufx_free_framebuffer(dev); if ((dev->fb_count == 0) && (info->fbdefio)) { fb_deferred_io_cleanup(info); @@ -1186,6 +1186,8 @@ static int ufx_ops_release(struct fb_info *info, int user) kref_put(&dev->kref, ufx_free); + mutex_unlock(&disconnect_mutex); + return 0; } @@ -1292,6 +1294,7 @@ static const struct fb_ops ufx_ops = { .fb_blank = ufx_ops_blank, .fb_check_var = ufx_ops_check_var, .fb_set_par = ufx_ops_set_par, + .fb_destroy = ufx_ops_destory, }; /* Assumes &info->lock held by caller @@ -1673,9 +1676,6 @@ static int ufx_usb_probe(struct usb_interface *interface, goto destroy_modedb; } - INIT_DELAYED_WORK(&dev->free_framebuffer_work, - ufx_free_framebuffer_work); - retval = ufx_reg_read(dev, 0x3000, &id_rev); check_warn_goto_error(retval, "error %d reading 0x3000 register from device", retval); dev_dbg(dev->gdev, "ID_REV register value 0x%08x", id_rev); @@ -1748,10 +1748,12 @@ static int ufx_usb_probe(struct usb_interface *interface, static void ufx_usb_disconnect(struct usb_interface *interface) { struct ufx_data *dev; + struct fb_info *info; mutex_lock(&disconnect_mutex); dev = usb_get_intfdata(interface); + info = dev->info; pr_debug("USB disconnect starting\n"); @@ -1765,12 +1767,15 @@ static void ufx_usb_disconnect(struct usb_interface *interface) /* if clients still have us open, will be freed on last close */ if (dev->fb_count == 0) - schedule_delayed_work(&dev->free_framebuffer_work, 0); + ufx_free_framebuffer(dev); - /* release reference taken by kref_init in probe() */ - kref_put(&dev->kref, ufx_free); + /* this function will wait for all in-flight urbs to complete */ + if (dev->urbs.count > 0) + ufx_free_urb_list(dev); - /* consider ufx_data freed */ + pr_debug("freeing ufx_data %p", dev); + + unregister_framebuffer(info); mutex_unlock(&disconnect_mutex); }
Several types of UAFs can occur when physically removing a USB device. Adds ufx_ops_destroy() function to .fb_destroy of fb_ops, and in this function, there is kref_put() that finally calls ufx_free(). This fix prevents multiple UAFs. Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com> Link: https://lore.kernel.org/linux-fbdev/20221011153436.GA4446@ubuntu/ --- v2: The v1 patch fixed several UAFs, but "info" was not kfree()d after the device was removed by calling the framebuffer_release() function from ufx_free_framebuffer(). This is because fb_info->count was not 0 at the time the framebuffer_release() function was called. Moved the framebuffer_release() function to the ufx_ops_destory() function. The ufx_ops_destory() function is a .fb_destory ops that is called after fb_info->count goes to zero. --- drivers/video/fbdev/smscufx.c | 55 +++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 25 deletions(-)