Skip to content

Commit 718ad96

Browse files
shuahkhgregkh
authored andcommitted
usbip: fix vhci_hcd attach_store() races leading to gpf
attach_store() is invoked when user requests import (attach) a device from usbip host. Attach and detach are governed by local state and shared state - Shared state (usbip device status) - Device status is used to manage the attach and detach operations on import-able devices. - Local state (tcp_socket, rx and tx thread task_struct ptrs) A valid tcp_socket controls rx and tx thread operations while the device is in exported state. - Device has to be in the right state to be attached and detached. Attach sequence includes validating the socket and creating receive (rx) and transmit (tx) threads to talk to the host to get access to the imported device. rx and tx threads depends on local and shared state to be correct and in sync. Detach sequence shuts the socket down and stops the rx and tx threads. Detach sequence relies on local and shared states to be in sync. There are races in updating the local and shared status in the current attach sequence resulting in crashes. These stem from starting rx and tx threads before local and global state is updated correctly to be in sync. 1. Doesn't handle kthread_create() error and saves invalid ptr in local state that drives rx and tx threads. 2. Updates tcp_socket and sockfd, starts stub_rx and stub_tx threads before updating usbip_device status to VDEV_ST_NOTASSIGNED. This opens up a race condition between the threads, port connect, and detach handling. Fix the above problems: - Stop using kthread_get_run() macro to create/start threads. - Create threads and get task struct reference. - Add kthread_create() failure handling and bail out. - Hold vhci and usbip_device locks to update local and shared states after creating rx and tx threads. - Update usbip_device status to VDEV_ST_NOTASSIGNED. - Update usbip_device tcp_socket, sockfd, tcp_rx, and tcp_tx - Start threads after usbip_device (tcp_socket, sockfd, tcp_rx, tcp_tx, and status) is complete. Credit goes to syzbot and Tetsuo Handa for finding and root-causing the kthread_get_run() improper error handling problem and others. This is hard problem to find and debug since the races aren't seen in a normal case. Fuzzing forces the race window to be small enough for the kthread_get_run() error path bug and starting threads before updating the local and shared state bug in the attach sequence. - Update usbip_device tcp_rx and tcp_tx pointers holding vhci and usbip_device locks. Tested with syzbot reproducer: - https://syzkaller.appspot.com/text?tag=ReproC&x=14801034d00000 Fixes: 9720b4b ("staging/usbip: convert to kthread") Cc: stable@vger.kernel.org Reported-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com> Reported-by: syzbot <syzbot+bf1a360e305ee719e364@syzkaller.appspotmail.com> Reported-by: syzbot <syzbot+95ce4b142579611ef0a9@syzkaller.appspotmail.com> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> Link: https://lore.kernel.org/r/bb434bd5d7a64fbec38b5ecfb838a6baef6eb12b.1615171203.git.skhan@linuxfoundation.org Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
1 parent 9380afd commit 718ad96

1 file changed

Lines changed: 25 additions & 4 deletions

File tree

drivers/usb/usbip/vhci_sysfs.c

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,8 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
312312
struct vhci *vhci;
313313
int err;
314314
unsigned long flags;
315+
struct task_struct *tcp_rx = NULL;
316+
struct task_struct *tcp_tx = NULL;
315317

316318
/*
317319
* @rhport: port number of vhci_hcd
@@ -360,9 +362,24 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
360362
return -EINVAL;
361363
}
362364

363-
/* now need lock until setting vdev status as used */
365+
/* create threads before locking */
366+
tcp_rx = kthread_create(vhci_rx_loop, &vdev->ud, "vhci_rx");
367+
if (IS_ERR(tcp_rx)) {
368+
sockfd_put(socket);
369+
return -EINVAL;
370+
}
371+
tcp_tx = kthread_create(vhci_tx_loop, &vdev->ud, "vhci_tx");
372+
if (IS_ERR(tcp_tx)) {
373+
kthread_stop(tcp_rx);
374+
sockfd_put(socket);
375+
return -EINVAL;
376+
}
377+
378+
/* get task structs now */
379+
get_task_struct(tcp_rx);
380+
get_task_struct(tcp_tx);
364381

365-
/* begin a lock */
382+
/* now begin lock until setting vdev status set */
366383
spin_lock_irqsave(&vhci->lock, flags);
367384
spin_lock(&vdev->ud.lock);
368385

@@ -372,6 +389,8 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
372389
spin_unlock_irqrestore(&vhci->lock, flags);
373390

374391
sockfd_put(socket);
392+
kthread_stop_put(tcp_rx);
393+
kthread_stop_put(tcp_tx);
375394

376395
dev_err(dev, "port %d already used\n", rhport);
377396
/*
@@ -390,15 +409,17 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
390409
vdev->speed = speed;
391410
vdev->ud.sockfd = sockfd;
392411
vdev->ud.tcp_socket = socket;
412+
vdev->ud.tcp_rx = tcp_rx;
413+
vdev->ud.tcp_tx = tcp_tx;
393414
vdev->ud.status = VDEV_ST_NOTASSIGNED;
394415
usbip_kcov_handle_init(&vdev->ud);
395416

396417
spin_unlock(&vdev->ud.lock);
397418
spin_unlock_irqrestore(&vhci->lock, flags);
398419
/* end the lock */
399420

400-
vdev->ud.tcp_rx = kthread_get_run(vhci_rx_loop, &vdev->ud, "vhci_rx");
401-
vdev->ud.tcp_tx = kthread_get_run(vhci_tx_loop, &vdev->ud, "vhci_tx");
421+
wake_up_process(vdev->ud.tcp_rx);
422+
wake_up_process(vdev->ud.tcp_tx);
402423

403424
rh_port_connect(vdev, speed);
404425

0 commit comments

Comments
 (0)