Re: [PATCH 4/6] usbip: fix stub_dev usbip_sockfd_store() races leading to gpf

From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
To: Shuah Khan <skhan@linuxfoundation.org>,
	shuah@kernel.org, valentina.manea.m@gmail.com,
	gregkh@linuxfoundation.org
Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/6] usbip: fix stub_dev usbip_sockfd_store() races leading to gpf
Date: Wed, 10 Mar 2021 19:38:13 +0900	[thread overview]
Message-ID: <3dab66dc-2981-bc88-a370-4b3178dfd390@i-love.sakura.ne.jp> (raw)
In-Reply-To: <0e8d26dc-d99c-bcaa-1e95-0b5d0e0e535b@linuxfoundation.org>

On 2021/03/10 11:07, Shuah Khan wrote:
> On 3/9/21 6:02 PM, Tetsuo Handa wrote:
>> On 2021/03/10 9:29, Shuah Khan wrote:
>>>> It is not a large grain lock. Since event_handler() is exclusively executed, this lock
>>>> does _NOT_ block event_handler() unless attach/detach operations run concurrently.
>>>>
>>>
>>> event handler queues the events. It shouldn't be blocked by attach
>>> and detach. The events could originate for various reasons during
>>> the host and vhci operations. I don't like using this lock for
>>> attach and detach.
>>
>> How can attach/detach deadlock event_handler()?
>> event_handler() calls e.g. vhci_shutdown_connection() via ud->eh_ops.shutdown(ud).
>> vhci_shutdown_connection() e.g. waits for termination of tx/rx threads via kthread_stop_put().
>> event_handler() is already blocked by detach operation.
>> How it can make situation worse to wait for creation of tx/rx threads in attach operation?
>>
> 
> event_lock shouldn't be held during event ops. usbip_event_add()
> uses it to add events. Protecting shutdown path needs a different
> approach.

I can't understand what you are talking about.

event_lock is defined as

  static DEFINE_SPINLOCK(event_lock);

in drivers/usb/usbip/usbip_event.c and usbip_event_add() uses it like

  spin_lock_irqsave(&event_lock, flags);
  spin_unlock_irqrestore(&event_lock, flags);

, but so what? I know event_lock spinlock cannot be taken when calling

  ud->eh_ops.shutdown(ud);
  ud->eh_ops.reset(ud);
  ud->eh_ops.unusable(ud);

in event_handler() because e.g. vhci_shutdown_connection() can sleep.

What my [PATCH v4 01/12] is suggesting is usbip_event_mutex which is defined as

  static DEFINE_MUTEX(usbip_event_mutex);

in drivers/usb/usbip/usbip_event.c and held when calling

  ud->eh_ops.shutdown(ud);
  ud->eh_ops.reset(ud);
  ud->eh_ops.unusable(ud);

in event_handler(). Since event_handler() is executed exclusively via
singlethreaded workqueue, "event_handler() holds usbip_event_mutex" itself
does not introduce a new lock dependency.

My question is, how holding usbip_event_mutex can cause deadlock if
usbip_sockfd_store()/attach_store()/detach_store() also hold usbip_event_mutex .

kthread_create() is essentially several kmalloc(GFP_KERNEL) where event_handler()
cannot interfere unless event_handler() serves as a memory reclaiming function.
My question again. What functions might sleep and hold locks other than
kthread_create() for tx/rx threads?

Your answer to my question (if you identified such dependency) will go into
patch shown bottom which replaces my [PATCH v4 01/12]-[PATCH v4 04/12] patches.

> 
> In any case, do you have comments on this patch which doesn't even
> touch vhci driver?

I'm just replying to your [PATCH 4/6] because all your [PATCH 4/6]-[PATCH 6/6]
patches have the same oversight.

> 
> I understand you are identifying additional race condition that
> the vhci patches in this series might not fix. That doesn't mean
> that these patches aren't valid.
> 
> Do you have any comments specific to the patches in this series?

Your [PATCH 5/6] is still racy regarding rh_port_connect() versus rh_port_disconnect().
As soon as you call wake_up_process(), rh_port_disconnect() can be called before
rh_port_connect() is called. Since you don't like serializing event_handler() and
usbip_sockfd_store()/attach_store()/detach_store() using usbip_event_mutex, my
patch shown below which uses attach_detach_lock mutex cannot close such race window.
Ideally, wake_up_process() should be deferred to after releasing attach_detach_lock,
but please answer to my question first.



From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Wed, 10 Mar 2021 18:31:54 +0900

syzbot is reporting a NULL pointer dereference at sock_sendmsg() [1], for
lack of serialization between attach_store() and event_handler() causes
vhci_shutdown_connection() to observe vdev->ud.tcp_tx == NULL while
vdev->ud.tcp_socket != NULL. Please read the reference link for details of
this race window.

While usbip module exclusively runs event_handler(), usbip module has never
thought the possibility that multiple userspace processes can concurrently
call usbip_sockfd_store()/attach_store()/detach_store(). As a result, there
is a TOCTTOU bug in these functions regarding ud->status value which can
result in similar crashes like [1].

Simplest way would be to run all of
event_handler()/usbip_sockfd_store()/attach_store()/detach_store() functions
exclusively using a global mutex. But Shuah Khan does not want to share a
global mutex between these functions, for ...[please fill in this part]... .

Therefore, this patch introduces a per-submodule local mutex which closes
race window within usbip_sockfd_store() and attach_store()/detach_store().

This local mutex cannot close race window between event_handler()
and usbip_sockfd_store()/attach_store()/detach_store(), for calling
wake_up_process() allows kernel thread to call
usbip_event_add(VDEV_EVENT_DOWN) and event_handler() will start
detach operation before this local mutex is released.
The race window via usbip_event_add(VDEV_EVENT_DOWN) from kernel thread
will be addressed by subsequent patches in this series, by splitting
kthread_get_run() into kthread_create() and wake_up_process(), and
deferring wake_up_process() to immediately before releasing this local
mutex.

[1] https://syzkaller.appspot.com/bug?extid=95ce4b142579611ef0a9

References: https://lkml.kernel.org/r/676d4518-0faa-9fab-15db-0db8d216d7fb@i-love.sakura.ne.jp

---
 drivers/usb/usbip/stub_dev.c   | 16 ++++++++++++++--
 drivers/usb/usbip/vhci_sysfs.c | 31 +++++++++++++++++++++++++++----
 drivers/usb/usbip/vudc_sysfs.c | 16 ++++++++++++++--
 3 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
index 2305d425e6c9..66c8f2385f3a 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;
@@ -104,6 +104,18 @@ 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)
+{
+	static DEFINE_MUTEX(attach_detach_lock);
+	ssize_t ret = mutex_lock_killable(&attach_detach_lock);
+
+	if (ret)
+		return ret;
+	ret = __usbip_sockfd_store(dev, attr, buf, count);
+	mutex_unlock(&attach_detach_lock);
+	return ret;
+}
 static DEVICE_ATTR_WO(usbip_sockfd);
 
 static struct attribute *usbip_attrs[] = {
diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index 96e5371dc335..777aba9b9115 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -17,6 +17,7 @@
 #include "vhci.h"
 
 /* TODO: refine locking ?*/
+static DEFINE_MUTEX(attach_detach_lock);
 
 /*
  * output example:
@@ -225,8 +226,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 +264,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 = mutex_lock_killable(&attach_detach_lock);
+
+	if (ret)
+		return ret;
+	ret = __detach_store(dev, attr, buf, count);
+	mutex_unlock(&attach_detach_lock);
+	return ret;
+}
 static DEVICE_ATTR_WO(detach);
 
 static int valid_args(__u32 *pdev_nr, __u32 *rhport,
@@ -300,8 +312,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;
@@ -396,6 +408,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 = mutex_lock_killable(&attach_detach_lock);
+
+	if (ret)
+		return ret;
+	ret = __attach_store(dev, attr, buf, count);
+	mutex_unlock(&attach_detach_lock);
+	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..b14876dd2c0c 100644
--- a/drivers/usb/usbip/vudc_sysfs.c
+++ b/drivers/usb/usbip/vudc_sysfs.c
@@ -90,8 +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;
@@ -184,6 +184,18 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a
 
 	return ret;
 }
+static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *attr,
+				  const char *in, size_t count)
+{
+	static DEFINE_MUTEX(attach_detach_lock);
+	ssize_t ret = mutex_lock_killable(&attach_detach_lock);
+
+	if (ret)
+		return ret;
+	ret = __usbip_sockfd_store(dev, attr, in, count);
+	mutex_unlock(&attach_detach_lock);
+	return ret;
+}
 static DEVICE_ATTR_WO(usbip_sockfd);
 
 static ssize_t usbip_status_show(struct device *dev,
-- 
2.18.4

