Re: [PATCH 05/11] usb: misc: Add NVIDIA LSTP USB adapter driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] driver-core: devres leak and broken unbind fence due to early action registration
- [High] module: use-after-free race in lstp_live_objects tracking during module unload
--

commit 1d5a9ec5ec6f0f561150a37e199ecaed8f91799d
Author: Adrian Ambrozewicz <aambrozewicz@nvidia.com>

usb: misc: Add NVIDIA LSTP USB adapter driver

The patch introduces the NVIDIA Low-Speed Transport Protocol (LSTP) USB
adapter driver. It multiplexes various channel types (SPI, I2C, GPIO,
UART, IPMI, MMIO) over a single USB bulk endpoint pair. The core driver
discovers channels and registers them as auxiliary devices for subsystem
drivers to bind against.

> diff --git a/drivers/usb/misc/lstp.c b/drivers/usb/misc/lstp.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/usb/misc/lstp.c
[ ... ]
> +static void lstp_kobj_release(struct kobject *kobj)
> +{
> +	kfree(container_of(kobj, struct lstp_kobj, kobj));
> +	if (atomic_dec_and_test(&lstp_live_objects))
> +		wake_up_var(&lstp_live_objects);

[Severity: High]
Does this code have a use-after-free race against module unload?

If this atomic decrement drops the counter to 0, the waiting thread in
lstp_module_exit() could unblock and complete the module unload, which unmaps
the module's text and data sections. 

If that happens before wake_up_var() executes, wake_up_var() will be called
from unmapped memory. Could we use a completion struct instead of atomics to
safely wait for module unload?

> +}
[ ... ]
> +static void lstp_channel_free(struct lstp_channel_priv *priv)
> +{
> +	/* Here, not at bus removal, so a live device's id cannot be recycled. */
> +	if (priv->auxdev_state != LSTP_AUXDEV_RAW)
> +		ida_free(&lstp_auxdev_ida, priv->auxdev.id);
> +	mutex_destroy(&priv->tx_mutex);
> +	mutex_destroy(&priv->cfg_lock);
> +	fwnode_handle_put(priv->pub.fwnode);
> +	kfree(priv->irq_buf);
> +	kfree(priv->auxdev.dev.platform_data);
> +	kfree(priv);
> +	if (atomic_dec_and_test(&lstp_live_objects))
> +		wake_up_var(&lstp_live_objects);

[Severity: High]
As with lstp_kobj_release() above, does this create a use-after-free race
against lstp_module_exit() unloading the module text and data?

> +}
[ ... ]
> +static int lstp_aux_bus_notify(struct notifier_block *nb, unsigned long action, void *data)
> +{
> +	struct lstp_usb *dev = container_of(nb, struct lstp_usb, aux_nb);
> +	struct device *cdev = data;
> +	struct lstp_channel_priv *priv;
> +	const struct lstp_ops *ops;
> +
> +	if (cdev->release != lstp_channel_dev_release)
> +		return NOTIFY_DONE;
> +	priv = to_lstp_priv_from_auxdev(to_auxiliary_dev(cdev));
> +	if (priv->usb != dev)
> +		return NOTIFY_DONE;
> +
> +	switch (action) {
> +	case BUS_NOTIFY_BIND_DRIVER:
> +		/*
> +		 * Before .probe(): book the re-fence. Devres is LIFO, so it
> +		 * fires after everything the driver goes on to register.
> +		 */
> +		if (devm_add_action(cdev, lstp_channel_quiesce_unbound, priv)) {

[Severity: High]
Can this cause a devres memory leak and a broken unbind fence?

The Linux driver core issues BUS_NOTIFY_BIND_DRIVER before calling
devres_open_group() to create the driver-specific devres group. This means the
devm_add_action() call here attaches the action to the device's global devres
list instead of the driver's group.

When the driver is unbound, devres_release_group() will only clean up
resources allocated within the group. The lstp_channel_quiesce_unbound()
action will never execute on unbind, leaking a struct action_devres on every
bind/unbind cycle.

Crucially, the unbound I/O fence is never raised, and priv->bulk_tx_urb is
never killed. A subsystem that retains a pinned channel could then continue
issuing hardware I/O and submitting URBs after its driver has been unbound,
breaking the intended isolation and lifecycle guarantees.

> +			/* lstp_aux_probe() fails the bind on the raised fence. */
> +			lstp_channel_mark_unbound(&priv->pub);
> +			dev_warn(cdev, "%s: ch_%d: Could not book the unbind fence\n", __func__,
> +				 priv->pub.ch_id);
> +			break;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-lstp-usb-adapter-v1-0-aambrozewicz@nvidia.com?part=5