[PATCH v5] usb: usbip: serialize attach/detach operations
From: Tetsuo Handa
Date: Sun Mar 14 2021 - 07:24:16 EST
The root problem syzbot has found [1] is that usbip module is not using
serialization between attach/detach operations and event_handler().
This results in the following race windows.
(1) two userspace processes can perform attach operation on the same
device by writing the same content to the same attach interface
file
(2) one userspace process can perform detach operation on a device by
writing to detach interface file while the other userspace process
is performing attach operation on that device by writing to attach
interface file
(3) event_handler() kernel workqueue thread can perform detach operation
on a device while some userspace process is still performing attach
operation on that device
What syzbot is reporting is (3), and what commits 46613c9dfa964c0c,
718ad9693e365612 and 9380afd6df70e24e did not take into account is
As soon as one side of {tx,rx} kernel threads starts from attach
operation, that kernel thread is allowed to call
usbip_event_add(USBIP_EH_SHUTDOWN) which in turn allows event_handler()
to call kthread_stop_put() on both {tx,rx} kernel threads via
ud->eh_ops.shutdown(ud), before the other side of {tx,rx} kernel threads
starts from attach operation.
which will be reported as either NULL pointer dereference or use-after-free
bug on the other side of {tx,rx} kernel threads.
Since this race window cannot be closed without introducing serialization,
this patch introduces usbip_event_mutex for serializing attach/detach
operations and event_handler().
[1] https://syzkaller.appspot.com/bug?extid=95ce4b142579611ef0a9
Reported-by: syzbot <syzbot+95ce4b142579611ef0a9@xxxxxxxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
Fixes: 46613c9dfa964c0c ("usbip: fix vudc usbip_sockfd_store races leading to gpf")
Fixes: 718ad9693e365612 ("usbip: fix vhci_hcd attach_store() races leading to gpf")
Fixes: 9380afd6df70e24e ("usbip: fix stub_dev usbip_sockfd_store() races leading to gpf")
---
drivers/usb/usbip/stub_dev.c | 15 +++++++++++++--
drivers/usb/usbip/usbip_common.h | 2 ++
drivers/usb/usbip/usbip_event.c | 15 +++++++++++++++
drivers/usb/usbip/vhci_sysfs.c | 30 ++++++++++++++++++++++++++----
drivers/usb/usbip/vudc_sysfs.c | 16 +++++++++++++---
5 files changed, 69 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
index 8f1de1fbbeed..79ebc9795b4a 100644
--- a/drivers/usb/usbip/stub_dev.c
+++ b/drivers/usb/usbip/stub_dev.c
@@ -39,8 +39,8 @@ static DEVICE_ATTR_RO(usbip_status);
* is used to transfer usbip requests by kernel threads. -1 is a magic number
* by which usbip connection is finished.
*/
-static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *attr,
- const char *buf, size_t count)
+static ssize_t __usbip_sockfd_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
{
struct stub_device *sdev = dev_get_drvdata(dev);
int sockfd = 0;
@@ -132,6 +132,17 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a
spin_unlock_irq(&sdev->ud.lock);
return -EINVAL;
}
+static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ ssize_t ret = usbip_event_lock_killable();
+
+ if (ret)
+ return ret;
+ ret = __usbip_sockfd_store(dev, attr, buf, count);
+ usbip_event_unlock();
+ return ret;
+}
static DEVICE_ATTR_WO(usbip_sockfd);
static struct attribute *usbip_attrs[] = {
diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h
index d60ce17d3dd2..ad7de3773e06 100644
--- a/drivers/usb/usbip/usbip_common.h
+++ b/drivers/usb/usbip/usbip_common.h
@@ -326,6 +326,8 @@ void usbip_stop_eh(struct usbip_device *ud);
void usbip_event_add(struct usbip_device *ud, unsigned long event);
int usbip_event_happened(struct usbip_device *ud);
int usbip_in_eh(struct task_struct *task);
+int usbip_event_lock_killable(void);
+void usbip_event_unlock(void);
static inline int interface_to_busnum(struct usb_interface *interface)
{
diff --git a/drivers/usb/usbip/usbip_event.c b/drivers/usb/usbip/usbip_event.c
index 5d88917c9631..e05b858f346d 100644
--- a/drivers/usb/usbip/usbip_event.c
+++ b/drivers/usb/usbip/usbip_event.c
@@ -58,6 +58,19 @@ static struct usbip_device *get_event(void)
}
static struct task_struct *worker_context;
+static DEFINE_MUTEX(usbip_event_mutex);
+
+int usbip_event_lock_killable(void)
+{
+ return mutex_lock_killable(&usbip_event_mutex);
+}
+EXPORT_SYMBOL_GPL(usbip_event_lock_killable);
+
+void usbip_event_unlock(void)
+{
+ mutex_unlock(&usbip_event_mutex);
+}
+EXPORT_SYMBOL_GPL(usbip_event_unlock);
static void event_handler(struct work_struct *work)
{
@@ -68,6 +81,7 @@ static void event_handler(struct work_struct *work)
}
while ((ud = get_event()) != NULL) {
+ mutex_lock(&usbip_event_mutex);
usbip_dbg_eh("pending event %lx\n", ud->event);
/*
@@ -91,6 +105,7 @@ static void event_handler(struct work_struct *work)
unset_event(ud, USBIP_EH_UNUSABLE);
}
+ mutex_unlock(&usbip_event_mutex);
wake_up(&ud->eh_waitq);
}
}
diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index c4b4256e5dad..d06087e4e29b 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -225,8 +225,8 @@ static int valid_port(__u32 *pdev_nr, __u32 *rhport)
return 1;
}
-static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
- const char *buf, size_t count)
+static ssize_t __detach_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
{
__u32 port = 0, pdev_nr = 0, rhport = 0;
struct usb_hcd *hcd;
@@ -263,6 +263,17 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
return count;
}
+static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ ssize_t ret = usbip_event_lock_killable();
+
+ if (ret)
+ return ret;
+ ret = __detach_store(dev, attr, buf, count);
+ usbip_event_unlock();
+ return ret;
+}
static DEVICE_ATTR_WO(detach);
static int valid_args(__u32 *pdev_nr, __u32 *rhport,
@@ -300,8 +311,8 @@ static int valid_args(__u32 *pdev_nr, __u32 *rhport,
*
* write() returns 0 on success, else negative errno.
*/
-static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
- const char *buf, size_t count)
+static ssize_t __attach_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
{
struct socket *socket;
int sockfd = 0;
@@ -425,6 +436,17 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
return count;
}
+static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ ssize_t ret = usbip_event_lock_killable();
+
+ if (ret)
+ return ret;
+ ret = __attach_store(dev, attr, buf, count);
+ usbip_event_unlock();
+ return ret;
+}
static DEVICE_ATTR_WO(attach);
#define MAX_STATUS_NAME 16
diff --git a/drivers/usb/usbip/vudc_sysfs.c b/drivers/usb/usbip/vudc_sysfs.c
index a3ec39fc6177..3e3e4ef298d2 100644
--- a/drivers/usb/usbip/vudc_sysfs.c
+++ b/drivers/usb/usbip/vudc_sysfs.c
@@ -90,9 +90,8 @@ static ssize_t dev_desc_read(struct file *file, struct kobject *kobj,
}
static BIN_ATTR_RO(dev_desc, sizeof(struct usb_device_descriptor));
-static ssize_t usbip_sockfd_store(struct device *dev,
- struct device_attribute *attr,
- const char *in, size_t count)
+static ssize_t __usbip_sockfd_store(struct device *dev, struct device_attribute *attr,
+ const char *in, size_t count)
{
struct vudc *udc = (struct vudc *) dev_get_drvdata(dev);
int rv;
@@ -219,6 +218,17 @@ static ssize_t usbip_sockfd_store(struct device *dev,
return ret;
}
+static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *attr,
+ const char *in, size_t count)
+{
+ ssize_t ret = usbip_event_lock_killable();
+
+ if (ret)
+ return ret;
+ ret = __usbip_sockfd_store(dev, attr, in, count);
+ usbip_event_unlock();
+ return ret;
+}
static DEVICE_ATTR_WO(usbip_sockfd);
static ssize_t usbip_status_show(struct device *dev,
--
2.18.4