On 2021/02/11 12:01, Tetsuo Handa wrote:

On 2021/02/11 10:04, Tetsuo Handa wrote:

quoted

On 2021/02/11 5:15, Shuah Khan wrote:

quoted

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

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];
 };