Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <2e5cbdfe-f6cd-d24f-9785-55176af6c975@gmail.com>
Date:   Wed, 26 Oct 2022 11:52:40 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
From:   Eli Billauer <eli.billauer@gmail.com>
Subject: [PATCH] char: xillybus: Prevent use-after-free due to race condition
To:     gregkh@linuxfoundation.org
Cc:     arnd@arndb.de, linux-kernel@vger.kernel.org,
        linux-usb@vger.kernel.org, imv4bel@gmail.com,
        Eli Billauer <eli.billauer@gmail.com>
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

xillybus_find_inode() is called by xillybus_open() and xillyusb_open()
to translate the inode's major and minor into a pointer to a relevant
data structure and an index.

But with xillyusb_open(), the data structure can be freed by
xillyusb_disconnect() during an unintentional time gap between the
release of the mutex that is taken by xillybus_find_inode() and the
mutex that is subsequently taken by xillyusb_open().

To fix this, xillybus_find_inode() supplies the pointer to the mutex that
it has locked (when returning success), so xillyusb_open() releases this
mutex only after obtaining the mutex that is specific to a device file.
This ensures that xillyusb_disconnect() won't release anything that is in
use.

This manipulation of mutexes poses no risk for a deadlock, because in
all usage scenarios, @unit_mutex (which is locked by xillybus_find_inode)
is always taken when no other mutex is locked. Hence a consistent locking
order is guaranteed.

xillybus_open() unlocks this mutex immediately, as this driver doesn't
support hot unplugging anyhow.

Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---
 drivers/char/xillybus/xillybus_class.c | 8 +++++---
 drivers/char/xillybus/xillybus_class.h | 2 ++
 drivers/char/xillybus/xillybus_core.c  | 6 +++++-
 drivers/char/xillybus/xillyusb.c       | 4 +++-
 4 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/char/xillybus/xillybus_class.c b/drivers/char/xillybus/xillybus_class.c
index 0f238648dcfe..c846dc3ed225 100644
--- a/drivers/char/xillybus/xillybus_class.c
+++ b/drivers/char/xillybus/xillybus_class.c
@@ -211,6 +211,7 @@ void xillybus_cleanup_chrdev(void *private_data,
 EXPORT_SYMBOL(xillybus_cleanup_chrdev);
 
 int xillybus_find_inode(struct inode *inode,
+			struct mutex **to_unlock,
 			void **private_data, int *index)
 {
 	int minor = iminor(inode);
@@ -227,13 +228,14 @@ int xillybus_find_inode(struct inode *inode,
 			break;
 		}
 
-	mutex_unlock(&unit_mutex);
-
-	if (!unit)
+	if (!unit) {
+		mutex_unlock(&unit_mutex);
 		return -ENODEV;
+	}
 
 	*private_data = unit->private_data;
 	*index = minor - unit->lowest_minor;
+	*to_unlock = &unit_mutex;
 
 	return 0;
 }
diff --git a/drivers/char/xillybus/xillybus_class.h b/drivers/char/xillybus/xillybus_class.h
index 5dbfdfc95c65..7cf89ac8bb32 100644
--- a/drivers/char/xillybus/xillybus_class.h
+++ b/drivers/char/xillybus/xillybus_class.h
@@ -12,6 +12,7 @@
 #include <linux/device.h>
 #include <linux/fs.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 
 int xillybus_init_chrdev(struct device *dev,
 			 const struct file_operations *fops,
@@ -25,6 +26,7 @@ void xillybus_cleanup_chrdev(void *private_data,
 			     struct device *dev);
 
 int xillybus_find_inode(struct inode *inode,
+			struct mutex **to_unlock, /* Mutex to release */
 			void **private_data, int *index);
 
 #endif /* __XILLYBUS_CLASS_H */
diff --git a/drivers/char/xillybus/xillybus_core.c b/drivers/char/xillybus/xillybus_core.c
index 11b7c4749274..7fd35f055310 100644
--- a/drivers/char/xillybus/xillybus_core.c
+++ b/drivers/char/xillybus/xillybus_core.c
@@ -1431,11 +1431,15 @@ static int xillybus_open(struct inode *inode, struct file *filp)
 	struct xilly_endpoint *endpoint;
 	struct xilly_channel *channel;
 	int index;
+	struct mutex *to_unlock; /* Mutex locked by xillybus_find_inode() */
 
-	rc = xillybus_find_inode(inode, (void **)&endpoint, &index);
+	rc = xillybus_find_inode(inode, &to_unlock,
+				 (void **)&endpoint, &index);
 	if (rc)
 		return rc;
 
+	mutex_unlock(to_unlock);
+
 	if (endpoint->fatal_error)
 		return -EIO;
 
diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
index 39bcbfd908b4..63e94d935067 100644
--- a/drivers/char/xillybus/xillyusb.c
+++ b/drivers/char/xillybus/xillyusb.c
@@ -1236,8 +1236,9 @@ static int xillyusb_open(struct inode *inode, struct file *filp)
 	struct xillyusb_endpoint *out_ep = NULL;
 	int rc;
 	int index;
+	struct mutex *to_unlock; /* Mutex locked by xillybus_find_inode() */
 
-	rc = xillybus_find_inode(inode, (void **)&xdev, &index);
+	rc = xillybus_find_inode(inode, &to_unlock, (void **)&xdev, &index);
 	if (rc)
 		return rc;
 
@@ -1245,6 +1246,7 @@ static int xillyusb_open(struct inode *inode, struct file *filp)
 	filp->private_data = chan;
 
 	mutex_lock(&chan->lock);
+	mutex_unlock(to_unlock);
 
 	rc = -ENODEV;
 
-- 
2.17.1