KASAN: null-ptr-deref Write in vhci_shutdown_connection

KASAN: null-ptr-deref Write in vhci_shutdown_connection

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    5e60366d Merge tag 'fallthrough-fixes-clang-5.11-rc1' of g..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=13f05613500000
kernel config:  https://syzkaller.appspot.com/x/.config?x=503d0089cd701d6d
dashboard link: https://syzkaller.appspot.com/bug?extid=a93fba6d384346a761e3
compiler:       gcc (GCC) 10.1.0-syz 20200507
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=14d0d8c5500000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1058e41f500000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com

vhci_hcd: stop threads
vhci_hcd: release socket
vhci_hcd: disconnect device
==================================================================
BUG: KASAN: null-ptr-deref in instrument_atomic_read_write include/linux/instrumented.h:101 [inline]
BUG: KASAN: null-ptr-deref in atomic_fetch_add_relaxed include/asm-generic/atomic-instrumented.h:142 [inline]
BUG: KASAN: null-ptr-deref in __refcount_add include/linux/refcount.h:193 [inline]
BUG: KASAN: null-ptr-deref in __refcount_inc include/linux/refcount.h:250 [inline]
BUG: KASAN: null-ptr-deref in refcount_inc include/linux/refcount.h:267 [inline]
BUG: KASAN: null-ptr-deref in get_task_struct include/linux/sched/task.h:102 [inline]
BUG: KASAN: null-ptr-deref in kthread_stop+0x90/0x760 kernel/kthread.c:591
Write of size 4 at addr 0000000000000024 by task kworker/u4:2/46

CPU: 0 PID: 46 Comm: kworker/u4:2 Not tainted 5.10.0-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: usbip_event event_handler
Call Trace:
 __dump_stack lib/dump_stack.c:79 [inline]
 dump_stack+0x107/0x163 lib/dump_stack.c:120
 __kasan_report mm/kasan/report.c:549 [inline]
 kasan_report.cold+0x5/0x37 mm/kasan/report.c:562
 check_memory_region_inline mm/kasan/generic.c:186 [inline]
 check_memory_region+0x13d/0x180 mm/kasan/generic.c:192
 instrument_atomic_read_write include/linux/instrumented.h:101 [inline]
 atomic_fetch_add_relaxed include/asm-generic/atomic-instrumented.h:142 [inline]
 __refcount_add include/linux/refcount.h:193 [inline]
 __refcount_inc include/linux/refcount.h:250 [inline]
 refcount_inc include/linux/refcount.h:267 [inline]
 get_task_struct include/linux/sched/task.h:102 [inline]
 kthread_stop+0x90/0x760 kernel/kthread.c:591
 vhci_shutdown_connection+0x17f/0x340 drivers/usb/usbip/vhci_hcd.c:1021
 event_handler+0x1f0/0x4f0 drivers/usb/usbip/usbip_event.c:78
 process_one_work+0x98d/0x1630 kernel/workqueue.c:2275
 worker_thread+0x64c/0x1120 kernel/workqueue.c:2421
 kthread+0x3b1/0x4a0 kernel/kthread.c:292
 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:296
==================================================================
Kernel panic - not syncing: panic_on_warn set ...
CPU: 0 PID: 46 Comm: kworker/u4:2 Tainted: G    B             5.10.0-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: usbip_event event_handler
Call Trace:
 __dump_stack lib/dump_stack.c:79 [inline]
 dump_stack+0x107/0x163 lib/dump_stack.c:120
 panic+0x343/0x77f kernel/panic.c:231
 end_report+0x58/0x5e mm/kasan/report.c:106
 __kasan_report mm/kasan/report.c:552 [inline]
 kasan_report.cold+0xd/0x37 mm/kasan/report.c:562
 check_memory_region_inline mm/kasan/generic.c:186 [inline]
 check_memory_region+0x13d/0x180 mm/kasan/generic.c:192
 instrument_atomic_read_write include/linux/instrumented.h:101 [inline]
 atomic_fetch_add_relaxed include/asm-generic/atomic-instrumented.h:142 [inline]
 __refcount_add include/linux/refcount.h:193 [inline]
 __refcount_inc include/linux/refcount.h:250 [inline]
 refcount_inc include/linux/refcount.h:267 [inline]
 get_task_struct include/linux/sched/task.h:102 [inline]
 kthread_stop+0x90/0x760 kernel/kthread.c:591
 vhci_shutdown_connection+0x17f/0x340 drivers/usb/usbip/vhci_hcd.c:1021
 event_handler+0x1f0/0x4f0 drivers/usb/usbip/usbip_event.c:78
 process_one_work+0x98d/0x1630 kernel/workqueue.c:2275
 worker_thread+0x64c/0x1120 kernel/workqueue.c:2421
 kthread+0x3b1/0x4a0 kernel/kthread.c:292
 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:296
Kernel Offset: disabled
Rebooting in 86400 seconds..


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

[PATCH] usb: usbip: fix error handling of kthread_get_run()

[Tetsuo Handa]

syzbot is reporting a crash at vhci_shutdown_connection() [1]. It turned
out that it was not a NULL pointer dereference but an ERR_PTR(-EINTR)
pointer dereference, for kthread_create() became killable due to
commit 786235eeba0e1e85 ("kthread: make kthread_create() killable").

When SIGKILLed while attach_store() is calling kthread_get_run(),
ERR_PTR(-EINTR) is stored into vdev->ud.tcp_{rx,tx}, and then
kthread_stop_put() is called on vdev->ud.tcp_{rx,tx} from
vhci_shutdown_connection() because vdev->ud.tcp_{rx,tx} != NULL.

Prior to commit 9720b4bc76a83807 ("staging/usbip: convert to kthread"),
"current" pointer is assigned to vdev->ud.tcp_{rx,tx} by usbip_thread()
kernel thread, and hence vdev->ud.tcp_{rx,tx} != NULL meant a valid task
pointer. Therefore, we should make kthread_get_run() return NULL when
kthread_create() failed.

[1] https://syzkaller.appspot.com/bug?id=c21c07f3d51769405e8efc027bdb927515dcc7d6

Reported-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com>
Reported-by: syzbot <syzbot+bf1a360e305ee719e364@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Fixes: 9720b4bc76a83807 ("staging/usbip: convert to kthread")
Cc: Arnd Bergmann <arnd@arndb.de>
---
 drivers/usb/usbip/usbip_common.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h
index 8be857a4fa13..a7c68097aa1d 100644
--- a/drivers/usb/usbip/usbip_common.h
+++ b/drivers/usb/usbip/usbip_common.h
@@ -286,6 +286,8 @@ struct usbip_device {
 	if (!IS_ERR(__k)) {						   \
 		get_task_struct(__k);					   \
 		wake_up_process(__k);					   \
+	} else {							   \
+		__k = NULL;						   \
 	}								   \
 	__k;								   \
 })
-- 
2.18.4

Re: [PATCH] usb: usbip: fix error handling of kthread_get_run()

[Shuah Khan]

On 2/5/21 6:57 AM, Tetsuo Handa wrote:
> syzbot is reporting a crash at vhci_shutdown_connection() [1]. It turned
> out that it was not a NULL pointer dereference but an ERR_PTR(-EINTR)
> pointer dereference, for kthread_create() became killable due to
> commit 786235eeba0e1e85 ("kthread: make kthread_create() killable").
> 
> When SIGKILLed while attach_store() is calling kthread_get_run(),
> ERR_PTR(-EINTR) is stored into vdev->ud.tcp_{rx,tx}, and then
> kthread_stop_put() is called on vdev->ud.tcp_{rx,tx} from
> vhci_shutdown_connection() because vdev->ud.tcp_{rx,tx} != NULL.
> 
> Prior to commit 9720b4bc76a83807 ("staging/usbip: convert to kthread"),
> "current" pointer is assigned to vdev->ud.tcp_{rx,tx} by usbip_thread()
> kernel thread, and hence vdev->ud.tcp_{rx,tx} != NULL meant a valid task
> pointer. Therefore, we should make kthread_get_run() return NULL when
> kthread_create() failed.
> 
> [1] https://syzkaller.appspot.com/bug?id=c21c07f3d51769405e8efc027bdb927515dcc7d6
> 
> Reported-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com>
> Reported-by: syzbot <syzbot+bf1a360e305ee719e364@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Fixes: 9720b4bc76a83807 ("staging/usbip: convert to kthread")
> Cc: Arnd Bergmann <arnd@arndb.de>
> ---
>   drivers/usb/usbip/usbip_common.h | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h
> index 8be857a4fa13..a7c68097aa1d 100644
> --- a/drivers/usb/usbip/usbip_common.h
> +++ b/drivers/usb/usbip/usbip_common.h
> @@ -286,6 +286,8 @@ struct usbip_device {
>   	if (!IS_ERR(__k)) {						   \
>   		get_task_struct(__k);					   \
>   		wake_up_process(__k);					   \
> +	} else {							   \
> +		__k = NULL;						   \
>   	}								   \
>   	__k;								   \
>   })
> 

Good find. For this fix to be complete, you will have to add checks
for kthread_get_run() NULL return in attach_store() and
usbip_sockfd_store() routines in stub_dev.c and vudc_sysfs.c

thanks,
-- Shuah

Re: [PATCH] usb: usbip: fix error handling of kthread_get_run()

[Tetsuo Handa]

On 2021/02/06 1:27, Shuah Khan wrote:
> Good find. For this fix to be complete, you will have to add checks
> for kthread_get_run() NULL return in attach_store() and
> usbip_sockfd_store() routines in stub_dev.c and vudc_sysfs.c

Initially I thought that the cleaner fix is to get kthread_create() out of kthread_get_run()
( the drivers/usb/usbip/vhci_sysfs.c portion in
https://syzkaller.appspot.com/x/patch.diff?x=16c3c090d00000 ) so that we can undo
kthread_create() via kthread_stop(). But I found that such fix makes little sense because
it is possible that SIGKILL is delivered between vhci_rx_loop() and vhci_tx_loop() have
started and before leaving attach_store().

Since the code prior to "staging/usbip: convert to kthread" was already capable of surviving
such race condition, this patch should be already good enough for sending to stable kernels.
Of course, since kthread_create() may return -ENOMEM without being SIGKILLed, we could update
attach_store() to report kthread_get_run() failure to the caller, but that will be a separate
patch. This patch alone avoids the crash although there is a hung task problem similar to
https://syzkaller.appspot.com/bug?id=5677eeeb83e5d47ef2b04e9bd68f5ff4c7e572ab remains
( https://syzkaller.appspot.com/text?tag=CrashReport&x=17aa3f78d00000 ). The cause of hung
task is currently unknown; maybe too much printk() messages.

Re: [PATCH] usb: usbip: fix error handling of kthread_get_run()

[Shuah Khan]

On 2/5/21 6:08 PM, Tetsuo Handa wrote:
> On 2021/02/06 1:27, Shuah Khan wrote:
>> Good find. For this fix to be complete, you will have to add checks
>> for kthread_get_run() NULL return in attach_store() and
>> usbip_sockfd_store() routines in stub_dev.c and vudc_sysfs.c
> 
> Initially I thought that the cleaner fix is to get kthread_create() out of kthread_get_run()
> ( the drivers/usb/usbip/vhci_sysfs.c portion in
> https://syzkaller.appspot.com/x/patch.diff?x=16c3c090d00000 ) so that we can undo
> kthread_create() via kthread_stop(). But I found that such fix makes little sense because
> it is possible that SIGKILL is delivered between vhci_rx_loop() and vhci_tx_loop() have
> started and before leaving attach_store().
> 
> Since the code prior to "staging/usbip: convert to kthread" was already capable of surviving
> such race condition, this patch should be already good enough for sending to stable kernels.
> Of course, since kthread_create() may return -ENOMEM without being SIGKILLed, we could update
> attach_store() to report kthread_get_run() failure to the caller, but that will be a separate
> patch. This patch alone avoids the crash although there is a hung task problem similar to
> https://syzkaller.appspot.com/bug?id=5677eeeb83e5d47ef2b04e9bd68f5ff4c7e572ab remains
> ( https://syzkaller.appspot.com/text?tag=CrashReport&x=17aa3f78d00000 ). The cause of hung
> task is currently unknown; maybe too much printk() messages.
> 

I would like to see to see a complete fix. This patch changes
kthread_get_run() to return NULL. Without adding handling for
NULL in the callers of kthread_get_run(), we will start seeing
problems.

Does this patch fix the problem syzbot found?

thanks,
-- Shuah

Re: [PATCH] usb: usbip: fix error handling of kthread_get_run()

[Tetsuo Handa]

On 2021/02/11 3:11, Shuah Khan wrote:
> I would like to see to see a complete fix. This patch changes
> kthread_get_run() to return NULL. Without adding handling for
> NULL in the callers of kthread_get_run(), we will start seeing
> problems.

What problems are you aware of?

> 
> Does this patch fix the problem syzbot found?

Yes, this patch as-is avoids the crash syzbot found.

Re: [PATCH] usb: usbip: fix error handling of kthread_get_run()

[Shuah Khan]

On 2/10/21 11:16 AM, Tetsuo Handa wrote:
> On 2021/02/11 3:11, Shuah Khan wrote:
>> I would like to see to see a complete fix. This patch changes
>> kthread_get_run() to return NULL. Without adding handling for
>> NULL in the callers of kthread_get_run(), we will start seeing
>> problems.
> 
> What problems are you aware of?
> 

The fact that driver doesn't cleanup after failing to create
the thread is a problem.

>>
>> Does this patch fix the problem syzbot found?
> 
> Yes, this patch as-is avoids the crash syzbot found.
> 

Good to know. Please add handling for kthread_get_run() return
in the places I suggested in you next version of this patch.

thanks,
-- Shuah

Re: [PATCH] usb: usbip: fix error handling of kthread_get_run()

[Tetsuo Handa]

On 2021/02/11 3:20, Shuah Khan wrote:
> On 2/10/21 11:16 AM, Tetsuo Handa wrote:
>> On 2021/02/11 3:11, Shuah Khan wrote:
>>> I would like to see to see a complete fix. This patch changes
>>> kthread_get_run() to return NULL. Without adding handling for
>>> NULL in the callers of kthread_get_run(), we will start seeing
>>> problems.
>>
>> What problems are you aware of?
>>
> 
> The fact that driver doesn't cleanup after failing to create
> the thread is a problem.

What are the cleanup functions?

Future attach_store() will succeed if cleanup operation (which does
vdev->ud.status = VDEV_ST_NULL;) is done, doesn't it?

And vhci_device_reset() and/or vhci_device_init() involves cleanup
operation (which does vdev->ud.status = VDEV_ST_NULL;), doesn't it?

> 
>>>
>>> Does this patch fix the problem syzbot found?
>>
>> Yes, this patch as-is avoids the crash syzbot found.
>>
> 
> Good to know. Please add handling for kthread_get_run() return
> in the places I suggested in you next version of this patch.

Since vhci_{rx,tx}_loop() do not involve cleanup operation (they simply
terminate upon kthread_should_stop() == true), I don't understand why
failing to start vhci_{rx,tx}_loop() makes difference. Cleanup will be
done by functions other than vhci_{rx,tx}_loop(), won't it?

Re: [PATCH] usb: usbip: fix error handling of kthread_get_run()

[Shuah Khan]

On 2/10/21 11:43 AM, Tetsuo Handa wrote:
> On 2021/02/11 3:20, Shuah Khan wrote:
>> On 2/10/21 11:16 AM, Tetsuo Handa wrote:
>>> On 2021/02/11 3:11, Shuah Khan wrote:
>>>> I would like to see to see a complete fix. This patch changes
>>>> kthread_get_run() to return NULL. Without adding handling for
>>>> NULL in the callers of kthread_get_run(), we will start seeing
>>>> problems.
>>>
>>> What problems are you aware of?
>>>
>>
>> The fact that driver doesn't cleanup after failing to create
>> the thread is a problem.
> 
> What are the cleanup functions?
> 

When user-space requests attaching to a device, attach_store()
tries to attach the requested device. When kthread_get_run()
failure is ignored silently, and continue with call to
rh_port_connect(), user-space assumes attach is successful.
User thinks attach is successful.

When and how will this attach failure gets reported to the
in this scenario?

Error handling for this case is no different from other error
paths in attach_store().

Please see error handling for other errors in attach_store().
In this case the right error handling is to rewind the vdev
init and bail out returning error. This would include setting
vdev->ud.status to VDEV_ST_NULL.

I found the following reproducer that tells me how attach
is triggered.
https://syzkaller.appspot.com/text?tag=ReproC&x=128506e4d00000

syzbot is helping us harden these paths, which is awesome.
Fixing these have to consider user api.

I you would like to fix this, please send me a complete fix.

thanks,
-- Shuah

Re: [PATCH] usb: usbip: fix error handling of kthread_get_run()

[Tetsuo Handa]

On 2021/02/11 5:15, Shuah Khan wrote:
> On 2/10/21 11:43 AM, Tetsuo Handa wrote:
>> On 2021/02/11 3:20, Shuah Khan wrote:
>>> On 2/10/21 11:16 AM, Tetsuo Handa wrote:
>>>> On 2021/02/11 3:11, Shuah Khan wrote:
>>>>> I would like to see to see a complete fix. This patch changes
>>>>> kthread_get_run() to return NULL. Without adding handling for
>>>>> NULL in the callers of kthread_get_run(), we will start seeing
>>>>> problems.
>>>>
>>>> What problems are you aware of?
>>>>
>>>
>>> The fact that driver doesn't cleanup after failing to create
>>> the thread is a problem.
>>
>> What are the cleanup functions?
>>
> 
> When user-space requests attaching to a device, attach_store()
> tries to attach the requested device. When kthread_get_run()
> failure is ignored silently, and continue with call to
> rh_port_connect(), user-space assumes attach is successful.
> User thinks attach is successful.

The

  struct kthread_create_info *create = kmalloc(sizeof(*create), GFP_KERNEL);

in __kthread_create_on_node() never fails unless killed by the OOM killer
due to the "too small to fail" memory-allocation rule, and

  wait_for_completion_killable(&done)

in __kthread_create_on_node() never fails unless killed. Creating a kernel
thread as root user unlikely fails, and memory allocations by that kernel
thread also never fails due to the "too small to fail" memory-allocation rule.

Therefore, kthread_get_run() effectively fails only when current thread
which called attach_store() was killed. And

> 
> When and how will this attach failure gets reported to the
> in this scenario?

if the current thread was killed, how can the failure get reported to
the user-space in this scenario?

> 
> Error handling for this case is no different from other error
> paths in attach_store().
> 
> Please see error handling for other errors in attach_store().

Being "killed" means that user-space can never know the result
unlike other error paths in attach_store().

> In this case the right error handling is to rewind the vdev
> init and bail out returning error. This would include setting
> vdev->ud.status to VDEV_ST_NULL.

If the user-space was killed, the kernel is responsible for offering
automatic cleanup which includes setting vdev->ud.status to VDEV_ST_NULL.

> 
> I found the following reproducer that tells me how attach
> is triggered.
> https://syzkaller.appspot.com/text?tag=ReproC&x=128506e4d00000

This reproducer (which is killed after 5 seconds from fork()) uses
only /sys/devices/platform/vhci_hcd.0/attach interface and never uses
detach interface. Detach and cleanup are up to automatic cleanup
offered by the kernel.

> 
> syzbot is helping us harden these paths, which is awesome.
> Fixing these have to consider user api.
> 
> I you would like to fix this, please send me a complete fix.

If you want to handle the unlikely "__kthread_create_on_node() fails without
being killed" case, such change ( the drivers/usb/usbip/vhci_sysfs.c portion in
https://syzkaller.appspot.com/x/patch.diff?x=16c3c090d00000 ) should be a separate
patch. Since this patch declares "Fixes: 9720b4bc76a83807 ("staging/usbip: convert to kthread")",
this patch intends for the minimal change and does not want to do extra things.

Re: [PATCH] usb: usbip: fix error handling of kthread_get_run()

[Tetsuo Handa]

On 2021/02/11 10:04, Tetsuo Handa wrote:
> On 2021/02/11 5:15, Shuah Khan wrote:
>> I you would like to fix this, please send me a complete fix.
> 
> If you want to handle the unlikely "__kthread_create_on_node() fails without
> being killed" case, such change ( the drivers/usb/usbip/vhci_sysfs.c portion in
> https://syzkaller.appspot.com/x/patch.diff?x=16c3c090d00000 ) should be a separate
> patch. Since this patch declares "Fixes: 9720b4bc76a83807 ("staging/usbip: convert to kthread")",
> this patch intends for the minimal change and does not want to do extra things.
> 

If you want a complete fix, the (untested) diff will become large.

 drivers/usb/usbip/stub_dev.c   | 61 ++++++++++++++++++++++------------
 drivers/usb/usbip/vhci_sysfs.c | 33 +++++++++++++++---
 drivers/usb/usbip/vudc_sysfs.c | 35 +++++++++++++++----
 3 files changed, 95 insertions(+), 34 deletions(-)

diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
index 2305d425e6c9..72c561878df7 100644
--- a/drivers/usb/usbip/stub_dev.c
+++ b/drivers/usb/usbip/stub_dev.c
@@ -39,77 +39,94 @@ 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)
 {
 	struct stub_device *sdev = dev_get_drvdata(dev);
 	int sockfd = 0;
 	struct socket *socket;
 	int rv;
+	int err;
+	struct task_struct *tx = NULL;
+	struct task_struct *rx = NULL;
 
 	if (!sdev) {
 		dev_err(dev, "sdev is null\n");
 		return -ENODEV;
 	}
 
 	rv = sscanf(buf, "%d", &sockfd);
 	if (rv != 1)
 		return -EINVAL;
 
 	if (sockfd != -1) {
-		int err;
+		socket = sockfd_lookup(sockfd, &err);
+		if (!socket)
+			return -EINVAL;
+
+		/* Create threads now. */
+		rx = kthread_create(stub_rx_loop, &sdev->ud, "stub_rx");
+		tx = kthread_create(stub_tx_loop, &sdev->ud, "stub_tx");
+		if (IS_ERR(rx) || IS_ERR(tx))
+			goto thread_error;
 
 		dev_info(dev, "stub up\n");
 
 		spin_lock_irq(&sdev->ud.lock);
 
 		if (sdev->ud.status != SDEV_ST_AVAILABLE) {
+			spin_unlock_irq(&sdev->ud.lock);
 			dev_err(dev, "not ready\n");
-			goto err;
+			err = -EINVAL;
+			goto thread_error;
 		}
 
-		socket = sockfd_lookup(sockfd, &err);
-		if (!socket)
-			goto err;
-
 		sdev->ud.tcp_socket = 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.status = SDEV_ST_USED;
 		spin_unlock_irq(&sdev->ud.lock);
 
+		/* Start the threads. */
+		get_task_struct(rx);
+		sdev->ud.tcp_rx = rx;
+		wake_up_process(rx);
+		get_task_struct(tx);
+		sdev->ud.tcp_tx = tx;
+		wake_up_process(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);
 
+		/* Race warning: sdev->ud.status == SDEV_ST_USED may be no longer true. */
 		usbip_event_add(&sdev->ud, SDEV_EVENT_DOWN);
 	}
 
 	return count;
-
-err:
-	spin_unlock_irq(&sdev->ud.lock);
-	return -EINVAL;
+thread_error:
+	sockfd_put(socket);
+	if (IS_ERR(rx))
+		err = PTR_ERR(rx);
+	else if (rx)
+		kthread_stop(rx);
+	if (IS_ERR(tx))
+		err = PTR_ERR(tx);
+	else if (tx)
+		kthread_stop(tx);
+	return err;
 }
 static DEVICE_ATTR_WO(usbip_sockfd);
 
 static struct attribute *usbip_attrs[] = {
 	&dev_attr_usbip_status.attr,
 	&dev_attr_usbip_sockfd.attr,
 	&dev_attr_usbip_debug.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(usbip);
diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index be37aec250c2..0d10021c4186 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -305,20 +305,22 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
 {
 	struct socket *socket;
 	int sockfd = 0;
 	__u32 port = 0, pdev_nr = 0, rhport = 0, devid = 0, speed = 0;
 	struct usb_hcd *hcd;
 	struct vhci_hcd *vhci_hcd;
 	struct vhci_device *vdev;
 	struct vhci *vhci;
 	int err;
 	unsigned long flags;
+	struct task_struct *tx;
+	struct task_struct *rx;
 
 	/*
 	 * @rhport: port number of vhci_hcd
 	 * @sockfd: socket descriptor of an established TCP connection
 	 * @devid: unique device identifier in a remote host
 	 * @speed: usb device speed in a remote host
 	 */
 	if (sscanf(buf, "%u %u %u %u", &port, &sockfd, &devid, &speed) != 4)
 		return -EINVAL;
 	pdev_nr = port_to_pdev_nr(port);
@@ -345,62 +347,83 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
 	if (speed == USB_SPEED_SUPER)
 		vdev = &vhci->vhci_hcd_ss->vdev[rhport];
 	else
 		vdev = &vhci->vhci_hcd_hs->vdev[rhport];
 
 	/* Extract socket from fd. */
 	socket = sockfd_lookup(sockfd, &err);
 	if (!socket)
 		return -EINVAL;
 
+	/* Create threads now. */
+	rx = kthread_create(vhci_rx_loop, &vdev->ud, "vhci_rx");
+	tx = kthread_create(vhci_tx_loop, &vdev->ud, "vhci_tx");
+	if (IS_ERR(rx) || IS_ERR(tx))
+		goto thread_error;
+
 	/* now need lock until setting vdev status as used */
 
 	/* begin a lock */
 	spin_lock_irqsave(&vhci->lock, flags);
 	spin_lock(&vdev->ud.lock);
 
 	if (vdev->ud.status != VDEV_ST_NULL) {
 		/* end of the lock */
 		spin_unlock(&vdev->ud.lock);
 		spin_unlock_irqrestore(&vhci->lock, flags);
 
-		sockfd_put(socket);
-
 		dev_err(dev, "port %d already used\n", rhport);
 		/*
 		 * Will be retried from userspace
 		 * if there's another free port.
 		 */
-		return -EBUSY;
+		err = -EBUSY;
+		goto thread_error;
 	}
 
 	dev_info(dev, "pdev(%u) rhport(%u) sockfd(%d)\n",
 		 pdev_nr, rhport, sockfd);
 	dev_info(dev, "devid(%u) speed(%u) speed_str(%s)\n",
 		 devid, speed, usb_speed_string(speed));
 
 	vdev->devid         = devid;
 	vdev->speed         = speed;
 	vdev->ud.sockfd     = sockfd;
 	vdev->ud.tcp_socket = socket;
 	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");
+	/* Start the threads. */
+	get_task_struct(rx);
+	vdev->ud.tcp_rx = rx;
+	wake_up_process(rx);
+	get_task_struct(tx);
+	vdev->ud.tcp_tx = tx;
+	wake_up_process(tx);
 
 	rh_port_connect(vdev, speed);
 
 	return count;
+thread_error:
+	sockfd_put(socket);
+	if (IS_ERR(rx))
+		err = PTR_ERR(rx);
+	else
+		kthread_stop(rx);
+	if (IS_ERR(tx))
+		err = PTR_ERR(tx);
+	else
+		kthread_stop(tx);
+	return err;
 }
 static DEVICE_ATTR_WO(attach);
 
 #define MAX_STATUS_NAME 16
 
 struct status_attr {
 	struct device_attribute attr;
 	char name[MAX_STATUS_NAME+1];
 };
 
diff --git a/drivers/usb/usbip/vudc_sysfs.c b/drivers/usb/usbip/vudc_sysfs.c
index 100f680c572a..d01acb6d6b1c 100644
--- a/drivers/usb/usbip/vudc_sysfs.c
+++ b/drivers/usb/usbip/vudc_sysfs.c
@@ -93,29 +93,40 @@ 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)
 {
 	struct vudc *udc = (struct vudc *) dev_get_drvdata(dev);
 	int rv;
 	int sockfd = 0;
 	int err;
 	struct socket *socket;
 	unsigned long flags;
 	int ret;
+	struct task_struct *tx = NULL;
+	struct task_struct *rx = NULL;
 
 	rv = kstrtoint(in, 0, &sockfd);
 	if (rv != 0)
 		return -EINVAL;
 
 	if (!udc) {
 		dev_err(dev, "no device");
 		return -ENODEV;
 	}
+
+	/* Create threads now. */
+	if (sockfd != -1) {
+		rx = kthread_create(&v_rx_loop, &udc->ud, "vudc_rx");
+		tx = kthread_create(&v_tx_loop, &udc->ud, "vudc_tx");
+		if (IS_ERR(rx) || IS_ERR(tx))
+			goto thread_error;
+	}
+
 	spin_lock_irqsave(&udc->lock, flags);
 	/* Don't export what we don't have */
 	if (!udc->driver || !udc->pullup) {
 		dev_err(dev, "gadget not bound");
 		ret = -ENODEV;
 		goto unlock;
 	}
 
 	if (sockfd != -1) {
 		if (udc->connected) {
@@ -132,33 +143,34 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a
 		}
 
 		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.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");
+		/* Start the threads. */
+		get_task_struct(rx);
+		udc->ud.tcp_rx = rx;
+		wake_up_process(rx);
+		get_task_struct(tx);
+		udc->ud.tcp_tx = tx;
+		wake_up_process(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);
 		udc->connected = 1;
 	} else {
 		if (!udc->connected) {
 			dev_err(dev, "Device not connected");
 			ret = -EINVAL;
 			goto unlock;
 		}
@@ -174,20 +186,29 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a
 	}
 
 	spin_unlock_irqrestore(&udc->lock, flags);
 
 	return count;
 
 unlock_ud:
 	spin_unlock_irq(&udc->ud.lock);
 unlock:
 	spin_unlock_irqrestore(&udc->lock, flags);
+thread_error:
+	if (IS_ERR(rx))
+		ret = PTR_ERR(rx);
+	else if (rx)
+		kthread_stop(rx);
+	if (IS_ERR(tx))
+		ret = PTR_ERR(tx);
+	else if (tx)
+		kthread_stop(tx);
 
 	return ret;
 }
 static DEVICE_ATTR_WO(usbip_sockfd);
 
 static ssize_t usbip_status_show(struct device *dev,
 			       struct device_attribute *attr, char *out)
 {
 	struct vudc *udc = (struct vudc *) dev_get_drvdata(dev);
 	int status;

Re: [PATCH] usb: usbip: fix error handling of kthread_get_run()

[Tetsuo Handa]

On 2021/02/11 12:01, Tetsuo Handa wrote:
> On 2021/02/11 10:04, Tetsuo Handa wrote:
>> On 2021/02/11 5:15, Shuah Khan wrote:
>>> I you would like to fix this, please send me a complete fix.
>>
>> If you want to handle the unlikely "__kthread_create_on_node() fails without
>> being killed" case, such change ( the drivers/usb/usbip/vhci_sysfs.c portion in
>> https://syzkaller.appspot.com/x/patch.diff?x=16c3c090d00000 ) should be a separate
>> patch. Since this patch declares "Fixes: 9720b4bc76a83807 ("staging/usbip: convert to kthread")",
>> this patch intends for the minimal change and does not want to do extra things.
>>
> 
> If you want a complete fix, the (untested) diff will become large.

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...

> diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
> index be37aec250c2..0d10021c4186 100644
> --- a/drivers/usb/usbip/vhci_sysfs.c
> +++ b/drivers/usb/usbip/vhci_sysfs.c
> @@ -305,20 +305,22 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
>  {
>  	struct socket *socket;
>  	int sockfd = 0;
>  	__u32 port = 0, pdev_nr = 0, rhport = 0, devid = 0, speed = 0;
>  	struct usb_hcd *hcd;
>  	struct vhci_hcd *vhci_hcd;
>  	struct vhci_device *vdev;
>  	struct vhci *vhci;
>  	int err;
>  	unsigned long flags;
> +	struct task_struct *tx;
> +	struct task_struct *rx;
>  
>  	/*
>  	 * @rhport: port number of vhci_hcd
>  	 * @sockfd: socket descriptor of an established TCP connection
>  	 * @devid: unique device identifier in a remote host
>  	 * @speed: usb device speed in a remote host
>  	 */
>  	if (sscanf(buf, "%u %u %u %u", &port, &sockfd, &devid, &speed) != 4)
>  		return -EINVAL;
>  	pdev_nr = port_to_pdev_nr(port);
> @@ -345,62 +347,83 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
>  	if (speed == USB_SPEED_SUPER)
>  		vdev = &vhci->vhci_hcd_ss->vdev[rhport];
>  	else
>  		vdev = &vhci->vhci_hcd_hs->vdev[rhport];
>  
>  	/* Extract socket from fd. */
>  	socket = sockfd_lookup(sockfd, &err);
>  	if (!socket)
>  		return -EINVAL;
>  
> +	/* Create threads now. */
> +	rx = kthread_create(vhci_rx_loop, &vdev->ud, "vhci_rx");
> +	tx = kthread_create(vhci_tx_loop, &vdev->ud, "vhci_tx");
> +	if (IS_ERR(rx) || IS_ERR(tx))
> +		goto thread_error;
> +
>  	/* now need lock until setting vdev status as used */
>  
>  	/* begin a lock */
>  	spin_lock_irqsave(&vhci->lock, flags);
>  	spin_lock(&vdev->ud.lock);
>  
>  	if (vdev->ud.status != VDEV_ST_NULL) {
>  		/* end of the lock */
>  		spin_unlock(&vdev->ud.lock);
>  		spin_unlock_irqrestore(&vhci->lock, flags);
>  
> -		sockfd_put(socket);
> -
>  		dev_err(dev, "port %d already used\n", rhport);
>  		/*
>  		 * Will be retried from userspace
>  		 * if there's another free port.
>  		 */
> -		return -EBUSY;
> +		err = -EBUSY;
> +		goto thread_error;
>  	}
>  
>  	dev_info(dev, "pdev(%u) rhport(%u) sockfd(%d)\n",
>  		 pdev_nr, rhport, sockfd);
>  	dev_info(dev, "devid(%u) speed(%u) speed_str(%s)\n",
>  		 devid, speed, usb_speed_string(speed));
>  
>  	vdev->devid         = devid;
>  	vdev->speed         = speed;
>  	vdev->ud.sockfd     = sockfd;
>  	vdev->ud.tcp_socket = socket;
>  	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");
> +	/* Start the threads. */
> +	get_task_struct(rx);
> +	vdev->ud.tcp_rx = rx;
> +	wake_up_process(rx);

Even this seemingly complete fix turned out to be incomplete.

kthread_get_run(vhci_rx_loop, &vdev->ud, "vhci_rx") creates a vhci_rx kernel thread
and immediately starts vhci_rx_loop() function. Then, vdev->ud.tcp_rx becomes non-NULL
because the vhci_rx kernel thread was successfully created. Then in vhci_rx_loop(),
vhci_rx_pdu(ud) will be called because kthread_should_stop() is false and
usbip_event_happened() is 0. In vhci_rx_pdu(), sock_recvmsg() is called from usbip_recv().

If the peer side of ud->tcp_socket already called shutdown(SHUT_WR), sock_recvmsg() detects
end of stream and returns 0. Then, vhci_rx_pdu() prints "connection closed" message and
calls usbip_event_add(ud, VDEV_EVENT_DOWN).
(Well, where is the code that verifies that tcp_socket is indeed a SOCK_STREAM socket?
If the file descriptor passed was a SOCK_DGRAM socket, sock_recvmsg() can't detect
end of stream...)

Now, kthread_get_run(vhci_tx_loop, &vdev->ud, "vhci_tx") creates a vhci_tx kernel thread
and immediately starts vhci_tx_loop() function. But if current thread which called
kthread_get_run() is preempted at this moment, vdev->ud.tcp_tx remains NULL despite
vhci_tx kernel thread was successfully created.

Then in vhci_tx_loop(), wait_event_interruptible() is called because kthread_should_stop()
is false and vhci_send_cmd_submit() is 0 and vhci_send_cmd_unlink() is 0. But the vhci_tx
kernel thread will call vhci_send_cmd_submit() again when list_empty(&vdev->priv_tx) becomes false.

Now, event_handler() is called by the usbip_event singlethreaded workqueue kernel thread
because the vhci_rx kernel thread called queue_work(usbip_queue, &usbip_work) from
usbip_event_add(). Since VDEV_EVENT_DOWN is defined as USBIP_EH_SHUTDOWN | USBIP_EH_RESET,
firstly ud->eh_ops.shutdown(ud) (which is mapped to vhci_shutdown_connection()) is called
and then ud->eh_ops.reset(ud) (which is mapped to vhci_device_reset()) is called.

In vhci_shutdown_connection(), it tries to shutdown the vhci_tx kernel thread and
the vhci_rx kernel thread before resetting vdev->ud.tcp_socket to NULL. But recall that
vdev->ud.tcp_tx can remains NULL due to preemption. As a result, despite the effort to handle
USBIP_EH_SHUTDOWN before USBIP_EH_RESET, kthread_stop_put(vdev->ud.tcp_tx) won't be called
before vdev->ud.tcp_socket = NULL is called.

When the vhci_tx kernel thread found that list_empty(&vdev->priv_tx) became false, it calls
vhci_send_cmd_submit() and triggers "general protection fault in tomoyo_socket_sendmsg_permission".
Therefore, I think

  "general protection fault in tomoyo_socket_sendmsg_permission" is a NULL pointer dereference
  which can happen if vhci_shutdown_connection() is called before vdev->ud.tcp_tx becomes non-NULL
  due to a race condition

and that the easiest way is to serialize attach operation and reset/detach operations using
a mutex, for event_handler() which calls reset/detach operations is a schedulable context.

> +	get_task_struct(tx);
> +	vdev->ud.tcp_tx = tx;
> +	wake_up_process(tx);

Maybe just grouping assignment of vdev->ud.tcp_socket, vdev->ud.status, vdev->ud.tcp_{rx,tx}
within one spinlock-protected section might be fine? But since vhci_port_disconnect() from
detach_store() seems to be racy with attach_store() (vhci_port_disconnect() can trigger
vhci_shutdown_connection() via usbip_event_add(&vdev->ud, VDEV_EVENT_DOWN) as soon as
attach_store() did vdev->ud.status = VDEV_ST_NOTASSIGNED), I suggest using a killable mutex
for serialization purpose is simpler and safer.

>  
>  	rh_port_connect(vdev, speed);
>  
>  	return count;
> +thread_error:
> +	sockfd_put(socket);
> +	if (IS_ERR(rx))
> +		err = PTR_ERR(rx);
> +	else
> +		kthread_stop(rx);
> +	if (IS_ERR(tx))
> +		err = PTR_ERR(tx);
> +	else
> +		kthread_stop(tx);
> +	return err;
>  }
>  static DEVICE_ATTR_WO(attach);
>  
>  #define MAX_STATUS_NAME 16
>  
>  struct status_attr {
>  	struct device_attribute attr;
>  	char name[MAX_STATUS_NAME+1];
>  };
>