Re: [PATCH v2 02/22] usb: typec: Represent USB4 on the Type-C bus

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] TOCTOU race introduced by double-read of `altmode->partner` in `typec_altmode_notify()`.

Pre-existing issues:
- [High] Missing NULL check on `partner` leads to a NULL pointer dereference in `typec_altmode_enter()` and `typec_altmode_exit()`.
- [High] Lockless double-read of `adev->dev.driver` in `typec_altmode_update_active()` can race with driver unbinding, causing a NULL pointer dereference.
--

commit 78f546f39cb57c2d6df37d112b4c42115768555d
Author: Sven Peter <sven@kernel.org>

usb: typec: Represent USB4 on the Type-C bus

USB4 technically isn't alternate mode and entered using the EUDO and has
no SVID or Discover Modes Response. We can still represent it like an
altmode on the Type-C bus though by adding a mode kind. Exclude the USB4
modes from any SVID-specific operations and driver binding, but allow
consumers to observe their state through notifications.

> diff --git a/drivers/usb/typec/bus.c b/drivers/usb/typec/bus.c
> index e84b134a33813..8e6d7769fac3f 100644
> --- a/drivers/usb/typec/bus.c
> +++ b/drivers/usb/typec/bus.c
> @@ -93,6 +93,8 @@ int typec_altmode_notify(struct typec_altmode *adev,
>  
>  	if (!adev)
>  		return 0;
> +	if (adev->mode_kind != TYPEC_MODE_KIND_ALTMODE)
> +		return -EOPNOTSUPP;
>  
>  	altmode = to_altmode(adev);
>  

[Severity: High]
Does this code introduce a regression with a TOCTOU race from a double-read of
altmode->partner?

Looking at typec_altmode_notify() in drivers/usb/typec/bus.c:

	if (!altmode->partner)
		return -ENODEV;

	is_port = is_typec_port(adev->dev.parent);
	partner = altmode->partner;

	ret = typec_altmode_set_switches(is_port ? altmode : partner, conf, data);

If typec_altmode_put_partner() sets altmode->partner to NULL concurrently
without holding a lock that serializes against typec_altmode_notify()
(for example, during a device unplug), could a race between these two lines
allow the second read of altmode->partner to be NULL? This could
unconditionally dereference partner->adev.ops later in the function.

[ ... ]
> @@ -125,12 +127,19 @@ EXPORT_SYMBOL_GPL(typec_altmode_notify);
>   */
>  int typec_altmode_enter(struct typec_altmode *adev, u32 *vdo)
>  {
> -	struct altmode *partner = to_altmode(adev)->partner;
> -	struct typec_altmode *pdev = &partner->adev;
> +	struct typec_altmode *pdev;
> +	struct altmode *partner;
>  	int ret;
>  
> -	if (!adev || adev->active)
> +	if (!adev)
>  		return 0;
> +	if (adev->mode_kind != TYPEC_MODE_KIND_ALTMODE)
> +		return -EOPNOTSUPP;
> +	if (adev->active)
> +		return 0;
> +
> +	partner = to_altmode(adev)->partner;
> +	pdev = &partner->adev;
>  
>  	if (!pdev->ops || !pdev->ops->enter)
>  		return -EOPNOTSUPP;

[Severity: High]
This is a pre-existing issue, but can this lead to a NULL pointer dereference
if partner is NULL?

If a partner altmode is registered but there is no matching port altmode
(for example, plugging a Thunderbolt device into a non-TBT Type-C port),
typec_altmode_set_partner() leaves altmode->partner as NULL.

If the Thunderbolt driver binds to this partner altmode and calls
typec_altmode_enter() from its workqueue, it would unconditionally compute
&partner->adev from a NULL partner pointer, and then dereference the invalid
pointer at pdev->ops.

[ ... ]
> @@ -156,13 +165,20 @@ EXPORT_SYMBOL_GPL(typec_altmode_enter);
>   */
>  int typec_altmode_exit(struct typec_altmode *adev)
>  {
> -	struct altmode *partner = to_altmode(adev)->partner;
> -	struct typec_altmode *pdev = &partner->adev;
> +	struct typec_altmode *pdev;
> +	struct altmode *partner;
>  	int ret;
>  
> -	if (!adev || !adev->active)
> +	if (!adev)
> +		return 0;
> +	if (adev->mode_kind != TYPEC_MODE_KIND_ALTMODE)
> +		return -EOPNOTSUPP;
> +	if (!adev->active)
>  		return 0;
>  
> +	partner = to_altmode(adev)->partner;
> +	pdev = &partner->adev;
> +
>  	if (!pdev->ops || !pdev->ops->exit)
>  		return -EOPNOTSUPP;

[Severity: High]
This is a pre-existing issue, but similar to typec_altmode_enter(), can this
cause a crash during altmode exit?

If partner is NULL, pdev = &partner->adev creates an invalid pointer that is
subsequently dereferenced at pdev->ops.

[ ... ]
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 5808a3ef0d6cd..869d6857c0dca 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -296,6 +297,8 @@ static void typec_altmode_put_partner(struct altmode *altmode)
>   *
>   * If a partner or cable plug executes Enter/Exit Mode command successfully, the
>   * drivers use this routine to report the updated state of the mode.
> + * For USB4 partner modes, drivers use this routine to report entry through
> + * Enter_USB and exit from USB4.
>   */
>  void typec_altmode_update_active(struct typec_altmode *adev, bool active)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but does the lockless double-read
of adev->dev.driver in typec_altmode_update_active() leave a race condition?

Earlier in this function, the code evaluates adev->dev.driver:

	if (!is_typec_port(adev->dev.parent) && adev->dev.driver) {
		if (!active)
			module_put(adev->dev.driver->owner);
		else
			WARN_ON(!try_module_get(adev->dev.driver->owner));
	}

Because this block only holds alt->state_lock and concurrent driver unbinding
sets dev->driver = NULL under device_lock(), without READ_ONCE() or proper
locking, the compiler can issue two reads for adev->dev.driver. 

If unbinding occurs between the check and accessing adev->dev.driver->owner,
can the second read evaluate to NULL and lead to a crash?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906-b4-apple-soc-tbt-v2-0-1f80085f93fb@kernel.org?part=2