Re: general protection fault in tomoyo_socket_sendmsg_permission
From: Tetsuo Handa
Date: Sat Feb 13 2021 - 05:11:26 EST
Greg, will you queue https://lkml.kernel.org/r/20210205135707.4574-1-penguin-kernel@xxxxxxxxxxxxxxxxxxx (which can
close a report which is wasting syzbot's resource with 5300+ crashes) for 5.12 ? The change shown below will be
too large to test before merge window for 5.12 opens.
The patch for fixing "general protection fault in tomoyo_socket_sendmsg_permission" will kill kthread_get_run().
Closing frequently crashing bug now is the better.
On 2021/02/11 22:40, Tetsuo Handa wrote:
> I guess that we need to serialize attach operation and reset/detach operations, for
> it seems there is a race window that might result in "general protection fault in
> tomoyo_socket_sendmsg_permission". Details follows...
Here is untested diff that is expected to be complete.
(1) Handle kthread_create() failure (which avoids "KASAN: null-ptr-deref Write in vhci_shutdown_connection")
by grouping socket lookup, SOCK_STREAM check and kthread_get_run() into usbip_prepare_threads() function.
(2) Serialize usbip_sockfd_store(), detach_store(), attach_store(), usbip_sockfd_store() and
ud->eh_ops.shutdown()/ud->eh_ops.reset()/ud->eh_ops.unusable() operations using usbip_store_mutex mutex
(which avoids "general protection fault in tomoyo_socket_sendmsg_permission").
(3) Don't reset ud->tcp_socket to NULL in vhci_device_reset(). Since tx_thread/rx_thread depends on
ud->tcp_socket != NULL whereas tcp_socket and tx_thread/rx_thread are assigned at the same time,
it is never safe to reset only ud->tcp_socket from ud->eh_ops.reset(). And actually nobody is
calling ud->eh_ops.reset() without ud->eh_ops.shutdown().
(4) usbip_sockfd_store() must perform {sdev,udc}->ud.status != SDEV_ST_AVAILABLE && {sdev,udc}->ud.status = SDEV_ST_USED
exclusively, or multiple tx_thread/rx_thread can be created when concurrently called. Although (2) will already
serialize this action, (1) will make it possible to perform within one spinlock section.
drivers/usb/usbip/stub_dev.c | 56 ++++++++++++++++++--------------
drivers/usb/usbip/usbip_common.c | 55 +++++++++++++++++++++++++++++++
drivers/usb/usbip/usbip_common.h | 13 ++++++++
drivers/usb/usbip/usbip_event.c | 9 +++++
drivers/usb/usbip/vhci_hcd.c | 7 +---
drivers/usb/usbip/vhci_sysfs.c | 50 ++++++++++++++++++++--------
drivers/usb/usbip/vudc_sysfs.c | 50 ++++++++++++++++------------
7 files changed, 175 insertions(+), 65 deletions(-)
diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
index 2305d425e6c9..522d58826049 100644
--- a/drivers/usb/usbip/stub_dev.c
+++ b/drivers/usb/usbip/stub_dev.c
@@ -39,12 +39,11 @@ 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;
- struct socket *socket;
int rv;
if (!sdev) {
@@ -57,7 +56,12 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a
return -EINVAL;
if (sockfd != -1) {
- int err;
+ struct usbip_thread_info uti;
+ int err = usbip_prepare_threads(&uti, &sdev->ud, sockfd,
+ stub_tx_loop, "stub_tx", stub_rx_loop, "stub_rx");
+
+ if (err)
+ return err;
dev_info(dev, "stub up\n");
@@ -65,44 +69,46 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a
if (sdev->ud.status != SDEV_ST_AVAILABLE) {
dev_err(dev, "not ready\n");
- goto err;
+ spin_unlock_irq(&sdev->ud.lock);
+ usbip_unprepare_threads(&uti);
+ return -EINVAL;
}
- socket = sockfd_lookup(sockfd, &err);
- if (!socket)
- goto err;
-
- sdev->ud.tcp_socket = socket;
+ sdev->ud.tcp_socket = uti.tcp_socket;
sdev->ud.sockfd = sockfd;
-
- spin_unlock_irq(&sdev->ud.lock);
-
- sdev->ud.tcp_rx = kthread_get_run(stub_rx_loop, &sdev->ud,
- "stub_rx");
- sdev->ud.tcp_tx = kthread_get_run(stub_tx_loop, &sdev->ud,
- "stub_tx");
-
- spin_lock_irq(&sdev->ud.lock);
+ sdev->ud.tcp_rx = uti.tcp_rx;
+ sdev->ud.tcp_tx = uti.tcp_tx;
sdev->ud.status = SDEV_ST_USED;
spin_unlock_irq(&sdev->ud.lock);
+ wake_up_process(sdev->ud.tcp_rx);
+ wake_up_process(sdev->ud.tcp_tx);
} else {
dev_info(dev, "stub down\n");
spin_lock_irq(&sdev->ud.lock);
- if (sdev->ud.status != SDEV_ST_USED)
- goto err;
-
+ if (sdev->ud.status != SDEV_ST_USED) {
+ spin_unlock_irq(&sdev->ud.lock);
+ return -EINVAL;
+ }
spin_unlock_irq(&sdev->ud.lock);
usbip_event_add(&sdev->ud, SDEV_EVENT_DOWN);
}
return count;
+}
-err:
- 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;
+
+ if (mutex_lock_killable(&usbip_store_mutex))
+ return -EINTR;
+ ret = __usbip_sockfd_store(dev, attr, buf, count);
+ mutex_unlock(&usbip_store_mutex);
+ return ret;
}
static DEVICE_ATTR_WO(usbip_sockfd);
diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
index 2ab99244bc31..f86e49fc89a7 100644
--- a/drivers/usb/usbip/usbip_common.c
+++ b/drivers/usb/usbip/usbip_common.c
@@ -748,6 +748,61 @@ int usbip_recv_xbuff(struct usbip_device *ud, struct urb *urb)
}
EXPORT_SYMBOL_GPL(usbip_recv_xbuff);
+int usbip_prepare_threads(struct usbip_thread_info *uti,
+ struct usbip_device *ud, int sockfd,
+ int (*tx_fn)(void *data), const char *tx_name,
+ int (*rx_fn)(void *data), const char *rx_name)
+{
+ int err;
+ struct socket *socket;
+ struct task_struct *tx;
+ struct task_struct *rx;
+
+ /* Extract socket from fd. */
+ socket = sockfd_lookup(sockfd, &err);
+ if (!socket)
+ return -EINVAL;
+ /* Verify that this is a stream socket. */
+ if (socket->type != SOCK_STREAM) {
+ err = -EINVAL;
+ goto out_socket;
+ }
+ /* Create threads for this socket. */
+ rx = kthread_create(rx_fn, ud, rx_name);
+ if (IS_ERR(rx)) {
+ err = PTR_ERR(rx);
+ goto out_socket;
+ }
+ tx = kthread_create(tx_fn, ud, tx_name);
+ if (IS_ERR(tx)) {
+ err = PTR_ERR(rx);
+ goto out_rx;
+ }
+ uti->tcp_socket = socket;
+ get_task_struct(rx);
+ uti->tcp_rx = rx;
+ get_task_struct(rx);
+ uti->tcp_tx = tx;
+ return 0;
+ out_rx:
+ kthread_stop(rx);
+ out_socket:
+ sockfd_put(socket);
+ return err;
+}
+EXPORT_SYMBOL_GPL(usbip_prepare_threads);
+
+void usbip_unprepare_threads(struct usbip_thread_info *uti)
+{
+ kthread_stop_put(uti->tcp_rx);
+ uti->tcp_rx = NULL;
+ kthread_stop_put(uti->tcp_rx);
+ uti->tcp_rx = NULL;
+ sockfd_put(uti->tcp_socket);
+ uti->tcp_socket = NULL;
+}
+EXPORT_SYMBOL_GPL(usbip_unprepare_threads);
+
static int __init usbip_core_init(void)
{
return usbip_init_eh();
diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h
index 8be857a4fa13..fba1827e719b 100644
--- a/drivers/usb/usbip/usbip_common.h
+++ b/drivers/usb/usbip/usbip_common.h
@@ -309,6 +309,19 @@ void usbip_header_correct_endian(struct usbip_header *pdu, int send);
struct usbip_iso_packet_descriptor*
usbip_alloc_iso_desc_pdu(struct urb *urb, ssize_t *bufflen);
+struct usbip_thread_info {
+ struct socket *tcp_socket;
+ struct task_struct *tcp_tx;
+ struct task_struct *tcp_rx;
+};
+
+extern struct mutex usbip_store_mutex;
+int usbip_prepare_threads(struct usbip_thread_info *uti,
+ struct usbip_device *ud, int sockfd,
+ int (*tx_fn)(void *data), const char *tx_name,
+ int (*rx_fn)(void *data), const char *rx_name);
+void usbip_unprepare_threads(struct usbip_thread_info *uti);
+
/* some members of urb must be substituted before. */
int usbip_recv_iso(struct usbip_device *ud, struct urb *urb);
void usbip_pad_iso(struct usbip_device *ud, struct urb *urb);
diff --git a/drivers/usb/usbip/usbip_event.c b/drivers/usb/usbip/usbip_event.c
index 5d88917c9631..d4f7dd366102 100644
--- a/drivers/usb/usbip/usbip_event.c
+++ b/drivers/usb/usbip/usbip_event.c
@@ -19,6 +19,9 @@ struct usbip_event {
static DEFINE_SPINLOCK(event_lock);
static LIST_HEAD(event_list);
+DEFINE_MUTEX(usbip_store_mutex);
+EXPORT_SYMBOL_GPL(usbip_store_mutex);
+
static void set_event(struct usbip_device *ud, unsigned long event)
{
unsigned long flags;
@@ -75,19 +78,25 @@ static void event_handler(struct work_struct *work)
* Shutdown the device.
*/
if (ud->event & USBIP_EH_SHUTDOWN) {
+ mutex_lock(&usbip_store_mutex);
ud->eh_ops.shutdown(ud);
+ mutex_unlock(&usbip_store_mutex);
unset_event(ud, USBIP_EH_SHUTDOWN);
}
/* Reset the device. */
if (ud->event & USBIP_EH_RESET) {
+ mutex_lock(&usbip_store_mutex);
ud->eh_ops.reset(ud);
+ mutex_unlock(&usbip_store_mutex);
unset_event(ud, USBIP_EH_RESET);
}
/* Mark the device as unusable. */
if (ud->event & USBIP_EH_UNUSABLE) {
+ mutex_lock(&usbip_store_mutex);
ud->eh_ops.unusable(ud);
+ mutex_unlock(&usbip_store_mutex);
unset_event(ud, USBIP_EH_UNUSABLE);
}
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 3209b5ddd30c..eaae6fa8cff4 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -1072,12 +1072,7 @@ static void vhci_device_reset(struct usbip_device *ud)
usb_put_dev(vdev->udev);
vdev->udev = NULL;
-
- if (ud->tcp_socket) {
- sockfd_put(ud->tcp_socket);
- ud->tcp_socket = NULL;
- ud->sockfd = -1;
- }
+ ud->sockfd = -1;
ud->status = VDEV_ST_NULL;
spin_unlock_irqrestore(&ud->lock, flags);
diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index be37aec250c2..5f282dc54626 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;
+
+ if (mutex_lock_killable(&usbip_store_mutex))
+ return -EINTR;
+ ret = __detach_store(dev, attr, buf, count);
+ mutex_unlock(&usbip_store_mutex);
+ return ret;
+}
static DEVICE_ATTR_WO(detach);
static int valid_args(__u32 *pdev_nr, __u32 *rhport,
@@ -300,10 +311,10 @@ 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;
+ struct usbip_thread_info uti;
int sockfd = 0;
__u32 port = 0, pdev_nr = 0, rhport = 0, devid = 0, speed = 0;
struct usb_hcd *hcd;
@@ -347,10 +358,10 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
else
vdev = &vhci->vhci_hcd_hs->vdev[rhport];
- /* Extract socket from fd. */
- socket = sockfd_lookup(sockfd, &err);
- if (!socket)
- return -EINVAL;
+ err = usbip_prepare_threads(&uti, &vdev->ud, sockfd,
+ vhci_tx_loop, "vhci_tx", vhci_rx_loop, "vhci_rx");
+ if (err)
+ return err;
/* now need lock until setting vdev status as used */
@@ -363,7 +374,7 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
spin_unlock(&vdev->ud.lock);
spin_unlock_irqrestore(&vhci->lock, flags);
- sockfd_put(socket);
+ usbip_unprepare_threads(&uti);
dev_err(dev, "port %d already used\n", rhport);
/*
@@ -381,20 +392,33 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
vdev->devid = devid;
vdev->speed = speed;
vdev->ud.sockfd = sockfd;
- vdev->ud.tcp_socket = socket;
+ vdev->ud.tcp_socket = uti.tcp_socket;
+ vdev->ud.tcp_rx = uti.tcp_rx;
+ vdev->ud.tcp_rx = uti.tcp_tx;
vdev->ud.status = VDEV_ST_NOTASSIGNED;
spin_unlock(&vdev->ud.lock);
spin_unlock_irqrestore(&vhci->lock, flags);
/* end the lock */
- vdev->ud.tcp_rx = kthread_get_run(vhci_rx_loop, &vdev->ud, "vhci_rx");
- vdev->ud.tcp_tx = kthread_get_run(vhci_tx_loop, &vdev->ud, "vhci_tx");
+ wake_up_process(vdev->ud.tcp_rx);
+ wake_up_process(vdev->ud.tcp_tx);
rh_port_connect(vdev, speed);
return count;
}
+static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ ssize_t ret;
+
+ if (mutex_lock_killable(&usbip_store_mutex))
+ return -EINTR;
+ ret = __attach_store(dev, attr, buf, count);
+ mutex_unlock(&usbip_store_mutex);
+ 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 100f680c572a..de5df1c7f32f 100644
--- a/drivers/usb/usbip/vudc_sysfs.c
+++ b/drivers/usb/usbip/vudc_sysfs.c
@@ -90,14 +90,13 @@ 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;
int sockfd = 0;
- int err;
- struct socket *socket;
+ struct usbip_thread_info uti = { };
unsigned long flags;
int ret;
@@ -109,6 +108,14 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a
dev_err(dev, "no device");
return -ENODEV;
}
+
+ if (sockfd != -1) {
+ ret = usbip_prepare_threads(&uti, &udc->ud, sockfd,
+ v_tx_loop, "vudc_tx", v_rx_loop, "vudc_rx");
+ if (ret)
+ return ret;
+ }
+
spin_lock_irqsave(&udc->lock, flags);
/* Don't export what we don't have */
if (!udc->driver || !udc->pullup) {
@@ -130,28 +137,17 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a
ret = -EINVAL;
goto unlock_ud;
}
-
- socket = sockfd_lookup(sockfd, &err);
- if (!socket) {
- dev_err(dev, "failed to lookup sock");
- ret = -EINVAL;
- goto unlock_ud;
- }
-
- udc->ud.tcp_socket = socket;
-
+ udc->ud.tcp_socket = uti.tcp_socket;
+ udc->ud.tcp_rx = uti.tcp_rx;
+ udc->ud.tcp_tx = uti.tcp_tx;
+ udc->ud.status = SDEV_ST_USED;
spin_unlock_irq(&udc->ud.lock);
spin_unlock_irqrestore(&udc->lock, flags);
- udc->ud.tcp_rx = kthread_get_run(&v_rx_loop,
- &udc->ud, "vudc_rx");
- udc->ud.tcp_tx = kthread_get_run(&v_tx_loop,
- &udc->ud, "vudc_tx");
+ wake_up_process(udc->ud.tcp_rx);
+ wake_up_process(udc->ud.tcp_tx);
spin_lock_irqsave(&udc->lock, flags);
- spin_lock_irq(&udc->ud.lock);
- udc->ud.status = SDEV_ST_USED;
- spin_unlock_irq(&udc->ud.lock);
ktime_get_ts64(&udc->start_time);
v_start_timer(udc);
@@ -181,7 +177,19 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a
spin_unlock_irq(&udc->ud.lock);
unlock:
spin_unlock_irqrestore(&udc->lock, flags);
+ if (uti.tcp_socket)
+ usbip_unprepare_threads(&uti);
+ return ret;
+}
+static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *attr,
+ const char *in, size_t count)
+{
+ ssize_t ret;
+ if (mutex_lock_killable(&usbip_store_mutex))
+ return -EINTR;
+ ret = __usbip_sockfd_store(dev, attr, in, count);
+ mutex_unlock(&usbip_store_mutex);
return ret;
}
static DEVICE_ATTR_WO(usbip_sockfd);
--
2.18.4