Re: [PATCH 8/8] virtio-usb: support a guest UDC name prefix from the bind event

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: virtio_usb: missing sanitization of host-provided UDC name
- [Low] usb: virtio_usb: stale parent device name retained on BIND retry
--

commit b4fd3e9cac2d1567822e6a554abe7a7e1afb42b9
Author: Igor Skalkin <igor.skalkin@oss.qualcomm.com>

virtio-usb: support a guest UDC name prefix from the bind event

This commit allows the host backend to supply a name prefix for a port's UDC
parent platform_device via the bind event. If the prefix is present, it is
used as the parent device's name and to construct the gadget name; otherwise,
it falls back to the generic GADGET_NAME.

> diff --git a/drivers/usb/virtio_usb/device.c b/drivers/usb/virtio_usb/device.c
> index ee41ea9d164f1..fedce742f9781 100644
> --- a/drivers/usb/virtio_usb/device.c
> +++ b/drivers/usb/virtio_usb/device.c
> @@ -1369,11 +1369,26 @@ static int virtio_usb_dc_get_endpoint_info(struct virtio_usb_dc *vudc)
>  	return rc;
>  }
>  
> -static int virtio_usb_dc_parent_create(struct virtio_usb_dc *vudc)
> +static int virtio_usb_dc_parent_create(struct virtio_usb_dc *vudc,
> +				       const unsigned char *udc_prefix)
>  {
> +	size_t prefix_len;
> +	char *parent_name;
>  	int rc;
>  
> -	vudc->pdev = platform_device_alloc(GADGET_NAME, vudc->port);
> +	prefix_len = strnlen((const char *)udc_prefix,
> +			     VIRTIO_USB_UDC_NAME_PREFIX_MAX);
> +	if (prefix_len) {
> +		parent_name = kasprintf(GFP_KERNEL, "%.*s", (int)prefix_len,
> +					(const char *)udc_prefix);

[Severity: High]
Does this code adequately sanitize the host-provided udc_prefix string?

If a malicious host supplies a string with invalid characters (such as '/'),
passing this unsanitized string directly into kasprintf to construct the parent
platform device name could trigger a kernel WARN in kobject_add(), which would
panic the guest if panic_on_warn is enabled.

> +	} else {
> +		parent_name = kstrdup(GADGET_NAME, GFP_KERNEL);
> +	}
> +	if (!parent_name)
> +		return -ENOMEM;
> +
> +	vudc->pdev = platform_device_alloc(parent_name, vudc->port);
> +	kfree(parent_name);
>  	if (!vudc->pdev)
>  		return -ENOMEM;

[ ... ]

> @@ -1447,7 +1463,8 @@ static void virtio_usb_dc_event_process(struct virtio_usb_event *event)
>  			break;
>  		}
>  		if (!vudc->pdev) {
> -			rc = virtio_usb_dc_parent_create(vudc);
> +			rc = virtio_usb_dc_parent_create(
> +				vudc, bind_evt->udc_name_prefix);

[Severity: Low]
If a previous BIND event failed later in this function, could vudc->pdev still
be populated here?

If it was left populated by a prior failed BIND, the new prefix is ignored,
and the parent platform device will retain a stale name from the previous BIND
event.

>  			if (rc) {
>  				dev_err(&vusb->vdev->dev,
>  					"Failed to create UDC parent device\n");
> @@ -1456,8 +1473,18 @@ static void virtio_usb_dc_event_process(struct virtio_usb_event *event)
>  				break;
>  			}
>  		}
> -		vudc->gadget.name =
> -			kasprintf(GFP_KERNEL, "%s_%d", GADGET_NAME, port_id);
> +		udc_prefix_len =
> +			strnlen((const char *)bind_evt->udc_name_prefix,
> +				VIRTIO_USB_UDC_NAME_PREFIX_MAX);
> +		if (udc_prefix_len) {
> +			vudc->gadget.name = kasprintf(
> +				GFP_KERNEL, "%.*s_p%d", (int)udc_prefix_len,
> +				(const char *)bind_evt->udc_name_prefix,
> +				port_id);

[Severity: High]
Are there any missing sanitization checks on the host-provided
bind_evt->udc_name_prefix here?

This unsanitized string is used to build the gadget name, which will also be
passed to device registration APIs. Similar to the parent_name, a malicious
host supplying invalid characters can trigger a kernel WARN or panic.

> +		} else {
> +			vudc->gadget.name = kasprintf(GFP_KERNEL, "%s_%d",
> +						      GADGET_NAME, port_id);
> +		}
>  		if (!vudc->gadget.name) {
>  			rc = -ENOMEM;
>  			kfree(vudc->veps);
> @@ -1480,7 +1507,6 @@ static void virtio_usb_dc_event_process(struct virtio_usb_event *event)
>  			break;
>  		}
>  
> -		//vudc->gadget.dev.init_name = gadget_name;
>  		vudc->gadget.dev.parent = &vudc->pdev->dev;
>  		rc = usb_add_gadget_udc(&vudc->pdev->dev, &vudc->gadget);
>  		if (rc) {
>  			dev_err(&vudc->pdev->dev, "Failed to add udc\n");
>  			kfree(vudc->veps);
>  			vudc->veps = NULL;
>  			kfree(vudc->gadget.name);
>  			break;
>  		}

[Severity: Low]
Does this error path fail to unregister vudc->pdev?

By not unregistering vudc->pdev, it is left for the next BIND event to wrongly
reuse. This creates a mismatch between the parent device name and the new
gadget name in sysfs on a BIND retry.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924160907.145405-1-igor.skalkin@oss.qualcomm.com?part=8