Re: [PATCH] phy: apple: atc: Fix typec switch/mux leak and UAF on unbind

Re: [PATCH] phy: apple: atc: Fix typec switch/mux leak and UAF on unbind

[Vladimir Oltean]

Sashiko says:

commit 13bcb3149b5056d5815f8cd52f9e018076602989
Author: David Carlier <devnexen@gmail.com>
phy: apple: atc: Fix typec switch/mux leak and UAF on unbind

This commit addresses a typec switch and mux leak during unbind by
saving the registered devices and unregistering them via
devm_add_action_or_reset. It also drops unused struct fields.

> diff --git a/drivers/phy/apple/atc.c b/drivers/phy/apple/atc.c
> index e9d106f135c51..4156fabad742c 100644
> --- a/drivers/phy/apple/atc.c
> +++ b/drivers/phy/apple/atc.c
[ ... ]
> @@ -2066,15 +2063,25 @@ static int atcphy_sw_set(struct typec_switch_dev *sw, enum typec_orientation ori
>  	return 0;
>  }
>
> +static void atcphy_typec_switch_unregister(void *data)
> +{
> +	typec_switch_unregister(data);
> +}
> +
>  static int atcphy_probe_switch(struct apple_atcphy *atcphy)
>  {
> +	struct typec_switch_dev *sw;
>  	struct typec_switch_desc sw_desc = {
>  		.drvdata = atcphy,
>  		.fwnode = atcphy->dev->fwnode,
>  		.set = atcphy_sw_set,
>  	};
>
> -	return PTR_ERR_OR_ZERO(typec_switch_register(atcphy->dev, &sw_desc));
> +	sw = typec_switch_register(atcphy->dev, &sw_desc);
> +	if (IS_ERR(sw))
> +		return PTR_ERR(sw);
> +
> +	return devm_add_action_or_reset(atcphy->dev, atcphy_typec_switch_unregister, sw);
>  }

Could this lead to a use-after-free if consumers still hold a reference to
the switch or mux?

When the driver unbinds, devm cleanup runs and frees the apple_atcphy
structure. The devm action calls typec_switch_unregister(), which drops the
sysfs representation but doesn't free the device object if a consumer (like
TCPM or a Type-C port driver) still holds a reference.

If the consumer is not unbound concurrently, it retains the reference and
remains active. Unregistering the switch does not clear its .set callback.
If a cable replug or orientation change occurs, the consumer could call
typec_switch_set(), which would invoke atcphy_sw_set().

Since atcphy_sw_set() extracts the freed apple_atcphy pointer from drvdata
and attempts to acquire its mutex, could this result in a use-after-free?
[ ... ]

> @@ -2146,15 +2153,25 @@ static int atcphy_mux_set(struct typec_mux_dev *mux, struct typec_mux_state *sta
>  	return atcphy_configure(atcphy, target_mode);
>  }
>
> +static void atcphy_typec_mux_unregister(void *data)
> +{
> +	typec_mux_unregister(data);
> +}
> +
>  static int atcphy_probe_mux(struct apple_atcphy *atcphy)
>  {
> +	struct typec_mux_dev *mux;
>  	struct typec_mux_desc mux_desc = {
>  		.drvdata = atcphy,
>  		.fwnode = atcphy->dev->fwnode,
>  		.set = atcphy_mux_set,
>  	};
>
> -	return PTR_ERR_OR_ZERO(typec_mux_register(atcphy->dev, &mux_desc));
> +	mux = typec_mux_register(atcphy->dev, &mux_desc);
> +	if (IS_ERR(mux))
> +		return PTR_ERR(mux);
> +
> +	return devm_add_action_or_reset(atcphy->dev, atcphy_typec_mux_unregister, mux);
>  }

Are we exposed to the same issue here with typec_mux_unregister()?
If a consumer still holds a reference to the mux device after the driver
unbinds and devm frees apple_atcphy, could an alt-mode transition invoke
atcphy_mux_set() and access the freed memory?

[human]

Basically it is stating that consumers will continue to hold a reference
to the Type-C mux and switch even if you go to the extent of unregistering
them from the framework. This is a known problem in many subsystems;
even the PHY framework suffers from it.
https://lore.kernel.org/linux-phy/aZejMSJ9qqRWb2pX@google.com/

I don't know how the Type-C framework deals with this, so maybe you can
clarify in your commit message what kind of problems the deregistration
will get rid of, and what kind of problems it won't.

How do the cable replug or alt mode changes trigger calls to
atcphy_sw_set() and atcphy_mux_set()? A call stack would help.

Ideally you want to help the reviewer understand that the change is
obviously correct and takes into consideration all relevant factors.