Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider: - [Critical] usb: virtio: DMA race and use-after-free due to fake early giveback - [Critical] usb: virtio: use-after-free of vudc->veps during device unbind - [Critical] usb: virtio: use-after-free in async cancel work due to missing refcount - [Critical] usb: virtio: list corruption between do_real_queue() and ep_disable - [High] usb: virtio: sleep in atomic context in virtio_ep_disable() - [High] usb: virtio: SETUP serialization fails due to un-checked pend_queue - [High] usb: virtio: deadlock and double-free on queueing to halted endpoint - [High] usb: virtio: vep_disable_work() disables wrong endpoint on host - [High] usb: virtio: TOCTOU race queues requests after endpoint disable - [High] usb: virtio: EP0 permanently hangs if enable_work is cancelled --
commit e8d36f763a91b5d6a7067cc2771333012c0636ae Author: Igor Skalkin <[email protected]> virtio-usb: rework endpoint lifecycle to an async split-phase state machine This commit changes the virtio-usb gadget driver's endpoint lifecycle to an asynchronous state machine to avoid sleeping in atomic gadget callbacks. Endpoint enable, disable, and queue operations now rely on workqueues and split-phase request handling. > diff --git a/drivers/usb/virtio_usb/device.c b/drivers/usb/virtio_usb/device.c > index 5ae0fc89bc287..9ee1f050019f1 100644 > --- a/drivers/usb/virtio_usb/device.c > +++ b/drivers/usb/virtio_usb/device.c [ ... ] > +static bool virtio_usb_dc_req_queue_empty(struct virtio_usb_ep *vep) > +{ > + struct virtio_usb_dc *vudc = vep->vudc; > + unsigned long flags; > + bool empty; > + > + spin_lock_irqsave(&vudc->lock, flags); > + empty = list_empty(&vep->req_queue); > + spin_unlock_irqrestore(&vudc->lock, flags); > + > + return empty; > +} [Severity: High] Does this logic ignore the pend_queue entirely? If another endpoint is EP_ENABLING, EP0 requests are deferred into pend_queue. Could virtio_usb_dc_wait_ep0_idle() mistakenly see an empty req_queue and allow a new SETUP packet to overwrite the previous control transfer state while it is still legitimately pending? [ ... ] > +static int do_real_queue(struct virtio_usb_ep *vep, struct usb_request *req) > +{ [ ... ] > + rc = virtio_usb_data_send(vudc->vusb, vreq, out_sgs, in_sgs); > + if (rc) { > + spin_lock_irqsave(&vudc->lock, flags); > + list_del_init(&vreq->list); > + spin_unlock_irqrestore(&vudc->lock, flags); [Severity: Critical] Can this error path corrupt the list if it races with virtio_ep_disable()? If virtio_usb_data_send() fails concurrently, virtio_ep_disable() might have already moved vreq into a local stack list (giveback_inflight) and dropped the lock to iterate over it locklessly. Calling list_del_init() here on vreq->list would mutate the elements of that local list, destroying the pointers. [ ... ] > +static void vep_disable_work(struct work_struct *work) > +{ > + struct virtio_usb_ep *vep = > + container_of(work, struct virtio_usb_ep, disable_work); > + struct virtio_usb_dc *vudc = vep->vudc; > + struct virtio_usb_cmd *cmd; > + struct virtio_usb_dev_cmd_hdr *hdr; > + u16 endpoint; > + > + if (!READ_ONCE(vudc->driver) || !vudc->registered) > + return; [Severity: Critical] Is it possible for vep to be a dangling pointer here? During device unbind (VIRTIO_USB_EVT_DEV_UNBIND), the driver calls kfree(vudc->veps) after cancelling only enable_work. If a disable_work was already queued, the workqueue could execute this function after vudc->veps has been freed, resulting in a use-after-free. > + endpoint = vep->ep_id | > + (usb_ep_dir_in(&vep->ep) ? VIRTIO_USB_EP_DIR_IN : > + VIRTIO_USB_EP_DIR_OUT); [Severity: High] Can usb_ep_dir_in() safely calculate the direction here? Gadget drivers generally clear ep->desc to NULL immediately after returning from usb_ep_disable(). Because this block runs asynchronously, ep->desc will likely be NULL, causing usb_ep_dir_in() to fall back to the OUT direction (0). If the target was an IN endpoint, won't this send the disable command for the wrong endpoint on the host? [ ... ] > static int virtio_ep_disable(struct usb_ep *ep) > { [ ... ] > + if (vep->state == EP_ENABLING) { > + /* > + * cancel_work_sync() can sleep and must not be called while > + * holding vudc->lock: vep_enable_work() only re-acquires > + * this same lock after its (possibly blocking) virtio > + * command completes, so holding the lock here across > + * cancel_work_sync() would deadlock against it. > + */ > + spin_unlock_irqrestore(&vudc->lock, flags); > + cancel_work_sync(&vep->enable_work); > + spin_lock_irqsave(&vudc->lock, flags); > + } [Severity: High] Is it safe to call cancel_work_sync() here? The USB gadget API specifies that usb_ep_ops->disable is atomic and can be called from interrupt context. Calling cancel_work_sync(), which can sleep, violates this API contract and can trigger a scheduling while atomic bug. [Severity: High] Additionally, if enable_work is forcefully cancelled here, how do the EP0 requests deferred into pend_queue get flushed? Does this leave EP0 permanently hung since virtio_usb_dc_flush_ep0_pend() is never called to release the stranded requests? [ ... ] > + /* > + * in-flight requests - make early giveback, remove from req_queue, > + * mark completed_early, but do not unref: unref will be in complete > + * handler. > + */ > list_for_each_entry_safe(vreq, vreq_tmp, &vep->req_queue, list) { > priv = virtio_usb_data_priv(vreq); > + priv->req.status = -ESHUTDOWN; > + priv->completed_early = true; > + list_del_init(&vreq->list); > + list_add_tail(&vreq->list, &giveback_inflight); > } [ ... ] > + /* in-flight: giveback now, unref will be in complete_req */ > + list_for_each_entry_safe(vreq, vreq_tmp, &giveback_inflight, list) { > + struct virtio_usb_dc_priv *p = virtio_usb_data_priv(vreq); > > - rc = virtio_usb_cmd_send_sync(vudc->vusb, NULL, NULL, cmd); > + list_del_init(&vreq->list); > + usb_gadget_giveback_request(&vep->ep, &p->req); > + } [Severity: Critical] Could this early giveback result in immediate hardware memory corruption? By handing the requests back to the gadget driver via usb_gadget_giveback_request() before the hardware backend processes the async disable command, the gadget driver may reuse or free the DMA buffers while the virtqueue is still actively reading or writing to them. [ ... ] > static int virtio_ep_queue(struct usb_ep *ep, struct usb_request *req, > gfp_t mem_flags) > { [ ... ] > + case EP_ENABLED: > + if (!vep->ep_id && virtio_usb_dc_ep_enabling_locked(vudc)) { [ ... ] > + } > + spin_unlock_irqrestore(&vudc->lock, flags); > + rc = do_real_queue(vep, req); > + /* if rc==0 - unref will be in completion, on failure > + * do_real_queue() has already dropped its own ref > + */ > + return rc; [Severity: High] Does this lock drop introduce a Time-of-Check to Time-of-Use race? Because the lock is dropped before do_real_queue() re-acquires it to add the request, a concurrent virtio_ep_disable() could mark the endpoint disabled and flush the queues. The queued request would then be blindly sent to the hardware anyway. [ ... ] > + case EP_HALTED: > + spin_unlock_irqrestore(&vudc->lock, flags); > + req->status = vep->last_err ? vep->last_err : -EPIPE; > + usb_gadget_giveback_request(&vep->ep, req); > + return req->status; [Severity: High] Does returning an error code while also performing a giveback violate the gadget API? The USB gadget API expects that if queue() returns an error, the controller did not take ownership and must not call the completion callback. The synchronous giveback could cause a deadlock if it attempts to re-acquire the caller's locks, and returning an error will likely cause the caller to free the request, resulting in a double-free. [ ... ] > static int virtio_ep_dequeue(struct usb_ep *ep, struct usb_request *req) > { [ ... ] > - if (rc) > + if (rc) // request not found in req_queue > return rc; > > - vreq = priv->vreq; > + schedule_work(&priv->cancel_work); > > - cmd = virtio_usb_dc_cmd_alloc(vudc, VIRTIO_USB_CMD_DEV_CANCEL, > - GFP_KERNEL); [Severity: Critical] Does scheduling priv->cancel_work without taking a reference to vreq risk a use-after-free? If the hardware naturally completes the request before this async cancel work executes, the completion handler will drop the final reference and the gadget driver will free the request. When virtio_usb_dc_cancel_work() later runs, it would access the freed priv memory. [ ... ] > static void virtio_usb_dc_event_process(struct virtio_usb_event *event) > { [ ... ] > + if (vudc->veps) { > + unsigned int i; > + > + for (i = 0; i < vudc->neps; i++) > + cancel_work_sync(&vudc->veps[i].enable_work); > + kfree(vudc->veps); > + vudc->veps = NULL; > + } (For context with the dangling pointer concern in vep_disable_work) This is where vudc->veps is freed during VIRTIO_USB_EVT_DEV_UNBIND without cancelling disable_work or flushing ep_cmd_wq. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
