| Message ID | 20210410004930.17411-1-tseewald@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | usbip: fix vudc usbip_sockfd_store races leading to gpf | expand |
On Fri, Apr 09, 2021 at 07:49:30PM -0500, Tom Seewald wrote: > [backport of mainline commit 46613c9dfa96 ("usbip: fix vudc > usbip_sockfd_store races leading to gpf") to 4.9 and 4.14] Thanks, now queued up. greg k-h
On 4/9/21 6:49 PM, Tom Seewald wrote: > [backport of mainline commit 46613c9dfa96 ("usbip: fix vudc > usbip_sockfd_store races leading to gpf") to 4.9 and 4.14] > > usbip_sockfd_store() is invoked when user requests attach (import) > detach (unimport) usb gadget device from usbip host. vhci_hcd sends > import request and usbip_sockfd_store() exports the device if it is > free for export. > > Export and unexport are governed by local state and shared state > - Shared state (usbip device status, sockfd) - sockfd and Device > status are used to determine if stub should be brought up or shut > down. Device status is shared between host and client. > - 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. > - While the device is exported, device status is marked used and socket, > sockfd, and thread pointers are valid. > > Export sequence (stub-up) includes validating the socket and creating > receive (rx) and transmit (tx) threads to talk to the client to provide > access to the exported device. rx and tx threads depends on local and > shared state to be correct and in sync. > > Unexport (stub-down) sequence shuts the socket down and stops the rx and > tx threads. Stub-down sequence relies on local and shared states to be > in sync. > > There are races in updating the local and shared status in the current > stub-up 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 SDEV_ST_USED. This opens up a > race condition between the threads and usbip_sockfd_store() stub up > and down 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 usbip_device lock to update local and shared states after > creating rx and tx threads. > - Update usbip_device status to SDEV_ST_USED. > - 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 a > 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 stub-up sequence. > > 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> > Fixes: 9720b4bc76a83807 ("staging/usbip: convert to kthread") > Cc: stable@vger.kernel.org Thanks for helping with this backport. Please add the stables it fixes here: Cc: stable@vger.kernel.org # 4.14.x # # 4.9.x Combine this patch and the fix for this in the same. 9858af27e69247c5d04c3b093190a93ca365f33d usbip: Fix incorrect double assignment to udc->ud.tcp_rx thanks, -- Shuah thanks, -- Shuah
On 4/12/21 1:33 PM, Shuah Khan wrote: > On 4/9/21 6:49 PM, Tom Seewald wrote: >> [backport of mainline commit 46613c9dfa96 ("usbip: fix vudc >> usbip_sockfd_store races leading to gpf") to 4.9 and 4.14] >> >> usbip_sockfd_store() is invoked when user requests attach (import) >> detach (unimport) usb gadget device from usbip host. vhci_hcd sends >> import request and usbip_sockfd_store() exports the device if it is >> free for export. >> >> Export and unexport are governed by local state and shared state >> - Shared state (usbip device status, sockfd) - sockfd and Device >> status are used to determine if stub should be brought up or shut >> down. Device status is shared between host and client. >> - 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. >> - While the device is exported, device status is marked used and socket, >> sockfd, and thread pointers are valid. >> >> Export sequence (stub-up) includes validating the socket and creating >> receive (rx) and transmit (tx) threads to talk to the client to provide >> access to the exported device. rx and tx threads depends on local and >> shared state to be correct and in sync. >> >> Unexport (stub-down) sequence shuts the socket down and stops the rx and >> tx threads. Stub-down sequence relies on local and shared states to be >> in sync. >> >> There are races in updating the local and shared status in the current >> stub-up 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 SDEV_ST_USED. This opens up a >> race condition between the threads and usbip_sockfd_store() stub up >> and down 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 usbip_device lock to update local and shared states after >> creating rx and tx threads. >> - Update usbip_device status to SDEV_ST_USED. >> - 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 a >> 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 stub-up sequence. >> >> 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> >> Fixes: 9720b4bc76a83807 ("staging/usbip: convert to kthread") >> Cc: stable@vger.kernel.org > > Thanks for helping with this backport. > > Please add the stables it fixes here: > Cc: stable@vger.kernel.org # 4.14.x # # 4.9.x > > Combine this patch and the fix for this in the same. > > 9858af27e69247c5d04c3b093190a93ca365f33d > usbip: Fix incorrect double assignment to udc->ud.tcp_rx > Please ignore the suggestion to combine. Greg already pulled this patch. Sorry for the noise. thanks, -- Shuah
diff --git a/drivers/usb/usbip/vudc_sysfs.c b/drivers/usb/usbip/vudc_sysfs.c index e3f7c76d1956..f44d98eeb36a 100644 --- a/drivers/usb/usbip/vudc_sysfs.c +++ b/drivers/usb/usbip/vudc_sysfs.c @@ -103,8 +103,9 @@ static ssize_t dev_desc_read(struct file *file, struct kobject *kobj, } static BIN_ATTR_RO(dev_desc, sizeof(struct usb_device_descriptor)); -static ssize_t store_sockfd(struct device *dev, struct device_attribute *attr, - const char *in, size_t count) +static ssize_t store_sockfd(struct device *dev, + struct device_attribute *attr, + const char *in, size_t count) { struct vudc *udc = (struct vudc *) dev_get_drvdata(dev); int rv; @@ -113,6 +114,8 @@ static ssize_t store_sockfd(struct device *dev, struct device_attribute *attr, struct socket *socket; unsigned long flags; int ret; + struct task_struct *tcp_rx = NULL; + struct task_struct *tcp_tx = NULL; rv = kstrtoint(in, 0, &sockfd); if (rv != 0) @@ -158,24 +161,47 @@ static ssize_t store_sockfd(struct device *dev, struct device_attribute *attr, goto sock_err; } - udc->ud.tcp_socket = socket; - + /* unlock and create threads and get tasks */ 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"); + tcp_rx = kthread_create(&v_rx_loop, &udc->ud, "vudc_rx"); + if (IS_ERR(tcp_rx)) { + sockfd_put(socket); + return -EINVAL; + } + tcp_tx = kthread_create(&v_tx_loop, &udc->ud, "vudc_tx"); + if (IS_ERR(tcp_tx)) { + kthread_stop(tcp_rx); + sockfd_put(socket); + return -EINVAL; + } + + /* get task structs now */ + get_task_struct(tcp_rx); + get_task_struct(tcp_tx); + /* lock and update udc->ud state */ spin_lock_irqsave(&udc->lock, flags); spin_lock_irq(&udc->ud.lock); + + udc->ud.tcp_socket = socket; + udc->ud.tcp_rx = tcp_rx; + udc->ud.tcp_rx = tcp_tx; udc->ud.status = SDEV_ST_USED; + spin_unlock_irq(&udc->ud.lock); do_gettimeofday(&udc->start_time); v_start_timer(udc); udc->connected = 1; + + spin_unlock_irqrestore(&udc->lock, flags); + + wake_up_process(udc->ud.tcp_rx); + wake_up_process(udc->ud.tcp_tx); + return count; + } else { if (!udc->connected) { dev_err(dev, "Device not connected");
[backport of mainline commit 46613c9dfa96 ("usbip: fix vudc usbip_sockfd_store races leading to gpf") to 4.9 and 4.14] usbip_sockfd_store() is invoked when user requests attach (import) detach (unimport) usb gadget device from usbip host. vhci_hcd sends import request and usbip_sockfd_store() exports the device if it is free for export. Export and unexport are governed by local state and shared state - Shared state (usbip device status, sockfd) - sockfd and Device status are used to determine if stub should be brought up or shut down. Device status is shared between host and client. - 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. - While the device is exported, device status is marked used and socket, sockfd, and thread pointers are valid. Export sequence (stub-up) includes validating the socket and creating receive (rx) and transmit (tx) threads to talk to the client to provide access to the exported device. rx and tx threads depends on local and shared state to be correct and in sync. Unexport (stub-down) sequence shuts the socket down and stops the rx and tx threads. Stub-down sequence relies on local and shared states to be in sync. There are races in updating the local and shared status in the current stub-up 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 SDEV_ST_USED. This opens up a race condition between the threads and usbip_sockfd_store() stub up and down 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 usbip_device lock to update local and shared states after creating rx and tx threads. - Update usbip_device status to SDEV_ST_USED. - 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 a 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 stub-up sequence. 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> Fixes: 9720b4bc76a83807 ("staging/usbip: convert to kthread") Cc: stable@vger.kernel.org Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> Signed-off-by: Tom Seewald <tseewald@gmail.com> --- drivers/usb/usbip/vudc_sysfs.c | 42 +++++++++++++++++++++++++++------- 1 file changed, 34 insertions(+), 8 deletions(-)