| Message ID | 20240709113851.14691-1-oneukum@suse.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | afdcfd3d6fcdeca2735ca8d994c5f2d24a368f0a |
| Headers | show |
| Series | usb: vhci-hcd: Do not drop references before new references are gained | expand |
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
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
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
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);
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(-)