Re: [PATCH 02/19] usb: typec: Add thunderbolt switch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Failure of `try_module_get()` is ignored, allowing the consumer to obtain a switch linked to a module that is being unloaded.
- [High] Unprotected access to `parent->driver` can cause a fatal NULL pointer dereference if the parent device is unbound.
- [High] The `typec_thunderbolt_switch_set()` callback can be invoked after the provider driver has unbound, leading to a Use-After-Free of provider private data.
--

commit 96a5ceca9e0641da0cd0d5f342a8b7d8ed98e9af
Author: Sven Peter <sven@kernel.org>

usb: typec: Add thunderbolt switch

On Apple Silicon the USB4/Thunderbolt host router can only be brought
up after the Type-C PHY has been configured and requires cable details
known only to the Type-C port controller.

> diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
> index 9b908c46bd7df..8339846ba355e 100644
> --- a/drivers/usb/typec/mux.c
> +++ b/drivers/usb/typec/mux.c

[ ... ]

> +struct typec_thunderbolt_switch *
> +fwnode_typec_thunderbolt_switch_get(struct fwnode_handle *fwnode)
> +{

[ ... ]

> +	sw = kzalloc_obj(*sw);
> +	if (!sw) {
> +		put_device(&sw_dev->dev);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	sw->sw_dev = sw_dev;
> +	WARN_ON(!try_module_get(sw_dev->dev.parent->driver->owner));

[Severity: High]
Does this code handle the case where try_module_get() fails?

If fwnode_typec_thunderbolt_switch_get() is called while the provider module
is unloading, try_module_get() will return 0. The code triggers a warning but
continues execution, returning a switch backed by a dying module instead of
returning an error like ERR_PTR(-ENODEV).

When the consumer later calls typec_thunderbolt_switch_set(), it could execute
freed module memory. Furthermore, typec_thunderbolt_switch_put() will
unconditionally call module_put(), dropping a reference that was never
acquired.

[Severity: High]
Can sw_dev->dev.parent->driver be NULL here?

If the provider device is unbound from its driver concurrently (for example,
via sysfs unbind), the driver core clears parent->driver to NULL.
Accessing driver->owner unconditionally could result in a fatal NULL pointer
dereference.

> +
> +	return sw;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_typec_thunderbolt_switch_get);
> +
> +/**
> + * typec_thunderbolt_switch_put - Release handle to USB4/Thunderbolt switch
> + * @sw: USB4/Thunderbolt switch
> + *
> + * Decrements the reference count of @sw and releases the handle.
> + */
> +void typec_thunderbolt_switch_put(struct typec_thunderbolt_switch *sw)
> +{
> +	if (IS_ERR_OR_NULL(sw))
> +		return;
> +
> +	module_put(sw->sw_dev->dev.parent->driver->owner);

[Severity: High]
Could parent->driver be NULL when this is called during teardown?

If the provider driver has already unbound, parent->driver will have been
cleared by the driver core. Accessing it unconditionally here to get the owner
could cause a NULL pointer dereference.

> +	put_device(&sw->sw_dev->dev);
> +	kfree(sw);
> +}
> +EXPORT_SYMBOL_GPL(typec_thunderbolt_switch_put);
> +
> +/**
> + * typec_thunderbolt_switch_set - Forward cable details to USB4/Thunderbolt switch
> + * @sw: USB4/Thunderbolt switch
> + * @data: Cable state and details
> + *
> + * Called by the Type-C port driver to forward the cable details
> + * out-of-band to the switch handler whenever a USB4/Thunderbolt
> + * connection comes up or goes away.
> + */
> +int typec_thunderbolt_switch_set(struct typec_thunderbolt_switch *sw,
> +				 const struct typec_thunderbolt_switch_data *data)
> +{
> +	if (IS_ERR_OR_NULL(sw))
> +		return 0;
> +
> +	return sw->sw_dev->set(sw->sw_dev, data);

[Severity: High]
Is it possible for the set() callback to be invoked after the provider driver
has unbound?

If a physical Type-C plug event occurs concurrently with a privileged sysfs
unbind, the consumer driver might call typec_thunderbolt_switch_set(). This
would blindly invoke the callback, potentially accessing driver-private data
that was already freed during unbind.

> +}
> +EXPORT_SYMBOL_GPL(typec_thunderbolt_switch_set);

[ ... ]

> +/**
> + * typec_thunderbolt_switch_unregister - Unregister USB4/Thunderbolt switch
> + * @sw_dev: USB4/Thunderbolt switch
> + *
> + * Unregister switch that was registered with
> + * typec_thunderbolt_switch_register().
> + */
> +void typec_thunderbolt_switch_unregister(struct typec_thunderbolt_switch_dev *sw_dev)
> +{
> +	if (!IS_ERR_OR_NULL(sw_dev))
> +		device_unregister(&sw_dev->dev);

[Severity: High]
Should unregistration clear the set callback or synchronize with consumers?

Since unregistration does not clear the callback or wait for consumers to
release it, the provider driver's remove callback will proceed to free its
private data, leading to the use-after-free mentioned above if consumers
are still actively invoking sw_dev->set().

> +}
> +EXPORT_SYMBOL_GPL(typec_thunderbolt_switch_unregister);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830-b4-apple-soc-tbt-v1-0-44bc9348683c@kernel.org?part=2