[PATCH] usb: vhci-hcd: Do not drop references before new references are gained

[PATCH] usb: vhci-hcd: Do not drop references before new references are gained

[Oliver Neukum]

At a few places the driver carries stale pointers
to references that can still be used. Make sure that does not happen.
This strictly speaking closes ZDI-CAN-22273, though there may be
similar races in the driver.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/usbip/vhci_hcd.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 82650c11e451..302a89aeb258 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -745,6 +745,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 	 *
 	 */
 	if (usb_pipedevice(urb->pipe) == 0) {
+		struct usb_device *old;
 		__u8 type = usb_pipetype(urb->pipe);
 		struct usb_ctrlrequest *ctrlreq =
 			(struct usb_ctrlrequest *) urb->setup_packet;
@@ -755,14 +756,15 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 			goto no_need_xmit;
 		}
 
+		old = vdev->udev;
 		switch (ctrlreq->bRequest) {
 		case USB_REQ_SET_ADDRESS:
 			/* set_address may come when a device is reset */
 			dev_info(dev, "SetAddress Request (%d) to port %d\n",
 				 ctrlreq->wValue, vdev->rhport);
 
-			usb_put_dev(vdev->udev);
 			vdev->udev = usb_get_dev(urb->dev);
+			usb_put_dev(old);
 
 			spin_lock(&vdev->ud.lock);
 			vdev->ud.status = VDEV_ST_USED;
@@ -781,8 +783,8 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 				usbip_dbg_vhci_hc(
 					"Not yet?:Get_Descriptor to device 0 (get max pipe size)\n");
 
-			usb_put_dev(vdev->udev);
 			vdev->udev = usb_get_dev(urb->dev);
+			usb_put_dev(old);
 			goto out;
 
 		default:
@@ -1067,6 +1069,7 @@ static void vhci_shutdown_connection(struct usbip_device *ud)
 static void vhci_device_reset(struct usbip_device *ud)
 {
 	struct vhci_device *vdev = container_of(ud, struct vhci_device, ud);
+	struct usb_device *old = vdev->udev;
 	unsigned long flags;
 
 	spin_lock_irqsave(&ud->lock, flags);
@@ -1074,8 +1077,8 @@ static void vhci_device_reset(struct usbip_device *ud)
 	vdev->speed  = 0;
 	vdev->devid  = 0;
 
-	usb_put_dev(vdev->udev);
 	vdev->udev = NULL;
+	usb_put_dev(old);
 
 	if (ud->tcp_socket) {
 		sockfd_put(ud->tcp_socket);
-- 
2.45.1

Re: [PATCH] usb: vhci-hcd: Do not drop references before new references are gained

[Shuah Khan]

On 7/9/24 05:38, Oliver Neukum wrote:
> At a few places the driver carries stale pointers
> to references that can still be used. Make sure that does not happen.
> This strictly speaking closes ZDI-CAN-22273, though there may be
> similar races in the driver.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>

Sorry I need a bit more explanation to follow the change you
are making. Also how did you find the problem?

> ---
>   drivers/usb/usbip/vhci_hcd.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> index 82650c11e451..302a89aeb258 100644
> --- a/drivers/usb/usbip/vhci_hcd.c
> +++ b/drivers/usb/usbip/vhci_hcd.c
> @@ -745,6 +745,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>   	 *
>   	 */
>   	if (usb_pipedevice(urb->pipe) == 0) {
> +		struct usb_device *old;
>   		__u8 type = usb_pipetype(urb->pipe);
>   		struct usb_ctrlrequest *ctrlreq =
>   			(struct usb_ctrlrequest *) urb->setup_packet;
> @@ -755,14 +756,15 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>   			goto no_need_xmit;
>   		}
>   
> +		old = vdev->udev;
>   		switch (ctrlreq->bRequest) {
>   		case USB_REQ_SET_ADDRESS:
>   			/* set_address may come when a device is reset */
>   			dev_info(dev, "SetAddress Request (%d) to port %d\n",
>   				 ctrlreq->wValue, vdev->rhport);
>   
> -			usb_put_dev(vdev->udev);
>   			vdev->udev = usb_get_dev(urb->dev);
> +			usb_put_dev(old);
>   
>   			spin_lock(&vdev->ud.lock);
>   			vdev->ud.status = VDEV_ST_USED;
> @@ -781,8 +783,8 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>   				usbip_dbg_vhci_hc(
>   					"Not yet?:Get_Descriptor to device 0 (get max pipe size)\n");
>   
> -			usb_put_dev(vdev->udev);
>   			vdev->udev = usb_get_dev(urb->dev);
> +			usb_put_dev(old);
>   			goto out;
>   
>   		default:
> @@ -1067,6 +1069,7 @@ static void vhci_shutdown_connection(struct usbip_device *ud)
>   static void vhci_device_reset(struct usbip_device *ud)
>   {
>   	struct vhci_device *vdev = container_of(ud, struct vhci_device, ud);
> +	struct usb_device *old = vdev->udev;
>   	unsigned long flags;
>   
>   	spin_lock_irqsave(&ud->lock, flags);
> @@ -1074,8 +1077,8 @@ static void vhci_device_reset(struct usbip_device *ud)
>   	vdev->speed  = 0;
>   	vdev->devid  = 0;
>   
> -	usb_put_dev(vdev->udev);
>   	vdev->udev = NULL;
> +	usb_put_dev(old);
>   
>   	if (ud->tcp_socket) {
>   		sockfd_put(ud->tcp_socket);

thanks,
-- Shuah

Re: [PATCH] usb: vhci-hcd: Do not drop references before new references are gained

[Oliver Neukum]

On 09.07.24 21:38, Shuah Khan wrote:
> On 7/9/24 05:38, Oliver Neukum wrote:
>> At a few places the driver carries stale pointers
>> to references that can still be used. Make sure that does not happen.
>> This strictly speaking closes ZDI-CAN-22273, though there may be
>> similar races in the driver.
>>
>> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> 
> Sorry I need a bit more explanation to follow the change you
> are making. Also how did you find the problem?

Hi,

I looked at the initial report you wrote and it seemed to me that the issue
was that vhci_device_reset() leaves a stale pointer around and vhci_urb_enqueue()
in the special cases drops the old reference before it gets a new reference,
which together causes the race condition you were hitting.

	Regards
		Oliver

Re: [PATCH] usb: vhci-hcd: Do not drop references before new references are gained

[Shuah Khan]

On 7/16/24 08:33, Oliver Neukum wrote:
> 
> 
> On 09.07.24 21:38, Shuah Khan wrote:
>> On 7/9/24 05:38, Oliver Neukum wrote:
>>> At a few places the driver carries stale pointers
>>> to references that can still be used. Make sure that does not happen.
>>> This strictly speaking closes ZDI-CAN-22273, though there may be
>>> similar races in the driver.
>>>
>>> Signed-off-by: Oliver Neukum <oneukum@suse.com>
>>
>> Sorry I need a bit more explanation to follow the change you
>> are making. Also how did you find the problem?
> 
> Hi,
> 
> I looked at the initial report you wrote and it seemed to me that the issue
> was that vhci_device_reset() leaves a stale pointer around and vhci_urb_enqueue()
> in the special cases drops the old reference before it gets a new reference,
> which together causes the race condition you were hitting.
> 

Thank you.

Acked-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah