Re: [PATCH 1/8] virtio-usb: add protocol header and skeleton dual-role driver

[sashiko-bot@kernel.org]

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 <aiswarya.cyriac@oss.qualcomm.com>

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/20260924160907.145405-1-igor.skalkin@oss.qualcomm.com?part=1