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

[Shuah Khan <skhan@linuxfoundation.org>]

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