| Message ID | 20210415144415.946778464@linuxfoundation.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
On Thu, 15 Apr 2021 at 17:01, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > From: Shuah Khan <skhan@linuxfoundation.org> > > commit 46613c9dfa964c0c60b5385dbdf5aaa18be52a9c upstream. > > 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. > Hi All, Sorry for reopening an old commit, but I am hoping to learn something here, see below in the code. > 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. > > Fixes: 9720b4bc76a83807 ("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/b1c08b983ffa185449c9f0f7d1021dc8c8454b60.1615171203.git.skhan@linuxfoundation.org > Signed-off-by: Tom Seewald <tseewald@gmail.com> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > drivers/usb/usbip/vudc_sysfs.c | 42 +++++++++++++++++++++++++++++++++-------- > 1 file changed, 34 insertions(+), 8 deletions(-) > > --- a/drivers/usb/usbip/vudc_sysfs.c > +++ b/drivers/usb/usbip/vudc_sysfs.c > @@ -103,8 +103,9 @@ unlock: > } > 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 devic > 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 devic > 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; Here: Isn't such pattern - spin_unlock_irq() followed by spin_unlock_irqrestore() a little risky? The spin_unlock_irq() should unconditionally enable the interrupts. There is therefore a window with few statements with all interrupts enabled. What happens if an interrupt comes exactly now? > + > + spin_unlock_irqrestore(&udc->lock, flags); ...and here: Additionally, the spin_unlock_irqrestore() will now have wrong flags. Assuming interrupts were enabled during spin_lock_irqsave(), the interrupt state is stored in flags, spin_unlock_irq() enabled interrupts and now spin_unlock_irqrestore() gets flags not matching real state. There should be warn_bogus_irq_restore() visible as well. The discussed pattern spin_unlock_irq+spin_unlock_irqrestore was here before, so this is not a comment about this specific patch but the entire usbip code. Best regards, Krzysztof
--- a/drivers/usb/usbip/vudc_sysfs.c +++ b/drivers/usb/usbip/vudc_sysfs.c @@ -103,8 +103,9 @@ unlock: } 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 devic 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 devic 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");