diff mbox series

char: xillybus: Fix use-after-free in xillyusb_open()

Message ID 20221022175404.GA375335@ubuntu
State New
Headers
Series char: xillybus: Fix use-after-free in xillyusb_open() |

Commit Message

Hyunwoo Kim Oct. 22, 2022, 5:54 p.m. UTC 
  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 xillyusb_open() function and
the xillyusb_disconnect() function, which may eventually result in UAF.

So, add a mutex to the xillyusb_open() and xillyusb_disconnect()
functions to avoid race contidion.

Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
---
 drivers/char/xillybus/xillyusb.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

Comments

Eli Billauer Oct. 23, 2022, 2:19 p.m. UTC | #1 
Hello, Hyunwoo.

> 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 xillyusb_open() function and
> the xillyusb_disconnect() function, which may eventually result in UAF.

Thanks a lot for pointing that out. In fact, this reveals two problems 
in the existing code:

(1) unit->private_data is accessed after the mutex has been released in 
xillybus_find_inode(), so there's no guarantee that it will be valid. 
This is what the test caught. This can however be fixed just by moving 
the release of the lock a few rows down.

(2) xillyusb_open() accesses @xdev without ensuring that it won't get freed.

Both of these two issues have a negligible probability of causing a 
visible problem, but this must be fixed, of course.

> 
> So, add a mutex to the xillyusb_open() and xillyusb_disconnect()
> functions to avoid race contidion.

I'm not very fond of this solution, partially because this mutex 
protects code and not data (There's this "Lock data, not code" rule, see 
[1]). Also, xillyusb_disconnect() can take a significant time to run, 
during which xillybus_open() for another (unrelated and still connected) 
XillyUSB device has to wait. I guess this demonstrates why protecting 
code with a mutex is considered bad practice.

Besides, there are already three mechanisms in place for preventing 
premature release of memory:

(1) @unit_mutex in xillybus_class.c, which protects @unit_list.
(2) @kref inside struct xillyusb_dev (xillyusb.c), which protects the 
structure it resides in.
(3) @error inside struct xillyusb_dev, which prevents xillybus_open() 
from opening a file that belongs to a device that is about to be released.

It's now apparent that they're not working well enough. Rather than 
adding another mutex, the existing mechanisms should be fixed.  Would 
you like to do this, or should I?

Thanks again,
   Eli

[1] Documentation/kernel-hacking/locking.rst in the kernel tree
Hyunwoo Kim Oct. 23, 2022, 2:26 p.m. UTC | #2 
On Sun, Oct 23, 2022 at 05:19:40PM +0300, Eli Billauer wrote:
> Hello, Hyunwoo.
> 
> > 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 xillyusb_open() function and
> > the xillyusb_disconnect() function, which may eventually result in UAF.
> 
> Thanks a lot for pointing that out. In fact, this reveals two problems in
> the existing code:
> 
> (1) unit->private_data is accessed after the mutex has been released in
> xillybus_find_inode(), so there's no guarantee that it will be valid. This
> is what the test caught. This can however be fixed just by moving the
> release of the lock a few rows down.
> 
> (2) xillyusb_open() accesses @xdev without ensuring that it won't get freed.
> 
> Both of these two issues have a negligible probability of causing a visible
> problem, but this must be fixed, of course.
> 
> > 
> > So, add a mutex to the xillyusb_open() and xillyusb_disconnect()
> > functions to avoid race contidion.
> 
> I'm not very fond of this solution, partially because this mutex protects
> code and not data (There's this "Lock data, not code" rule, see [1]). Also,
> xillyusb_disconnect() can take a significant time to run, during which
> xillybus_open() for another (unrelated and still connected) XillyUSB device
> has to wait. I guess this demonstrates why protecting code with a mutex is
> considered bad practice.
> 
> Besides, there are already three mechanisms in place for preventing
> premature release of memory:
> 
> (1) @unit_mutex in xillybus_class.c, which protects @unit_list.
> (2) @kref inside struct xillyusb_dev (xillyusb.c), which protects the
> structure it resides in.
> (3) @error inside struct xillyusb_dev, which prevents xillybus_open() from
> opening a file that belongs to a device that is about to be released.
> 
> It's now apparent that they're not working well enough. Rather than adding
> another mutex, the existing mechanisms should be fixed.  Would you like to
> do this, or should I?

Thanks for the detailed feedback.
It's probably better for you to work on it.


Regards,
Hyunwoo Kim.
Eli Billauer Oct. 24, 2022, 7:10 a.m. UTC | #3 
On 23/10/2022 17:26, Hyunwoo Kim wrote:
> It's probably better for you to work on it.

OK. I'll be back with a patch soon.

Regards,
    Eli
diff mbox series

Patch

diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
index 39bcbfd908b4..d615f74dcf36 100644
--- a/drivers/char/xillybus/xillyusb.c
+++ b/drivers/char/xillybus/xillyusb.c
@@ -63,6 +63,7 @@  static const struct usb_device_id xillyusb_table[] = {
 };
 
 MODULE_DEVICE_TABLE(usb, xillyusb_table);
+static DEFINE_MUTEX(disconnect_mutex);
 
 struct xillyusb_dev;
 
@@ -1237,9 +1238,13 @@  static int xillyusb_open(struct inode *inode, struct file *filp)
 	int rc;
 	int index;
 
+	mutex_lock(&disconnect_mutex);
+
 	rc = xillybus_find_inode(inode, (void **)&xdev, &index);
-	if (rc)
+	if (rc) {
+		mutex_unlock(&disconnect_mutex);
 		return rc;
+	}
 
 	chan = &xdev->channels[index];
 	filp->private_data = chan;
@@ -1379,6 +1384,8 @@  static int xillyusb_open(struct inode *inode, struct file *filp)
 			request_read_anything(chan, OPCODE_SET_PUSH);
 	}
 
+	mutex_unlock(&disconnect_mutex);
+
 	return 0;
 
 unfifo:
@@ -1410,10 +1417,14 @@  static int xillyusb_open(struct inode *inode, struct file *filp)
 
 	kref_put(&xdev->kref, cleanup_dev);
 
+	mutex_unlock(&disconnect_mutex);
+
 	return rc;
 
 unmutex_fail:
 	mutex_unlock(&chan->lock);
+	mutex_unlock(&disconnect_mutex);
+
 	return rc;
 }
 
@@ -1698,6 +1709,8 @@  static int xillyusb_release(struct inode *inode, struct file *filp)
 	struct xillyusb_dev *xdev = chan->xdev;
 	int rc_read = 0, rc_write = 0;
 
+	mutex_lock(&disconnect_mutex);
+
 	if (filp->f_mode & FMODE_READ) {
 		struct xillyfifo *in_fifo = chan->in_fifo;
 
@@ -1760,6 +1773,8 @@  static int xillyusb_release(struct inode *inode, struct file *filp)
 
 	kref_put(&xdev->kref, cleanup_dev);
 
+	mutex_unlock(&disconnect_mutex);
+
 	return rc_read ? rc_read : rc_write;
 }
 
@@ -2172,6 +2187,8 @@  static void xillyusb_disconnect(struct usb_interface *interface)
 	int rc;
 	int i;
 
+	mutex_lock(&disconnect_mutex);
+
 	xillybus_cleanup_chrdev(xdev, &interface->dev);
 
 	/*
@@ -2228,6 +2245,8 @@  static void xillyusb_disconnect(struct usb_interface *interface)
 	xdev->dev = NULL;
 
 	kref_put(&xdev->kref, cleanup_dev);
+
+	mutex_unlock(&disconnect_mutex);
 }
 
 static struct usb_driver xillyusb_driver = {