* KASAN: null-ptr-deref Write in vhci_shutdown_connection @ 2020-12-20 18:44 syzbot 2021-02-05 13:57 ` [PATCH] usb: usbip: fix error handling of kthread_get_run() Tetsuo Handa 0 siblings, 1 reply; 12+ messages in thread From: syzbot @ 2020-12-20 18:44 UTC (permalink / raw) To: gregkh, linux-kernel, linux-usb, shuah, syzkaller-bugs, valentina.manea.m 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] usb: usbip: fix error handling of kthread_get_run() 2020-12-20 18:44 KASAN: null-ptr-deref Write in vhci_shutdown_connection syzbot @ 2021-02-05 13:57 ` Tetsuo Handa 2021-02-05 16:27 ` Shuah Khan 0 siblings, 1 reply; 12+ messages in thread From: Tetsuo Handa @ 2021-02-05 13:57 UTC (permalink / raw) To: Greg Kroah-Hartman, Valentina Manea, Shuah Khan Cc: Shuah Khan, Hillf Danton, linux-usb, Tetsuo Handa, Arnd Bergmann 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: usbip: fix error handling of kthread_get_run() 2021-02-05 13:57 ` [PATCH] usb: usbip: fix error handling of kthread_get_run() Tetsuo Handa @ 2021-02-05 16:27 ` Shuah Khan 2021-02-06 1:08 ` Tetsuo Handa 0 siblings, 1 reply; 12+ messages in thread From: Shuah Khan @ 2021-02-05 16:27 UTC (permalink / raw) To: Tetsuo Handa, Greg Kroah-Hartman, Valentina Manea, Shuah Khan Cc: Hillf Danton, linux-usb, Arnd Bergmann, 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: usbip: fix error handling of kthread_get_run() 2021-02-05 16:27 ` Shuah Khan @ 2021-02-06 1:08 ` Tetsuo Handa 2021-02-10 18:11 ` Shuah Khan 0 siblings, 1 reply; 12+ messages in thread From: Tetsuo Handa @ 2021-02-06 1:08 UTC (permalink / raw) To: Shuah Khan, Greg Kroah-Hartman, Valentina Manea, Shuah Khan Cc: Hillf Danton, linux-usb, Arnd Bergmann 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: usbip: fix error handling of kthread_get_run() 2021-02-06 1:08 ` Tetsuo Handa @ 2021-02-10 18:11 ` Shuah Khan 2021-02-10 18:16 ` Tetsuo Handa 0 siblings, 1 reply; 12+ messages in thread From: Shuah Khan @ 2021-02-10 18:11 UTC (permalink / raw) To: Tetsuo Handa, Greg Kroah-Hartman, Valentina Manea, Shuah Khan, Shuah Khan Cc: Hillf Danton, linux-usb, Arnd Bergmann 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: usbip: fix error handling of kthread_get_run() 2021-02-10 18:11 ` Shuah Khan @ 2021-02-10 18:16 ` Tetsuo Handa 2021-02-10 18:20 ` Shuah Khan 0 siblings, 1 reply; 12+ messages in thread From: Tetsuo Handa @ 2021-02-10 18:16 UTC (permalink / raw) To: Shuah Khan, Greg Kroah-Hartman, Valentina Manea, Shuah Khan Cc: Hillf Danton, linux-usb, Arnd Bergmann 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: usbip: fix error handling of kthread_get_run() 2021-02-10 18:16 ` Tetsuo Handa @ 2021-02-10 18:20 ` Shuah Khan 2021-02-10 18:43 ` Tetsuo Handa 0 siblings, 1 reply; 12+ messages in thread From: Shuah Khan @ 2021-02-10 18:20 UTC (permalink / raw) To: Tetsuo Handa, Greg Kroah-Hartman, Valentina Manea, Shuah Khan Cc: Hillf Danton, linux-usb, Arnd Bergmann, 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: usbip: fix error handling of kthread_get_run() 2021-02-10 18:20 ` Shuah Khan @ 2021-02-10 18:43 ` Tetsuo Handa 2021-02-10 20:15 ` Shuah Khan 0 siblings, 1 reply; 12+ messages in thread From: Tetsuo Handa @ 2021-02-10 18:43 UTC (permalink / raw) To: Shuah Khan, Greg Kroah-Hartman, Valentina Manea, Shuah Khan Cc: Hillf Danton, linux-usb, Arnd Bergmann 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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: usbip: fix error handling of kthread_get_run() 2021-02-10 18:43 ` Tetsuo Handa @ 2021-02-10 20:15 ` Shuah Khan 2021-02-11 1:04 ` Tetsuo Handa 0 siblings, 1 reply; 12+ messages in thread From: Shuah Khan @ 2021-02-10 20:15 UTC (permalink / raw) To: Tetsuo Handa, Greg Kroah-Hartman, Valentina Manea, Shuah Khan Cc: Hillf Danton, linux-usb, Arnd Bergmann, 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: usbip: fix error handling of kthread_get_run() 2021-02-10 20:15 ` Shuah Khan @ 2021-02-11 1:04 ` Tetsuo Handa 2021-02-11 3:01 ` Tetsuo Handa 0 siblings, 1 reply; 12+ messages in thread From: Tetsuo Handa @ 2021-02-11 1:04 UTC (permalink / raw) To: Shuah Khan, Greg Kroah-Hartman, Valentina Manea, Shuah Khan Cc: Hillf Danton, linux-usb, Arnd Bergmann 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: usbip: fix error handling of kthread_get_run() 2021-02-11 1:04 ` Tetsuo Handa @ 2021-02-11 3:01 ` Tetsuo Handa 2021-02-11 13:40 ` Tetsuo Handa 0 siblings, 1 reply; 12+ messages in thread From: Tetsuo Handa @ 2021-02-11 3:01 UTC (permalink / raw) To: Shuah Khan, Greg Kroah-Hartman, Valentina Manea, Shuah Khan Cc: Hillf Danton, linux-usb, Arnd Bergmann 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; ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: usbip: fix error handling of kthread_get_run() 2021-02-11 3:01 ` Tetsuo Handa @ 2021-02-11 13:40 ` Tetsuo Handa 0 siblings, 0 replies; 12+ messages in thread From: Tetsuo Handa @ 2021-02-11 13:40 UTC (permalink / raw) To: Shuah Khan, Greg Kroah-Hartman, Valentina Manea, Shuah Khan Cc: Hillf Danton, linux-usb, Arnd Bergmann 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]; > }; > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-02-11 13:44 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-20 18:44 KASAN: null-ptr-deref Write in vhci_shutdown_connection syzbot 2021-02-05 13:57 ` [PATCH] usb: usbip: fix error handling of kthread_get_run() Tetsuo Handa 2021-02-05 16:27 ` Shuah Khan 2021-02-06 1:08 ` Tetsuo Handa 2021-02-10 18:11 ` Shuah Khan 2021-02-10 18:16 ` Tetsuo Handa 2021-02-10 18:20 ` Shuah Khan 2021-02-10 18:43 ` Tetsuo Handa 2021-02-10 20:15 ` Shuah Khan 2021-02-11 1:04 ` Tetsuo Handa 2021-02-11 3:01 ` Tetsuo Handa 2021-02-11 13:40 ` Tetsuo Handa
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).