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 <[email protected]> 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/[email protected]?part=8
