Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] usb: virtio_usb Kconfig allows built-in driver with modular gadget subsystem - [Medium] virtio-usb: 16-bit port ID in device UAPI structures truncates 32-bit index - [Medium] virtio-usb: missing bounds check on untrusted nports causes huge allocation - [Low] virtio-usb: redundant USB prefix in UAPI SuperSpeed enum constant - [Low] virtio-usb: missing error check after virtio_find_vqs() copies stale pointer --
commit 326b99edf62be9a25cd868d49b2b33423f8607c9 Author: Aiswarya Cyriac <[email protected]> virtio-usb: add protocol header and skeleton dual-role driver Add the virtio_usb device and virtqueue-wrapper skeleton, the protocol header shared with the vhost gadget driver and userspace backend, and probe()/remove() plumbing with no role support yet. > diff --git a/drivers/usb/virtio_usb/Kconfig b/drivers/usb/virtio_usb/Kconfig > new file mode 100644 > index 0000000000000..bd377b9cbb43a > --- /dev/null > +++ b/drivers/usb/virtio_usb/Kconfig > @@ -0,0 +1,21 @@ > +config USB_VIRTIO > + tristate "Virtio USB device support" > + depends on USB_SUPPORT && VIRTIO > + depends on USB || USB_GADGET > + depends on USB if !USB_GADGET > + depends on USB_GADGET if !USB [Severity: High] Does this logic allow CONFIG_USB_VIRTIO to be built-in while the gadget subsystem is modular? If CONFIG_USB=y and CONFIG_USB_GADGET=m, it looks like 'depends on USB if !USB_GADGET' evaluates to ignored, permitting CONFIG_USB_VIRTIO=y. Because device.o is unconditionally compiled into the driver, could this cause undefined references to modular gadget APIs from built-in code, breaking the vmlinux link? [ ... ] > diff --git a/drivers/usb/virtio_usb/controller.c > b/drivers/usb/virtio_usb/controller.c > new file mode 100644 > index 0000000000000..2fc6f502b48f3 > --- /dev/null > +++ b/drivers/usb/virtio_usb/controller.c [ ... ] > +static int virtio_usb_find_vqs(struct virtio_usb *vusb) > +{ > + unsigned int nvqs = vusb->nvqs, i; > + struct virtqueue **vqs = NULL; > + int rc = -ENOMEM; > + > + vqs = kcalloc(nvqs, sizeof(void *), GFP_KERNEL); > + if (!vqs) > + return -ENOMEM; > + > + struct virtqueue_info *vqs_info = > + kcalloc(nvqs, sizeof(*vqs_info), GFP_KERNEL); > + if (!vqs_info) { > + rc = -ENOMEM; > + goto on_exit; > + } > + for (i = 0; i < nvqs; i++) { > + vqs_info[i].name = vusb->vqueues[i].name; > + vqs_info[i].callback = vusb->vqueues[i].callback; > + } > + rc = virtio_find_vqs(vusb->vdev, nvqs, vqs, vqs_info, NULL); > + > + kfree(vqs_info); > + > + for (i = 0; i < nvqs; i++) { > + vusb->vqueues[i].vqueue = vqs[i]; > + spin_lock_init(&vusb->vqueues[i].lock); > + } [Severity: Low] Is there a missing error check for 'rc' here? If virtio_find_vqs() fails and returns an error code, the code seems to fall through to the loop and blindly assign vusb->vqueues[i].vqueue = vqs[i] using uninitialized or dangling pointers from the vqs array before exiting via 'on_exit'. Should there be an 'if (rc) goto on_exit;' before the loop? > +on_exit: > + kfree(vqs); > + return rc; > +} [ ... ] > +static int virtio_usb_probe(struct virtio_device *vdev) > +{ > + struct virtio_usb *vusb; > + unsigned int nvqs = 0; > + int rc = 0; > + > + vusb = devm_kzalloc(&vdev->dev, sizeof(*vusb), GFP_KERNEL); > + if (!vusb) > + return -ENOMEM; > + > + vusb->vdev = vdev; > + vdev->priv = vusb; > + > + virtio_cread_le(vdev, struct virtio_usb_config, ports, &vusb->nports); > + > + vusb->vports = devm_kcalloc(&vdev->dev, vusb->nports, > + sizeof(*vusb->vports), GFP_KERNEL); [Severity: Medium] Does this allocation need a bounds check? The untrusted value 'vusb->nports' is read directly from the device config space and passed to devm_kcalloc() with GFP_KERNEL. Since there is no architectural cap checked, could a malicious host or hypervisor supply a massive value (e.g., 100M) and trigger OOM killer activity or large allocation warnings? [ ... ] > diff --git a/include/uapi/linux/virtio_usb.h b/include/uapi/linux/virtio_usb.h > new file mode 100644 > index 0000000000000..b9dc448e38ef6 > --- /dev/null > +++ b/include/uapi/linux/virtio_usb.h [ ... ] > +enum { > + VIRTIO_USB_SPEED_UNKNOWN = 0, > + VIRTIO_USB_SPEED_LOW, > + VIRTIO_USB_SPEED_FULL, /* usb 1.1 */ > + VIRTIO_USB_SPEED_HIGH, /* usb 2.0 */ > + VIRTIO_USB_SPEED_WIRELESS, /* wireless (usb 2.5) */ > + VIRTIO_USB_USB_SPEED_SUPER, /* usb 3.0 */ [Severity: Low] Is the "USB" prefix redundant here? This defines VIRTIO_USB_USB_SPEED_SUPER instead of VIRTIO_USB_SPEED_SUPER, breaking naming consistency and forcing userspace to use the flawed macro name. > + VIRTIO_USB_SPEED_SUPER_PLUS, /* usb 3.1 */ > +}; [ ... ] > +struct virtio_usb_dev_cmd_hdr { > + __le32 code; /* VIRTIO_USB_CMD_DEV_XXX */ > + __le16 port; /* Port ID */ > + __le16 endpoint; /* Endpoint ID */ > +}; [Severity: Medium] Will this 16-bit port ID correctly route commands for all virtual ports? Since 'virtio_usb_config.ports' and host UAPI structures define the port count and IDs as 32-bit (__le32), a 16-bit port field here silently truncates the index for any port >= 65536. [ ... ] > +struct virtio_usb_request { > + __le64 tag; > + __le16 port; /* Port ID */ > + __le16 endpoint; /* Endpoint ID */ > + __le16 transfer_type; /* VIRTIO_USB_EP_XXX */ > + __le16 transfer_flags; /* VIRTIO_USB_FLAG_XXX */ [Severity: Medium] Similar to 'virtio_usb_dev_cmd_hdr' above, does this 16-bit port ID truncate the 32-bit port index? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
