On 2/22/21 6:59 PM, Tetsuo Handa wrote:
syzbot is reporting an ERR_PTR(-EINTR) pointer dereference at
vhci_shutdown_connection() [1], 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 means a valid task
pointer. However, this patch does not make kthread_get_run() return NULL
when kthread_create() failed, for this patch removes kthread_get_run() in
order to fix the other bug described below.
syzbot is also reporting a NULL pointer dereference at sock_sendmsg() [2],
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.
Therefore, this patch does the following things in order to fix reported
bugs and other possible bugs.
(1) Handle kthread_create() failure (which fixes [1]) by grouping socket
lookup, kthread_create() and get_task_struct() into
usbip_prepare_threads() function.
(2) Serialize usbip_sockfd_store(), detach_store(), attach_store() and
ud->eh_ops.{shutdown,reset,unusable}() operations using
usbip_event_mutex mutex (which fixes [2]). Introducing such large
mutex should be safe because ud->tcp_{tx,rx} must not wait for
event_handler() to flush because event_handler() is processed by a
singlethreaded workqueue.
(3) Add SOCK_STREAM check into usbip_prepare_threads(), for current code
is not verifying that a file descriptor passed is actually a stream
socket. If the file descriptor passed was a SOCK_DGRAM socket,
sock_recvmsg() can't detect end of stream.
(4) Don't perform ud->tcp_socket = NULL in vhci_device_reset().
Since ud->tcp_{tx,rx} depend on ud->tcp_socket != NULL whereas
ud->tcp_socket and ud->tcp_{tx,rx} are assigned at the same time,
it is never safe to reset ud->tcp_socket from vhci_device_reset()
without calling kthread_stop_put() from vhci_shutdown_connection().
(5) usbip_sockfd_store() must perform
if ({sdev,udc}->ud.status != SDEV_ST_AVAILABLE) {
/* misc assignments for attach operation */
{sdev,udc}->ud.status = SDEV_ST_USED;
}
atomically, or multiple ud->tcp_{tx,rx} are created (which will later
cause a crash like [2]) and refcount on ud->tcp_socket is leaked when
usbip_sockfd_store() is concurrently called.
[1] https://syzkaller.appspot.com/bug?extid=a93fba6d384346a761e3
[2] https://syzkaller.appspot.com/bug?extid=95ce4b142579611ef0a9
Reported-and-tested-by: syzbot <syzbot+a93fba6d384346a761e3@xxxxxxxxxxxxxxxxxxxxxxxxx>
Reported-by: syzbot <syzbot+bf1a360e305ee719e364@xxxxxxxxxxxxxxxxxxxxxxxxx>
Reported-by: syzbot <syzbot+95ce4b142579611ef0a9@xxxxxxxxxxxxxxxxxxxxxxxxx>
References: https://lkml.kernel.org/r/676d4518-0faa-9fab-15db-0db8d216d7fb@xxxxxxxxxxxxxxxxxxx
Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
Fixes: 9720b4bc76a83807 ("staging/usbip: convert to kthread")
---
Changes since v1:
Tetsuo Handa found that the PTR_ERR() access in usbip_prepare_threads()
in v1 was wrong, and fixed it in v2.
Changes since v2:
The kernel test robot <lkp@xxxxxxxxx> found the same error using
scripts/coccinelle/tests/odd_ptr_err.cocci and
Julia Lawall <julia.lawall@xxxxxxxx> sent us a patch, but nothing
changed because that error was already fixed in v2.
drivers/usb/usbip/stub_dev.c | 56 ++++++++++++++++++--------------
drivers/usb/usbip/usbip_common.c | 55 +++++++++++++++++++++++++++++++
drivers/usb/usbip/usbip_common.h | 25 +++++++-------
drivers/usb/usbip/usbip_event.c | 15 +++++++++
drivers/usb/usbip/vhci_hcd.c | 6 ----
drivers/usb/usbip/vhci_sysfs.c | 50 ++++++++++++++++++++--------
drivers/usb/usbip/vudc_sysfs.c | 50 ++++++++++++++++------------
7 files changed, 181 insertions(+), 76 deletions(-)
This patch makes changes to 3 different drivers. Please split these
patches. Makes it easier to revert or fix them.
Patch1 could add common routines and use them in stud_dev and vudc
Same for usbip_event_lock_killable() and usbip_event_unlock().
Introduce them in a separate patch.
__usbip_sockfd_store() could be made common to stub_dev and vudc
similar to usbip_prepare_threads() and usbip_unprepare_threads()
It will lot easier to review if this large patch is split into
smaller patches.
thanks,
-- Shuah
