Skip to contentSkip to conversation

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

PATCH Superseded David Carlier 4 participants 5 messages May 7 – May 14

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 phy: apple: atc: Fix typec switch/mux leak 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 when the driver unbinds (e.g. via sysfs unbind) neither typec_switch_unregister() nor typec_mux_unregister() is called. The framework reference taken in typec_switch_register() (device_initialize() + device_add() in drivers/usb/typec/mux.c) is therefore never dropped and the typec_switch_dev / typec_mux_dev objects stay live forever, with their sysfs entries under the typec_mux class also left behind. A subsequent rebind cannot recreate them with the same fwnode-derived name.

Save the registered handles and unregister them through devm_add_action_or_reset() so framework registration is torn down in step with the driver's other devm-managed state. While here, drop struct apple_atcphy::sw and ::mux: they were declared with the consumer-side types (typec_switch *, typec_mux *) instead of the provider-side types and were never assigned.

Scope of the fix
----------------
This patch fixes the registration leak only. It does not close the use-after-free window that arises when a consumer that obtained a reference via fwnode_typec_switch_get() / fwnode_typec_mux_get() outlives the provider unbind: such consumers keep the underlying typec_switch_dev / typec_mux_dev alive past device_unregister(), and a later typec_switch_set() / typec_mux_set() still invokes the registered atcphy_sw_set() / atcphy_mux_set(), which dereferences the freed apple_atcphy through typec_{switch,mux}_get_drvdata().

On Apple Silicon the relevant consumers are the typec port and the cd321x controller registered by drivers/usb/typec/tipd/core.c. Cable plug / orientation events and alt-mode transitions trigger the .set callbacks via:

tps6598x_interrupt() drivers/usb/typec/tipd/core.c

    tps6598x_handle_plug_event()
      tps6598x_connect()/_disconnect()
        typec_set_orientation()        drivers/usb/typec/class.c
          typec_switch_set(port->sw)   drivers/usb/typec/mux.c
            atcphy_sw_set()            drivers/phy/apple/atc.c

cd321x_update_work() drivers/usb/typec/tipd/core.c

    cd321x_typec_update_mode()
      typec_mux_set(cd321x->mux)       drivers/usb/typec/mux.c
        atcphy_mux_set()               drivers/phy/apple/atc.c

Closing that window requires framework support for invalidating consumer-held references on provider unbind. The same consumer-survives-provider pattern has been discussed for the PHY framework [1] and is out of scope here.

[1] https://lore.kernel.org/linux-phy/[email protected]/

Fixes 8e98ca1e74db ("phy: apple: Add Apple Type-C PHY")
---
 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
Reviewed-by on v2 1/1
#1 lore raw
Quoted 42 lines · David Carlier

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 when the driver unbinds (e.g. via sysfs unbind) neither typec_switch_unregister() nor typec_mux_unregister() is called. The framework reference taken in typec_switch_register() (device_initialize() + device_add() in drivers/usb/typec/mux.c) is therefore never dropped and the typec_switch_dev / typec_mux_dev objects stay live forever, with their sysfs entries under the typec_mux class also left behind. A subsequent rebind cannot recreate them with the same fwnode-derived name.

Save the registered handles and unregister them through devm_add_action_or_reset() so framework registration is torn down in step with the driver's other devm-managed state. While here, drop struct apple_atcphy::sw and ::mux: they were declared with the consumer-side types (typec_switch *, typec_mux *) instead of the provider-side types and were never assigned.

Scope of the fix
----------------
This patch fixes the registration leak only. It does not close the use-after-free window that arises when a consumer that obtained a reference via fwnode_typec_switch_get() / fwnode_typec_mux_get() outlives the provider unbind: such consumers keep the underlying typec_switch_dev / typec_mux_dev alive past device_unregister(), and a later typec_switch_set() / typec_mux_set() still invokes the registered atcphy_sw_set() / atcphy_mux_set(), which dereferences the freed apple_atcphy through typec_{switch,mux}_get_drvdata().

On Apple Silicon the relevant consumers are the typec port and the cd321x controller registered by drivers/usb/typec/tipd/core.c. Cable plug / orientation events and alt-mode transitions trigger the .set callbacks via:

tps6598x_interrupt() drivers/usb/typec/tipd/core.c

    tps6598x_handle_plug_event()
      tps6598x_connect()/_disconnect()
        typec_set_orientation()        drivers/usb/typec/class.c
          typec_switch_set(port->sw)   drivers/usb/typec/mux.c
            atcphy_sw_set()            drivers/phy/apple/atc.c
Quoted 4 lines

cd321x_update_work() drivers/usb/typec/tipd/core.c

    cd321x_typec_update_mode()
      typec_mux_set(cd321x->mux)       drivers/usb/typec/mux.c
        atcphy_mux_set()               drivers/phy/apple/atc.c

Ok, so the claim from v1 that this patch fixes crashes from these code paths is not correct, since there is nothing that would make the typec port drop its references acquired via typec_switch_get() and typec_mux_get().

Quoted 10 lines

Closing that window requires framework support for invalidating consumer-held references on provider unbind. The same consumer-survives-provider pattern has been discussed for the PHY framework [1] and is out of scope here.

[1] https://lore.kernel.org/linux-phy/[email protected]/

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

---

The commit message is much better. But there is a checkpatch issue which appears to be valid, see:

commit 931d5c36c7369b65adb9e3d197a8d3a8a913db8c

Author: Joe Perches <[email protected]>
Date:   Fri Jan 16 09:42:52 2026 -0800

checkpatch: add an invalid patch separator test

Some versions of tools that apply patches incorrectly allow lines that start with 3 dashes and have additional content on the same line.

Checkpatch will now emit an ERROR on these lines and optionally convert those lines from dashes to equals with --fix.

I don't have such tooling (git am from version 2.43.0 applies the patch without discarding the text beneath "Scope of the fix" just fine), but the commit is from Jan 2026, so that tooling must still exist somewhere. So please resent with different formatting somehow (either a space before the title, or replace the ---- with ==== or ~~~~, whatever).

With that addressed, please add:

#2 lore raw

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

In reply to David Carlier

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 when the driver unbinds (e.g. via sysfs unbind) neither typec_switch_unregister() nor typec_mux_unregister() is called. The framework reference taken in typec_switch_register() (device_initialize() + device_add() in drivers/usb/typec/mux.c) is therefore never dropped and the typec_switch_dev / typec_mux_dev objects stay live forever, with their sysfs entries under the typec_mux class also left behind. A subsequent rebind cannot recreate them with the same fwnode-derived name.

Save the registered handles and unregister them through devm_add_action_or_reset() so framework registration is torn down in step with the driver's other devm-managed state. While here, drop struct apple_atcphy::sw and ::mux: they were declared with the consumer-side types (typec_switch *, typec_mux *) instead of the provider-side types and were never assigned.

Scope of the fix
================
This patch fixes the registration leak only. It does not close the use-after-free window that arises when a consumer that obtained a reference via fwnode_typec_switch_get() / fwnode_typec_mux_get() outlives the provider unbind: such consumers keep the underlying typec_switch_dev / typec_mux_dev alive past device_unregister(), and a later typec_switch_set() / typec_mux_set() still invokes the registered atcphy_sw_set() / atcphy_mux_set(), which dereferences the freed apple_atcphy through typec_{switch,mux}_get_drvdata().

On Apple Silicon the relevant consumers are the typec port and the cd321x controller registered by drivers/usb/typec/tipd/core.c. Cable plug / orientation events and alt-mode transitions trigger the .set callbacks via:

tps6598x_interrupt() drivers/usb/typec/tipd/core.c

    tps6598x_handle_plug_event()
      tps6598x_connect()/_disconnect()
        typec_set_orientation()        drivers/usb/typec/class.c
          typec_switch_set(port->sw)   drivers/usb/typec/mux.c
            atcphy_sw_set()            drivers/phy/apple/atc.c

cd321x_update_work() drivers/usb/typec/tipd/core.c

    cd321x_typec_update_mode()
      typec_mux_set(cd321x->mux)       drivers/usb/typec/mux.c
        atcphy_mux_set()               drivers/phy/apple/atc.c

Closing that window requires framework support for invalidating consumer-held references on provider unbind. The same consumer-survives-provider pattern has been discussed for the PHY framework [1] and is out of scope here.

[1] https://lore.kernel.org/linux-phy/[email protected]/

Fixes 8e98ca1e74db ("phy: apple: Add Apple Type-C PHY")
---
 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
Tested-by on v3 1/1
#3 lore raw
Quoted 61 lines · David Carlier
 drivers/phy/apple/atc.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)
drivers/phy/apple/atc.c +17 -4
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,
 	};

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

Quoted 10 lines
drivers/phy/apple/atc.c +5 -1
 
-	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)

Looks okay. Ran it on M1 MBP, no issues as far as I can tell.

maintainer Applied
#4 lore raw
In reply to David Carlier
Quoted 12 lines · David Carlier

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 when the driver unbinds (e.g. via sysfs unbind) neither typec_switch_unregister() nor typec_mux_unregister() is called. The framework reference taken in typec_switch_register() (device_initialize() + device_add() in drivers/usb/typec/mux.c) is therefore never dropped and the typec_switch_dev / typec_mux_dev objects stay live forever, with their sysfs entries under the typec_mux class also left behind. A subsequent rebind cannot recreate them with the same fwnode-derived name.

Quoted 1 line

[...]

Applied, thanks!

[1/1] phy: apple: atc: Fix typec switch/mux leak on unbind

      commit: 1854082fe0ddb81bc93d1f8e8a00554217fd09d1

Best regards,

Signature · 1 line
~Vinod