| Message ID | 20210205135707.4574-1-penguin-kernel@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: usbip: fix error handling of kthread_get_run() | expand |
On 2/5/21 6:57 AM, Tetsuo Handa wrote: > syzbot is reporting a crash at vhci_shutdown_connection() [1]. It turned > out that it was not a NULL pointer dereference but an ERR_PTR(-EINTR) > pointer dereference, 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 meant a valid task > pointer. Therefore, we should make kthread_get_run() return NULL when > kthread_create() failed. > > [1] https://syzkaller.appspot.com/bug?id=c21c07f3d51769405e8efc027bdb927515dcc7d6 > > Reported-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com> > Reported-by: syzbot <syzbot+bf1a360e305ee719e364@syzkaller.appspotmail.com> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Fixes: 9720b4bc76a83807 ("staging/usbip: convert to kthread") > Cc: Arnd Bergmann <arnd@arndb.de> > --- > drivers/usb/usbip/usbip_common.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h > index 8be857a4fa13..a7c68097aa1d 100644 > --- a/drivers/usb/usbip/usbip_common.h > +++ b/drivers/usb/usbip/usbip_common.h > @@ -286,6 +286,8 @@ struct usbip_device { > if (!IS_ERR(__k)) { \ > get_task_struct(__k); \ > wake_up_process(__k); \ > + } else { \ > + __k = NULL; \ > } \ > __k; \ > }) > Good find. For this fix to be complete, you will have to add checks for kthread_get_run() NULL return in attach_store() and usbip_sockfd_store() routines in stub_dev.c and vudc_sysfs.c thanks, -- Shuah
On 2021/02/06 1:27, Shuah Khan wrote: > Good find. For this fix to be complete, you will have to add checks > for kthread_get_run() NULL return in attach_store() and > usbip_sockfd_store() routines in stub_dev.c and vudc_sysfs.c Initially I thought that the cleaner fix is to get kthread_create() out of kthread_get_run() ( the drivers/usb/usbip/vhci_sysfs.c portion in https://syzkaller.appspot.com/x/patch.diff?x=16c3c090d00000 ) so that we can undo kthread_create() via kthread_stop(). But I found that such fix makes little sense because it is possible that SIGKILL is delivered between vhci_rx_loop() and vhci_tx_loop() have started and before leaving attach_store(). Since the code prior to "staging/usbip: convert to kthread" was already capable of surviving such race condition, this patch should be already good enough for sending to stable kernels. Of course, since kthread_create() may return -ENOMEM without being SIGKILLed, we could update attach_store() to report kthread_get_run() failure to the caller, but that will be a separate patch. This patch alone avoids the crash although there is a hung task problem similar to https://syzkaller.appspot.com/bug?id=5677eeeb83e5d47ef2b04e9bd68f5ff4c7e572ab remains ( https://syzkaller.appspot.com/text?tag=CrashReport&x=17aa3f78d00000 ). The cause of hung task is currently unknown; maybe too much printk() messages.
On 2/5/21 6:08 PM, Tetsuo Handa wrote: > On 2021/02/06 1:27, Shuah Khan wrote: >> Good find. For this fix to be complete, you will have to add checks >> for kthread_get_run() NULL return in attach_store() and >> usbip_sockfd_store() routines in stub_dev.c and vudc_sysfs.c > > Initially I thought that the cleaner fix is to get kthread_create() out of kthread_get_run() > ( the drivers/usb/usbip/vhci_sysfs.c portion in > https://syzkaller.appspot.com/x/patch.diff?x=16c3c090d00000 ) so that we can undo > kthread_create() via kthread_stop(). But I found that such fix makes little sense because > it is possible that SIGKILL is delivered between vhci_rx_loop() and vhci_tx_loop() have > started and before leaving attach_store(). > > Since the code prior to "staging/usbip: convert to kthread" was already capable of surviving > such race condition, this patch should be already good enough for sending to stable kernels. > Of course, since kthread_create() may return -ENOMEM without being SIGKILLed, we could update > attach_store() to report kthread_get_run() failure to the caller, but that will be a separate > patch. This patch alone avoids the crash although there is a hung task problem similar to > https://syzkaller.appspot.com/bug?id=5677eeeb83e5d47ef2b04e9bd68f5ff4c7e572ab remains > ( https://syzkaller.appspot.com/text?tag=CrashReport&x=17aa3f78d00000 ). The cause of hung > task is currently unknown; maybe too much printk() messages. > I would like to see to see a complete fix. This patch changes kthread_get_run() to return NULL. Without adding handling for NULL in the callers of kthread_get_run(), we will start seeing problems. Does this patch fix the problem syzbot found? thanks, -- Shuah
On 2021/02/11 3:11, Shuah Khan wrote: > I would like to see to see a complete fix. This patch changes > kthread_get_run() to return NULL. Without adding handling for > NULL in the callers of kthread_get_run(), we will start seeing > problems. What problems are you aware of? > > Does this patch fix the problem syzbot found? Yes, this patch as-is avoids the crash syzbot found.
On 2/10/21 11:16 AM, Tetsuo Handa wrote: > On 2021/02/11 3:11, Shuah Khan wrote: >> I would like to see to see a complete fix. This patch changes >> kthread_get_run() to return NULL. Without adding handling for >> NULL in the callers of kthread_get_run(), we will start seeing >> problems. > > What problems are you aware of? > The fact that driver doesn't cleanup after failing to create the thread is a problem. >> >> Does this patch fix the problem syzbot found? > > Yes, this patch as-is avoids the crash syzbot found. > Good to know. Please add handling for kthread_get_run() return in the places I suggested in you next version of this patch. thanks, -- Shuah
On 2021/02/11 3:20, Shuah Khan wrote: > On 2/10/21 11:16 AM, Tetsuo Handa wrote: >> On 2021/02/11 3:11, Shuah Khan wrote: >>> I would like to see to see a complete fix. This patch changes >>> kthread_get_run() to return NULL. Without adding handling for >>> NULL in the callers of kthread_get_run(), we will start seeing >>> problems. >> >> What problems are you aware of? >> > > The fact that driver doesn't cleanup after failing to create > the thread is a problem. What are the cleanup functions? Future attach_store() will succeed if cleanup operation (which does vdev->ud.status = VDEV_ST_NULL;) is done, doesn't it? And vhci_device_reset() and/or vhci_device_init() involves cleanup operation (which does vdev->ud.status = VDEV_ST_NULL;), doesn't it? > >>> >>> Does this patch fix the problem syzbot found? >> >> Yes, this patch as-is avoids the crash syzbot found. >> > > Good to know. Please add handling for kthread_get_run() return > in the places I suggested in you next version of this patch. Since vhci_{rx,tx}_loop() do not involve cleanup operation (they simply terminate upon kthread_should_stop() == true), I don't understand why failing to start vhci_{rx,tx}_loop() makes difference. Cleanup will be done by functions other than vhci_{rx,tx}_loop(), won't it?
On 2/10/21 11:43 AM, Tetsuo Handa wrote: > On 2021/02/11 3:20, Shuah Khan wrote: >> On 2/10/21 11:16 AM, Tetsuo Handa wrote: >>> On 2021/02/11 3:11, Shuah Khan wrote: >>>> I would like to see to see a complete fix. This patch changes >>>> kthread_get_run() to return NULL. Without adding handling for >>>> NULL in the callers of kthread_get_run(), we will start seeing >>>> problems. >>> >>> What problems are you aware of? >>> >> >> The fact that driver doesn't cleanup after failing to create >> the thread is a problem. > > What are the cleanup functions? > When user-space requests attaching to a device, attach_store() tries to attach the requested device. When kthread_get_run() failure is ignored silently, and continue with call to rh_port_connect(), user-space assumes attach is successful. User thinks attach is successful. When and how will this attach failure gets reported to the in this scenario? Error handling for this case is no different from other error paths in attach_store(). Please see error handling for other errors in attach_store(). In this case the right error handling is to rewind the vdev init and bail out returning error. This would include setting vdev->ud.status to VDEV_ST_NULL. I found the following reproducer that tells me how attach is triggered. https://syzkaller.appspot.com/text?tag=ReproC&x=128506e4d00000 syzbot is helping us harden these paths, which is awesome. Fixing these have to consider user api. I you would like to fix this, please send me a complete fix. thanks, -- Shuah
On 2021/02/11 5:15, Shuah Khan wrote: > On 2/10/21 11:43 AM, Tetsuo Handa wrote: >> On 2021/02/11 3:20, Shuah Khan wrote: >>> On 2/10/21 11:16 AM, Tetsuo Handa wrote: >>>> On 2021/02/11 3:11, Shuah Khan wrote: >>>>> I would like to see to see a complete fix. This patch changes >>>>> kthread_get_run() to return NULL. Without adding handling for >>>>> NULL in the callers of kthread_get_run(), we will start seeing >>>>> problems. >>>> >>>> What problems are you aware of? >>>> >>> >>> The fact that driver doesn't cleanup after failing to create >>> the thread is a problem. >> >> What are the cleanup functions? >> > > When user-space requests attaching to a device, attach_store() > tries to attach the requested device. When kthread_get_run() > failure is ignored silently, and continue with call to > rh_port_connect(), user-space assumes attach is successful. > User thinks attach is successful. The struct kthread_create_info *create = kmalloc(sizeof(*create), GFP_KERNEL); in __kthread_create_on_node() never fails unless killed by the OOM killer due to the "too small to fail" memory-allocation rule, and wait_for_completion_killable(&done) in __kthread_create_on_node() never fails unless killed. Creating a kernel thread as root user unlikely fails, and memory allocations by that kernel thread also never fails due to the "too small to fail" memory-allocation rule. Therefore, kthread_get_run() effectively fails only when current thread which called attach_store() was killed. And > > When and how will this attach failure gets reported to the > in this scenario? if the current thread was killed, how can the failure get reported to the user-space in this scenario? > > Error handling for this case is no different from other error > paths in attach_store(). > > Please see error handling for other errors in attach_store(). Being "killed" means that user-space can never know the result unlike other error paths in attach_store(). > In this case the right error handling is to rewind the vdev > init and bail out returning error. This would include setting > vdev->ud.status to VDEV_ST_NULL. If the user-space was killed, the kernel is responsible for offering automatic cleanup which includes setting vdev->ud.status to VDEV_ST_NULL. > > I found the following reproducer that tells me how attach > is triggered. > https://syzkaller.appspot.com/text?tag=ReproC&x=128506e4d00000 This reproducer (which is killed after 5 seconds from fork()) uses only /sys/devices/platform/vhci_hcd.0/attach interface and never uses detach interface. Detach and cleanup are up to automatic cleanup offered by the kernel. > > syzbot is helping us harden these paths, which is awesome. > Fixing these have to consider user api. > > 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.
On 2021/02/11 10:04, Tetsuo Handa wrote: > On 2021/02/11 5:15, Shuah Khan wrote: >> 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. drivers/usb/usbip/stub_dev.c | 61 ++++++++++++++++++++++------------ drivers/usb/usbip/vhci_sysfs.c | 33 +++++++++++++++--- drivers/usb/usbip/vudc_sysfs.c | 35 +++++++++++++++---- 3 files changed, 95 insertions(+), 34 deletions(-) diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c index 2305d425e6c9..72c561878df7 100644 --- a/drivers/usb/usbip/stub_dev.c +++ b/drivers/usb/usbip/stub_dev.c @@ -39,77 +39,94 @@ static DEVICE_ATTR_RO(usbip_status); * is used to transfer usbip requests by kernel threads. -1 is a magic number * by which usbip connection is finished. */ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { struct stub_device *sdev = dev_get_drvdata(dev); int sockfd = 0; struct socket *socket; int rv; + int err; + struct task_struct *tx = NULL; + struct task_struct *rx = NULL; if (!sdev) { dev_err(dev, "sdev is null\n"); return -ENODEV; } rv = sscanf(buf, "%d", &sockfd); if (rv != 1) return -EINVAL; if (sockfd != -1) { - int err; + socket = sockfd_lookup(sockfd, &err); + if (!socket) + return -EINVAL; + + /* Create threads now. */ + rx = kthread_create(stub_rx_loop, &sdev->ud, "stub_rx"); + tx = kthread_create(stub_tx_loop, &sdev->ud, "stub_tx"); + if (IS_ERR(rx) || IS_ERR(tx)) + goto thread_error; dev_info(dev, "stub up\n"); spin_lock_irq(&sdev->ud.lock); if (sdev->ud.status != SDEV_ST_AVAILABLE) { + spin_unlock_irq(&sdev->ud.lock); dev_err(dev, "not ready\n"); - goto err; + err = -EINVAL; + goto thread_error; } - socket = sockfd_lookup(sockfd, &err); - if (!socket) - goto err; - sdev->ud.tcp_socket = socket; sdev->ud.sockfd = sockfd; - - spin_unlock_irq(&sdev->ud.lock); - - sdev->ud.tcp_rx = kthread_get_run(stub_rx_loop, &sdev->ud, - "stub_rx"); - sdev->ud.tcp_tx = kthread_get_run(stub_tx_loop, &sdev->ud, - "stub_tx"); - - spin_lock_irq(&sdev->ud.lock); sdev->ud.status = SDEV_ST_USED; spin_unlock_irq(&sdev->ud.lock); + /* Start the threads. */ + get_task_struct(rx); + sdev->ud.tcp_rx = rx; + wake_up_process(rx); + get_task_struct(tx); + sdev->ud.tcp_tx = tx; + wake_up_process(tx); + } else { dev_info(dev, "stub down\n"); spin_lock_irq(&sdev->ud.lock); - if (sdev->ud.status != SDEV_ST_USED) - goto err; - + if (sdev->ud.status != SDEV_ST_USED) { + spin_unlock_irq(&sdev->ud.lock); + return -EINVAL; + } spin_unlock_irq(&sdev->ud.lock); + /* Race warning: sdev->ud.status == SDEV_ST_USED may be no longer true. */ usbip_event_add(&sdev->ud, SDEV_EVENT_DOWN); } return count; - -err: - spin_unlock_irq(&sdev->ud.lock); - return -EINVAL; +thread_error: + sockfd_put(socket); + if (IS_ERR(rx)) + err = PTR_ERR(rx); + else if (rx) + kthread_stop(rx); + if (IS_ERR(tx)) + err = PTR_ERR(tx); + else if (tx) + kthread_stop(tx); + return err; } static DEVICE_ATTR_WO(usbip_sockfd); static struct attribute *usbip_attrs[] = { &dev_attr_usbip_status.attr, &dev_attr_usbip_sockfd.attr, &dev_attr_usbip_debug.attr, NULL, }; ATTRIBUTE_GROUPS(usbip); diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c index be37aec250c2..0d10021c4186 100644 --- a/drivers/usb/usbip/vhci_sysfs.c +++ b/drivers/usb/usbip/vhci_sysfs.c @@ -305,20 +305,22 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr, { struct socket *socket; int sockfd = 0; __u32 port = 0, pdev_nr = 0, rhport = 0, devid = 0, speed = 0; struct usb_hcd *hcd; struct vhci_hcd *vhci_hcd; struct vhci_device *vdev; struct vhci *vhci; int err; unsigned long flags; + struct task_struct *tx; + struct task_struct *rx; /* * @rhport: port number of vhci_hcd * @sockfd: socket descriptor of an established TCP connection * @devid: unique device identifier in a remote host * @speed: usb device speed in a remote host */ if (sscanf(buf, "%u %u %u %u", &port, &sockfd, &devid, &speed) != 4) return -EINVAL; pdev_nr = port_to_pdev_nr(port); @@ -345,62 +347,83 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr, if (speed == USB_SPEED_SUPER) vdev = &vhci->vhci_hcd_ss->vdev[rhport]; else vdev = &vhci->vhci_hcd_hs->vdev[rhport]; /* Extract socket from fd. */ socket = sockfd_lookup(sockfd, &err); if (!socket) return -EINVAL; + /* Create threads now. */ + rx = kthread_create(vhci_rx_loop, &vdev->ud, "vhci_rx"); + tx = kthread_create(vhci_tx_loop, &vdev->ud, "vhci_tx"); + if (IS_ERR(rx) || IS_ERR(tx)) + goto thread_error; + /* now need lock until setting vdev status as used */ /* begin a lock */ spin_lock_irqsave(&vhci->lock, flags); spin_lock(&vdev->ud.lock); if (vdev->ud.status != VDEV_ST_NULL) { /* end of the lock */ spin_unlock(&vdev->ud.lock); spin_unlock_irqrestore(&vhci->lock, flags); - sockfd_put(socket); - dev_err(dev, "port %d already used\n", rhport); /* * Will be retried from userspace * if there's another free port. */ - return -EBUSY; + err = -EBUSY; + goto thread_error; } dev_info(dev, "pdev(%u) rhport(%u) sockfd(%d)\n", pdev_nr, rhport, sockfd); dev_info(dev, "devid(%u) speed(%u) speed_str(%s)\n", devid, speed, usb_speed_string(speed)); vdev->devid = devid; vdev->speed = speed; vdev->ud.sockfd = sockfd; vdev->ud.tcp_socket = socket; vdev->ud.status = VDEV_ST_NOTASSIGNED; spin_unlock(&vdev->ud.lock); spin_unlock_irqrestore(&vhci->lock, flags); /* end the lock */ - vdev->ud.tcp_rx = kthread_get_run(vhci_rx_loop, &vdev->ud, "vhci_rx"); - vdev->ud.tcp_tx = kthread_get_run(vhci_tx_loop, &vdev->ud, "vhci_tx"); + /* Start the threads. */ + get_task_struct(rx); + vdev->ud.tcp_rx = rx; + wake_up_process(rx); + get_task_struct(tx); + vdev->ud.tcp_tx = tx; + wake_up_process(tx); 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]; }; diff --git a/drivers/usb/usbip/vudc_sysfs.c b/drivers/usb/usbip/vudc_sysfs.c index 100f680c572a..d01acb6d6b1c 100644 --- a/drivers/usb/usbip/vudc_sysfs.c +++ b/drivers/usb/usbip/vudc_sysfs.c @@ -93,29 +93,40 @@ static BIN_ATTR_RO(dev_desc, sizeof(struct usb_device_descriptor)); static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *attr, const char *in, size_t count) { struct vudc *udc = (struct vudc *) dev_get_drvdata(dev); int rv; int sockfd = 0; int err; struct socket *socket; unsigned long flags; int ret; + struct task_struct *tx = NULL; + struct task_struct *rx = NULL; rv = kstrtoint(in, 0, &sockfd); if (rv != 0) return -EINVAL; if (!udc) { dev_err(dev, "no device"); return -ENODEV; } + + /* Create threads now. */ + if (sockfd != -1) { + rx = kthread_create(&v_rx_loop, &udc->ud, "vudc_rx"); + tx = kthread_create(&v_tx_loop, &udc->ud, "vudc_tx"); + if (IS_ERR(rx) || IS_ERR(tx)) + goto thread_error; + } + spin_lock_irqsave(&udc->lock, flags); /* Don't export what we don't have */ if (!udc->driver || !udc->pullup) { dev_err(dev, "gadget not bound"); ret = -ENODEV; goto unlock; } if (sockfd != -1) { if (udc->connected) { @@ -132,33 +143,34 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a } socket = sockfd_lookup(sockfd, &err); if (!socket) { dev_err(dev, "failed to lookup sock"); ret = -EINVAL; goto unlock_ud; } udc->ud.tcp_socket = socket; + udc->ud.status = SDEV_ST_USED; 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"); + /* Start the threads. */ + get_task_struct(rx); + udc->ud.tcp_rx = rx; + wake_up_process(rx); + get_task_struct(tx); + udc->ud.tcp_tx = tx; + wake_up_process(tx); spin_lock_irqsave(&udc->lock, flags); - spin_lock_irq(&udc->ud.lock); - udc->ud.status = SDEV_ST_USED; - spin_unlock_irq(&udc->ud.lock); ktime_get_ts64(&udc->start_time); v_start_timer(udc); udc->connected = 1; } else { if (!udc->connected) { dev_err(dev, "Device not connected"); ret = -EINVAL; goto unlock; } @@ -174,20 +186,29 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a } spin_unlock_irqrestore(&udc->lock, flags); return count; unlock_ud: spin_unlock_irq(&udc->ud.lock); unlock: spin_unlock_irqrestore(&udc->lock, flags); +thread_error: + if (IS_ERR(rx)) + ret = PTR_ERR(rx); + else if (rx) + kthread_stop(rx); + if (IS_ERR(tx)) + ret = PTR_ERR(tx); + else if (tx) + kthread_stop(tx); return ret; } static DEVICE_ATTR_WO(usbip_sockfd); static ssize_t usbip_status_show(struct device *dev, struct device_attribute *attr, char *out) { struct vudc *udc = (struct vudc *) dev_get_drvdata(dev); int status;
On 2021/02/11 12:01, Tetsuo Handa wrote: > On 2021/02/11 10:04, Tetsuo Handa wrote: >> On 2021/02/11 5:15, Shuah Khan wrote: >>> 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... > diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c > index be37aec250c2..0d10021c4186 100644 > --- a/drivers/usb/usbip/vhci_sysfs.c > +++ b/drivers/usb/usbip/vhci_sysfs.c > @@ -305,20 +305,22 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr, > { > struct socket *socket; > int sockfd = 0; > __u32 port = 0, pdev_nr = 0, rhport = 0, devid = 0, speed = 0; > struct usb_hcd *hcd; > struct vhci_hcd *vhci_hcd; > struct vhci_device *vdev; > struct vhci *vhci; > int err; > unsigned long flags; > + struct task_struct *tx; > + struct task_struct *rx; > > /* > * @rhport: port number of vhci_hcd > * @sockfd: socket descriptor of an established TCP connection > * @devid: unique device identifier in a remote host > * @speed: usb device speed in a remote host > */ > if (sscanf(buf, "%u %u %u %u", &port, &sockfd, &devid, &speed) != 4) > return -EINVAL; > pdev_nr = port_to_pdev_nr(port); > @@ -345,62 +347,83 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr, > if (speed == USB_SPEED_SUPER) > vdev = &vhci->vhci_hcd_ss->vdev[rhport]; > else > vdev = &vhci->vhci_hcd_hs->vdev[rhport]; > > /* Extract socket from fd. */ > socket = sockfd_lookup(sockfd, &err); > if (!socket) > return -EINVAL; > > + /* Create threads now. */ > + rx = kthread_create(vhci_rx_loop, &vdev->ud, "vhci_rx"); > + tx = kthread_create(vhci_tx_loop, &vdev->ud, "vhci_tx"); > + if (IS_ERR(rx) || IS_ERR(tx)) > + goto thread_error; > + > /* now need lock until setting vdev status as used */ > > /* begin a lock */ > spin_lock_irqsave(&vhci->lock, flags); > spin_lock(&vdev->ud.lock); > > if (vdev->ud.status != VDEV_ST_NULL) { > /* end of the lock */ > spin_unlock(&vdev->ud.lock); > spin_unlock_irqrestore(&vhci->lock, flags); > > - sockfd_put(socket); > - > dev_err(dev, "port %d already used\n", rhport); > /* > * Will be retried from userspace > * if there's another free port. > */ > - return -EBUSY; > + err = -EBUSY; > + goto thread_error; > } > > dev_info(dev, "pdev(%u) rhport(%u) sockfd(%d)\n", > pdev_nr, rhport, sockfd); > dev_info(dev, "devid(%u) speed(%u) speed_str(%s)\n", > devid, speed, usb_speed_string(speed)); > > vdev->devid = devid; > vdev->speed = speed; > vdev->ud.sockfd = sockfd; > vdev->ud.tcp_socket = socket; > vdev->ud.status = VDEV_ST_NOTASSIGNED; > > spin_unlock(&vdev->ud.lock); > spin_unlock_irqrestore(&vhci->lock, flags); > /* end the lock */ > > - vdev->ud.tcp_rx = kthread_get_run(vhci_rx_loop, &vdev->ud, "vhci_rx"); > - vdev->ud.tcp_tx = kthread_get_run(vhci_tx_loop, &vdev->ud, "vhci_tx"); > + /* Start the threads. */ > + get_task_struct(rx); > + vdev->ud.tcp_rx = rx; > + wake_up_process(rx); 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]; > }; >
diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h index 8be857a4fa13..a7c68097aa1d 100644 --- a/drivers/usb/usbip/usbip_common.h +++ b/drivers/usb/usbip/usbip_common.h @@ -286,6 +286,8 @@ struct usbip_device { if (!IS_ERR(__k)) { \ get_task_struct(__k); \ wake_up_process(__k); \ + } else { \ + __k = NULL; \ } \ __k; \ })
syzbot is reporting a crash at vhci_shutdown_connection() [1]. It turned out that it was not a NULL pointer dereference but an ERR_PTR(-EINTR) pointer dereference, 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 meant a valid task pointer. Therefore, we should make kthread_get_run() return NULL when kthread_create() failed. [1] https://syzkaller.appspot.com/bug?id=c21c07f3d51769405e8efc027bdb927515dcc7d6 Reported-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com> Reported-by: syzbot <syzbot+bf1a360e305ee719e364@syzkaller.appspotmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Fixes: 9720b4bc76a83807 ("staging/usbip: convert to kthread") Cc: Arnd Bergmann <arnd@arndb.de> --- drivers/usb/usbip/usbip_common.h | 2 ++ 1 file changed, 2 insertions(+)