[PATCH v2] video: fbdev: smscufx: Fix use-after-free in ufx_ops_open()

Subsystems: framebuffer layer, smsc ufx6000 and ufx7000 usb to vga driver, the rest

DORMANTno replies

3 messages, 2 authors, 2022-09-25 · open the first message on its own page

[PATCH v2] video: fbdev: smscufx: Fix use-after-free in ufx_ops_open()

From: Hyunwoo Kim <hidden>
Date: 2022-09-25 16:04:29

A race condition may occur if the user physically removes the
USB device while calling open() for this device node.

This is a race condition between the ufx_ops_open() function and
the ufx_usb_disconnect() function, which may eventually result in UAF.

So, add a mutex to the ufx_ops_open() and ufx_usb_disconnect() functions
to avoid race contidion of krefs.

Signed-off-by: Hyunwoo Kim <redacted>
---
 drivers/video/fbdev/smscufx.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
index d7aa5511c361..02a22e31e9c0 100644
--- a/drivers/video/fbdev/smscufx.c
+++ b/drivers/video/fbdev/smscufx.c
@@ -137,6 +137,8 @@ static int ufx_submit_urb(struct ufx_data *dev, struct urb * urb, size_t len);
 static int ufx_alloc_urb_list(struct ufx_data *dev, int count, size_t size);
 static void ufx_free_urb_list(struct ufx_data *dev);
 
+static DEFINE_MUTEX(disconnect_mutex);
+
 /* reads a control register */
 static int ufx_reg_read(struct ufx_data *dev, u32 index, u32 *data)
 {
@@ -1065,15 +1067,21 @@ static int ufx_ops_open(struct fb_info *info, int user)
 {
 	struct ufx_data *dev = info->par;
 
+	mutex_lock(&disconnect_mutex);
+
 	/* fbcon aggressively connects to first framebuffer it finds,
 	 * preventing other clients (X) from working properly. Usually
 	 * not what the user wants. Fail by default with option to enable. */
-	if (user == 0 && !console)
+	if (user == 0 && !console) {
+		mutex_unlock(&disconnect_mutex);
 		return -EBUSY;
+	}
 
 	/* If the USB device is gone, we don't accept new opens */
-	if (dev->virtualized)
+	if (dev->virtualized) {
+		mutex_unlock(&disconnect_mutex);
 		return -ENODEV;
+	}
 
 	dev->fb_count++;
 
@@ -1097,6 +1105,8 @@ static int ufx_ops_open(struct fb_info *info, int user)
 	pr_debug("open /dev/fb%d user=%d fb_info=%p count=%d",
 		info->node, user, info, dev->fb_count);
 
+	mutex_unlock(&disconnect_mutex);
+
 	return 0;
 }
 
@@ -1741,6 +1751,8 @@ static void ufx_usb_disconnect(struct usb_interface *interface)
 {
 	struct ufx_data *dev;
 
+	mutex_lock(&disconnect_mutex);
+
 	dev = usb_get_intfdata(interface);
 
 	pr_debug("USB disconnect starting\n");
@@ -1761,6 +1773,8 @@ static void ufx_usb_disconnect(struct usb_interface *interface)
 	kref_put(&dev->kref, ufx_free);
 
 	/* consider ufx_data freed */
+
+	mutex_unlock(&disconnect_mutex);
 }
 
 static struct usb_driver ufx_driver = {
-- 
2.25.1

Re: [PATCH v2] video: fbdev: smscufx: Fix use-after-free in ufx_ops_open()

From: Helge Deller <deller@gmx.de>
Date: 2022-09-25 13:09:55

On 9/25/22 14:32, Hyunwoo Kim wrote:
quoted hunk
A race condition may occur if the user physically removes the
USB device while calling open() for this device node.

This is a race condition between the ufx_ops_open() function and
the ufx_usb_disconnect() function, which may eventually result in UAF.

So, add a mutex to the ufx_ops_open() and ufx_usb_disconnect() functions
to avoid race contidion of krefs.

Signed-off-by: Hyunwoo Kim <redacted>
---
  drivers/video/fbdev/smscufx.c | 18 ++++++++++++++++--
  1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
index d7aa5511c361..02a22e31e9c0 100644
--- a/drivers/video/fbdev/smscufx.c
+++ b/drivers/video/fbdev/smscufx.c
@@ -137,6 +137,8 @@ static int ufx_submit_urb(struct ufx_data *dev, struct urb * urb, size_t len);
  static int ufx_alloc_urb_list(struct ufx_data *dev, int count, size_t size);
  static void ufx_free_urb_list(struct ufx_data *dev);

+static DEFINE_MUTEX(disconnect_mutex);
+
  /* reads a control register */
  static int ufx_reg_read(struct ufx_data *dev, u32 index, u32 *data)
  {
@@ -1065,15 +1067,21 @@ static int ufx_ops_open(struct fb_info *info, int user)
  {
  	struct ufx_data *dev = info->par;

+	mutex_lock(&disconnect_mutex);
+
  	/* fbcon aggressively connects to first framebuffer it finds,
  	 * preventing other clients (X) from working properly. Usually
  	 * not what the user wants. Fail by default with option to enable. */
-	if (user == 0 && !console)
+	if (user == 0 && !console) {
+		mutex_unlock(&disconnect_mutex);
  		return -EBUSY;
it seems user and console don't need to be protected by the lock.
Does it make sense to move them out of the lock?

Helge

Re: [PATCH v2] video: fbdev: smscufx: Fix use-after-free in ufx_ops_open()

From: Hyunwoo Kim <hidden>
Date: 2022-09-25 16:03:28

On Sun, Sep 25, 2022 at 03:09:47PM +0200, Helge Deller wrote:
it seems user and console don't need to be protected by the lock.
Does it make sense to move them out of the lock?
you're right. Since both variables are not related to a race condition, 
it is better to get them out of the lock.

I will submit a v3 patch.


Best Regards,
Hyunwoo Kim.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help