lkml.org 
[lkml]   [2021]   [Mar]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH 0/6] usbip fixes to crashes found by syzbot
From
Date
On 2021/03/12 14:44, Tetsuo Handa wrote:
> And what you are missing in your [PATCH 4,5,6/6] is
>
> diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
> index c4457026d5ad..3c64bd06ab53 100644
> --- a/drivers/usb/usbip/vhci_sysfs.c
> +++ b/drivers/usb/usbip/vhci_sysfs.c
> @@ -423,6 +423,7 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
> /* end the lock */
>
> wake_up_process(vdev->ud.tcp_rx);
> + schedule_timeout_uninterruptible(HZ); // Consider being preempted here.
> wake_up_process(vdev->ud.tcp_tx);
>
> rh_port_connect(vdev, speed);
>
> . wake_up_process(tcp_tx) can call vhci_shutdown_connection() before wake_up_process(tcp_tx) is called.

wake_up_process(tcp_rx) can call vhci_shutdown_connection() before wake_up_process(tcp_tx) is called.

> Since vhci_shutdown_connection() destroys tcp_tx thread and releases tcp_tx memory via kthread_stop_put(tcp_tx),
> wake_up_process(tcp_tx) will access already freed memory. Your patch converted "NULL pointer dereference caused by
> failing to call kthread_stop_put(tcp_tx)" into "use after free caused by succeeding to call kthread_stop_put(tcp_tx)".
>

And note that

diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index c4457026d5ad..0e1a81d4632c 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -422,11 +422,11 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
spin_unlock_irqrestore(&vhci->lock, flags);
/* end the lock */

- wake_up_process(vdev->ud.tcp_rx);
- wake_up_process(vdev->ud.tcp_tx);
-
rh_port_connect(vdev, speed);

+ wake_up_process(vdev->ud.tcp_tx);
+ wake_up_process(vdev->ud.tcp_rx);
+
return count;
}
static DEVICE_ATTR_WO(attach);

is as well not sufficient, for you are still missing

diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index c4457026d5ad..c958f89a9196 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -422,11 +422,13 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
spin_unlock_irqrestore(&vhci->lock, flags);
/* end the lock */

- wake_up_process(vdev->ud.tcp_rx);
- wake_up_process(vdev->ud.tcp_tx);
+ schedule_timeout_uninterruptible(HZ); // Consider being preempted here.

rh_port_connect(vdev, speed);

+ wake_up_process(vdev->ud.tcp_tx);
+ wake_up_process(vdev->ud.tcp_rx);
+
return count;
}
static DEVICE_ATTR_WO(attach);
because vhci_port_disconnect() from detach_store() can call usbip_event_add(&vdev->ud, VDEV_EVENT_DOWN)
(same use after free bug regarding tcp_tx and tcp_rx) as soon as all shared states are set up and
spinlocks are released.

What you had better consider first is how to protect event_handler()/usbip_sockfd_store()/attach_store()/detach_store() functions
from concurrent calls. Please respond to https://lkml.kernel.org/r/3dab66dc-2981-bc88-a370-4b3178dfd390@i-love.sakura.ne.jp
before you try to make further changes.

\
 
 \ /
  Last update: 2021-03-13 01:51    [W:1.306 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site