fbdev: smscufx: Fix several use-after-free bugs

commit cc67482c9e5f2c80d62f623bcc347c29f9f648e1 upstream.

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/
Cc: <stable@vger.kernel.org>
Signed-off-by: Helge Deller <deller@gmx.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
index 7673db5..5fa3f1e 100644 (file)
--- 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 */
@@ -1116,15 +1115,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,
@@ -1133,14 +1141,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);
@@ -1152,11 +1155,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);
 }
@@ -1168,11 +1166,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);
@@ -1185,6 +1185,8 @@ static int ufx_ops_release(struct fb_info *info, int user)
 
        kref_put(&dev->kref, ufx_free);
 
+       mutex_unlock(&disconnect_mutex);
+
        return 0;
 }
 
@@ -1291,6 +1293,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
@@ -1672,9 +1675,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);
@@ -1747,10 +1747,12 @@ e_nomem:
 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");
 
@@ -1764,12 +1766,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);
 }