Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider: - [Critical] virtio-usb: missing bounds check on actual_length enables OOB memory access - [High] virtio-usb: host dataq cancel_work_sync() on uninitialized work_struct - [High] virtio-usb: missing sg_mark_end() in virtio_usb_hc_set_urb_sgs() - [High] virtio-usb: invalid sg_virt() on Highmem pages crashes DMA mapping - [High] virtio-usb: virtqueue teardown before virtio_reset_device() causes UAF - [High] virtio-usb: virtio_usb_port_from_urb() misroutes URBs for devices behind hubs - [High] virtio-usb: virtio_usb_data_send() enables interrupts in atomic context - [High] virtio-usb: leaking vurb heap address as tag breaks KASLR and causes ABA - [High] virtio-usb: heap overflow in virtio_usb_hub_control() when wLength is small - [Medium] virtio-usb: missing NULL check on events array causes panic during init - [Medium] virtio-usb: virtio_usb_evt_drain_stop_cb() leaks pending buffers on teardown - [Medium] virtio-usb: bitfield usage in port status breaks big-endian hosts --
commit 89dd2308fe62a55b4cf51aa86800ab5c1ff53727 Author: Aiswarya Cyriac <[email protected]> virtio-usb: add host role (USB Host Controller) support Adds common virtqueue handling code and the virtio-usb host controller implementation. Host virtual ports are initialized with pre-allocated leaf slots, and role resolution is initially hardcoded to the host role. > diff --git a/drivers/usb/virtio_usb/controller.c > b/drivers/usb/virtio_usb/controller.c > index 2fc6f502b48f3..216edfcaa3c00 100644 > --- a/drivers/usb/virtio_usb/controller.c > +++ b/drivers/usb/virtio_usb/controller.c [ ... ] > +static int virtio_usb_probe(struct virtio_device *vdev) > +{ [ ... ] > + rc = virtio_usb_find_vqs(vusb); > + if (rc) { > + dev_err(&vdev->dev, "%s virtio_usb_find_vqs() error(%d)\n", > + __func__, rc); > + goto on_error; > + } > + > + if (vusb->host_role) { > + INIT_WORK(&vusb->vq_host_data_rx_work, virtio_usb_hc_rx_work); [Severity: High] Does this code call cancel_work_sync() on an uninitialized work_struct if probe fails? If virtio_usb_find_vqs() returns an error, the driver jumps to the error path and calls virtio_usb_remove(), which eventually invokes virtio_usb_hc_dataq_stop_cb(). That function unconditionally calls cancel_work_sync(&vusb->vq_host_data_rx_work). Since INIT_WORK hasn't been called yet, could this trigger a kernel panic or lockdep warning? [ ... ] > +static void virtio_usb_remove(struct virtio_device *vdev) > +{ > + struct virtio_usb *vusb = vdev->priv; > + int i; > + > + for (i = 0; i < vusb->nvqs; i++) > + vusb->vqueues[i].stop(vusb, &vusb->vqueues[i]); > + > + if (vusb->host_role && vusb->vports) { > + for (i = 0; i < (int)vusb->nports; i++) > + virtio_usb_hc_vp_deinit(vusb, i); > + } > + > + virtio_reset_device(vdev); [Severity: High] Can this order of operations cause a use-after-free? The stop() callbacks (like virtio_usb_hc_dataq_stop_cb()) unlink all pending URBs and free their associated memory while the virtio device is still active. Since virtio_reset_device() is called afterward, could the hardware concurrently complete a pending request and DMA-write the response into the just-freed memory? > diff --git a/drivers/usb/virtio_usb/host.c b/drivers/usb/virtio_usb/host.c > new file mode 100644 > index 0000000000000..9926e66bcc286 > --- /dev/null > +++ b/drivers/usb/virtio_usb/host.c [ ... ] > +static int virtio_usb_hub_control(struct usb_hcd *hcd, u16 type, u16 wValue, > + u16 wIndex, char *buffer, u16 wLength, > + bool ss_mode) > +{ > + struct virtio_usb_hc_port *port; > + struct virtio_usb_hc_vp *vhcd_vp = vhcd_get(hcd); > + > + switch (type) { > + case GetHubDescriptor: { > + struct usb_hub_descriptor *dsc = > + (struct usb_hub_descriptor *)buffer; > + > + memset(dsc, 0, sizeof(struct usb_hub_descriptor)); [Severity: High] Can this memset() overflow the provided buffer? The buffer size is determined by the wLength parameter. If userspace submits a control transfer via usbfs with a wLength smaller than sizeof(struct usb_hub_descriptor), will this unconditional memset() write past the allocated buffer bounds? [ ... ] > +static void virtio_usb_hc_complete_urb(struct virtio_usb_data *vurb) > +{ [ ... ] > + urb->actual_length = le32_to_cpu(response->actual_length); [Severity: Critical] Is it safe to trust the actual_length provided by the virtio device without bounds checking? If the untrusted response->actual_length exceeds urb->transfer_buffer_length, can subsequent USB class drivers use this value to read or write past the end of the transfer buffer? > + if (urb->status == -EINVAL) [ ... ] > + if (usb_pipeisoc(urb->pipe)) { > + iso_urb_status = (void *)response + sizeof(*response); > + > + for (i = 0; i < urb->number_of_packets; i++) { > + struct virtio_usb_iso_status *iso_status = > + &iso_urb_status[i]; > + status = le32_to_cpu(iso_status->status); > + > + urb->iso_frame_desc[i].actual_length = > + le32_to_cpu(iso_status->actual_length); [Severity: Critical] Does this code also need bounds checking against the expected isochronous frame lengths to prevent out-of-bounds memory accesses? [ ... ] > +static struct scatterlist * > +virtio_usb_hc_set_urb_sgs(struct virtio_usb_hc_priv *priv, gfp_t gfp) > +{ [ ... ] > + for_each_sg(urb->sg, sg, nsgs, i) { > + sg_length = sg->length; > + > + if (sg_length > buffer_length) > + sg_length = buffer_length; > + > + sg_set_page(&sgs[i], sg_page(sg), sg_length, > + sg->offset); > + > + buffer_length -= sg_length; > + if (!buffer_length) > + break; > + } [Severity: High] When breaking early from this loop, does the code miss marking the end of the new scatterlist? Without calling sg_mark_end(&sgs[i]), passing this array to virtqueue_add_sgs() could cause the iterator to walk past the valid elements into the zero-initialized remainder, leading to DMA mapping crashes. [ ... ] > +static struct virtio_usb_data * > +virtio_usb_hc_data_alloc(struct virtio_usb_hc_port *port, struct urb *urb, > + gfp_t gfp) > +{ [ ... ] > + request->tag = cpu_to_le64((uintptr_t)vurb); [Severity: High] Is it intended to expose a raw kernel heap address to the virtio backend? Leaking this pointer breaks KASLR. Additionally, in virtio_usb_hc_dequeue(), this same pointer is used as the cancellation tag. If the URB is concurrently completed and its memory freed and reallocated for a new URB, could the delayed cancel command erroneously cancel the new URB (the ABA problem)? [ ... ] > +static struct virtio_usb_hc_port *virtio_usb_port_from_urb(struct usb_hcd > *hcd, > + struct urb *urb) > +{ > + struct virtio_usb_hc_vp *vhcd_vp = vhcd_get(hcd); > + int slot = urb->dev->portnum - 1; > + > + if (slot < 0 || slot >= VIRTIO_USB_VP_MAX_PORTS) > + return NULL; > + return &vhcd_vp->ports[slot]; > +} [Severity: High] Does this logic assume that all URBs target a device directly connected to the root hub? If a guest enumerates a USB hub, urb->dev->portnum would correspond to the parent hub's port rather than the root hub. Could this misroute requests to the wrong port slot on the virtio backend? [ ... ] > +static int virtio_usb_hc_enqueue(struct usb_hcd *hcd, struct urb *urb, > + gfp_t mem_flags) > +{ [ ... ] > + if (urb->transfer_buffer) { > + sg_init_one(psg_data, urb->transfer_buffer, > + urb->transfer_buffer_length); > + } else if (urb->num_sgs > 1) { > + psg_data = virtio_usb_hc_set_urb_sgs(priv, mem_flags); > + if (!psg_data) { > + rc = -ENOMEM; > + goto on_exit; > + } > + } else if (urb->transfer_buffer_length && urb->sg) { > + sg_init_one(psg_data, sg_virt(urb->sg), > + urb->transfer_buffer_length); > + } else { > + psg_data = NULL; > + } [Severity: High] Is it safe to use sg_virt() on an arbitrary scatterlist element here? The USB core allows URB scatterlists to reference HIGHMEM pages, for which page_address() returns NULL. In that case, sg_virt() would calculate an invalid virtual address (NULL + offset), potentially crashing the system during DMA mapping. [ ... ] > +int virtio_usb_hc_event_populate(struct virtio_usb *vusb) > +{ [ ... ] > + struct virtio_usb_event *events; > + > + events = virtio_usb_events_alloc( > + vusb, evt_queue, sizeof(struct virtio_usb_host_port_event)); > + > + return virtio_usb_events_populate(events); > +} [Severity: Medium] Does this code dereference the events array without checking for NULL? If virtio_usb_events_alloc() fails to allocate memory and returns NULL, passing it directly to virtio_usb_events_populate() will cause a kernel panic when the array is dereferenced. > diff --git a/drivers/usb/virtio_usb/host.h b/drivers/usb/virtio_usb/host.h > new file mode 100644 > index 0000000000000..8c5a233417f82 > --- /dev/null > +++ b/drivers/usb/virtio_usb/host.h [ ... ] > +struct virtio_usb_port_hs_status { > + u16 connect : 1; > + u16 enable : 1; > + u16 suspend : 1; > + u16 over_current : 1; > + u16 reset : 1; > + u16 reserved0 : 3; > + u16 power : 1; > + u16 low_speed : 1; > + u16 high_speed : 1; > + u16 test_mode : 1; > + u16 indicator_control : 1; > + u16 reserved1 : 3; > +}; [Severity: Medium] Can this bitfield definition break hub emulation on big-endian architectures? Memory layout for C bitfields is implementation-defined and reversed on big-endian systems. Since the values are written directly to a u16 buffer in virtio_usb_hub_control(), could this cause the USB core to observe scrambled port status bits? > diff --git a/drivers/usb/virtio_usb/vq_common.c > b/drivers/usb/virtio_usb/vq_common.c > new file mode 100644 > index 0000000000000..baaf29db62f4d > --- /dev/null > +++ b/drivers/usb/virtio_usb/vq_common.c [ ... ] > +int virtio_usb_data_send(struct virtio_usb *vusb, struct virtio_usb_data > *data, > + struct scatterlist *out_sgs, > + struct scatterlist *in_sgs) > +{ [ ... ] > + spin_lock_irq(&queue->lock); > + rc = virtqueue_add_sgs(queue->vqueue, psgs, nouts, nins, data, > + GFP_ATOMIC); > + if (!rc) > + notify = virtqueue_kick_prepare(queue->vqueue); > + spin_unlock_irq(&queue->lock); [Severity: High] Does spin_unlock_irq() incorrectly re-enable hardware interrupts when called from atomic context? This function is invoked by virtio_usb_hc_enqueue() (the urb_enqueue HCD callback), which is often executed in atomic hardirq context when USB drivers submit URBs from their completion handlers. Unconditionally enabling interrupts here can lead to nested interrupts or stack overflows. [ ... ] > +void virtio_usb_evt_drain_stop_cb(struct virtio_usb_queue *vq, > + struct work_struct *work) > +{ [ ... ] > + spin_lock_irqsave(&vq->lock, flags); > + virtqueue_disable_cb(vq->vqueue); > + while ((buf = virtqueue_get_buf(vq->vqueue, &length))) > + ; > + spin_unlock_irqrestore(&vq->lock, flags); > +} [Severity: Medium] Should this loop use virtqueue_detach_unused_buf() instead of virtqueue_get_buf()? Because virtqueue_get_buf() only returns buffers that have been completed by the device, preallocated event buffers might remain stuck on the vring. When the virtio core destroys the queues, it expects all buffers to be properly detached, potentially leading to leaked descriptor slots and core warnings. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
