From cc67482c9e5f2c80d62f623bcc347c29f9f648e1 Mon Sep 17 00:00:00 2001

From: Hyunwoo Kim <imv4bel@gmail.com>

Date: Thu, 20 Oct 2022 18:15:44 -0700

Subject: [PATCH] fbdev: smscufx: Fix several use-after-free bugs

Git-commit: cc67482c9e5f2c80d62f623bcc347c29f9f648e1

Patch-mainline: v6.1-rc3

References: git-fixes

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>

Acked-by: Takashi Iwai <tiwai@suse.de>

---

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

 }

-- 

2.35.3