Re: [PATCH] usb: misc: onboard_usb_dev: don't defer the USB device probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: misc: onboard_usb_dev: UAF race in onboard_dev_attach_usbdev()
--

commit f6a7eed8d40c04892ba2d262827780a7d1796445
Author: James Clark <jjc@jclark.com>

usb: misc: onboard_usb_dev: don't defer the USB device probe

This commit avoids returning -EPROBE_DEFER in onboard_dev_usbdev_probe() when
the corresponding platform device has not finished probing, preventing an
infinite re-probe loop caused by generic USB interface driver bindings.
Instead, it binds the USB device with NULL drvdata and extends
attach_usb_driver_work to attach such USB devices once the platform device
probes.

> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
> index c604062404162..aac4e29c82335 100644
> --- a/drivers/usb/misc/onboard_usb_dev.c
> +++ b/drivers/usb/misc/onboard_usb_dev.c
[ ... ]
> @@ -301,6 +302,47 @@ static const struct attribute_group onboard_dev_group = {
>  __ATTRIBUTE_GROUPS(onboard_dev);
>  
>  
> +/*
> + * Attach a USB device that was bound by onboard_dev_usbdev_probe() while its
> + * platform device was still unbound. The device lock serialises this with
> + * the probe and with an unbind. This walks the USB bus, not the driver's
> + * device list: unbinding a device holds its lock while it waits for all
> + * references to its node on the driver's list to be dropped, so an iterator
> + * over that list that takes the device lock would deadlock with it.
> + */
> +static int onboard_dev_attach_usbdev(struct usb_device *udev, void *data)
> +{
> +	struct device *dev = &udev->dev;
> +	struct onboard_dev *onboard_dev;
> +	int err;
> +
> +	/* Unlocked pre-check, so as not to wait for every USB device's lock. */
> +	if (READ_ONCE(dev->driver) != &onboard_dev_usbdev_driver.driver)
> +		return 0;
> +
> +	device_lock(dev);
> +
> +	if (dev->driver != &onboard_dev_usbdev_driver.driver ||
> +	    dev_get_drvdata(dev))
> +		goto out;
> +
> +	onboard_dev = _find_onboard_dev(dev);
> +	if (IS_ERR_OR_NULL(onboard_dev))
> +		goto out;
> +
> +	err = onboard_dev_add_usbdev(onboard_dev, udev);
> +	if (err)
> +		dev_err(dev, "failed to attach to %s: %pe\n",
> +			dev_name(onboard_dev->dev), ERR_PTR(err));
> +	else
> +		dev_set_drvdata(dev, onboard_dev);

[Severity: High]
Can onboard_dev_attach_usbdev() race with platform device unbind and cause a
use-after-free of struct onboard_dev (and potentially struct platform_device)?

In _find_onboard_dev(), the platform device is looked up, its drvdata is read
without holding device_lock(&pdev->dev), and the reference on &pdev->dev is
immediately dropped before returning the devm-allocated onboard_dev pointer:

drivers/usb/misc/onboard_usb_dev.c:_find_onboard_dev() {
	...
	onboard_dev = dev_get_drvdata(&pdev->dev);
	put_device(&pdev->dev);

	return onboard_dev;
}

When the platform device is unbound (for example via sysfs unbind, parent hub
disconnect calling onboard_dev_destroy_pdevs(), or
CONFIG_DEBUG_TEST_DRIVER_REMOVE), __device_release_driver(&pdev->dev) calls
onboard_dev_remove() followed by device_unbind_cleanup():

drivers/usb/misc/onboard_usb_dev.c:onboard_dev_remove() {
	struct onboard_dev *onboard_dev = dev_get_drvdata(&pdev->dev);
	...
	onboard_dev->going_away = true;

	mutex_lock(&onboard_dev->lock);

	/* unbind the USB devices to avoid dangling references to this device */
	while (!list_empty(&onboard_dev->udev_list)) {
		...
	}
}

Because a USB device bound with NULL drvdata is not yet in
onboard_dev->udev_list, onboard_dev_remove() does not unbind it or take
device_lock(&udev->dev), nor does it cancel attach_usb_driver_work or clear
the platform device's drvdata. In device_unbind_cleanup(),
devres_release_all() frees the devm-allocated struct onboard_dev before
dev_set_drvdata(&pdev->dev, NULL) is called.

If onboard_dev_attach_usbdev() (or onboard_dev_usbdev_probe()) runs
concurrently with platform device unbind, _find_onboard_dev() can return the
non-NULL onboard_dev pointer just before devres_release_all() frees it,
allowing onboard_dev_add_usbdev() to lock and access freed memory.

Furthermore, even if onboard_dev_add_usbdev() sees onboard_dev->going_away set
under onboard_dev->lock and returns -EINVAL before devres_release_all() runs,
the error path in onboard_dev_attach_usbdev() then evaluates
dev_name(onboard_dev->dev) outside onboard_dev->lock, dereferencing
onboard_dev->dev after onboard_dev (and potentially pdev, since put_device()
was already called) has been freed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030114705.3159093-1-jjc@jclark.com?part=1