phy: apple: atc: Fix typec switch/mux leak and UAF on unbind

PATCH Superseded David Carlier 2 participants 2 messages Apr 20 – May 7

Subsystems (2)

Generic PHY framework Arm/Apple machine support

lore

Share & tools

lore

David Carlier

author

2026-04-20 23:10 UTC #0 lore raw

PATCH 1/1 phy: apple: atc: Fix typec switch/mux leak and UAF on unbind

atcphy_probe_switch() and atcphy_probe_mux() discard the pointers returned by typec_switch_register() and typec_mux_register(). The platform driver has no .remove callback, so the registered switch and mux devices outlive the apple_atcphy struct, which is released by devm cleanup on driver unbind.

A subsequent typec event (cable orientation change, alt-mode transition) then invokes the registered atcphy_sw_set() or atcphy_mux_set() callback, which retrieves the freed apple_atcphy and dereferences it. Unbind followed by a cable replug or alt-mode change is enough to trigger the use-after-free.

Save the registered switch and mux and unregister them through devm_add_action_or_reset() so the framework references disappear in step with the driver's devm-allocated state. Drop the unused struct apple_atcphy::sw and ::mux fields, which were declared with the wrong consumer-side types and never assigned.

Fixes 8e98ca1e74db ("phy: apple: Add Apple Type-C PHY")

Signed-off-by David Carlier <[email protected]>

---
 drivers/phy/apple/atc.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

drivers/phy/apple/atc.c +22 -5

index e9d106f135c5..4156fabad742 100644
@@ -628,9 +628,6 @@ struct apple_atcphy {
 
 	struct reset_controller_dev rcdev;
 
-	struct typec_switch *sw;
-	struct typec_mux *mux;
-
 	struct mutex lock;
 };

@@ -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);
 }
 
 static int atcphy_mux_set(struct typec_mux_dev *mux, struct typec_mux_state *state)

@@ -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);
 }
 
 static int atcphy_load_tunables(struct apple_atcphy *atcphy)

Signature · 1 line

2.53.0

Vladimir Oltean

2026-05-07 12:06 UTC #1 lore raw

Sashiko says:

commit 13bcb3149b5056d5815f8cd52f9e018076602989
Author: David Carlier <[email protected]>
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.

Quoted 4 lines

drivers/phy/apple/atc.c +0 -0
index e9d106f135c51..4156fabad742c 100644

[ ... ]

Quoted 15 lines

drivers/phy/apple/atc.c +6 -0

@@ -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,

drivers/phy/apple/atc.c static int atcphy_sw_set(struct typec_switch_dev *sw, enum typec_orientation ori · line 2085

Quoted 10 lines

drivers/phy/apple/atc.c +5 -1

 		.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? [ ... ]

Quoted 15 lines

drivers/phy/apple/atc.c +6 -0

@@ -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,

drivers/phy/apple/atc.c static int atcphy_mux_set(struct typec_mux_dev *mux, struct typec_mux_state *sta · line 2175

Quoted 10 lines

drivers/phy/apple/atc.c +5 -1

 		.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/[email protected]/

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.