On Sat, Feb 20, 2021 at 12:08:32AM +0900, 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")
> ---
> 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(-)
What changed from v1? Why isn't that info below the --- line?
Please do a v3 with that fixed up.
thanks,
greg k-h
