[PATCH] power: supply: charger-manager: tear down sysfs and disable charging before freeing regulators

[PATCH] power: supply: charger-manager: tear down sysfs and disable charging before freeing regulators

[Fan Wu]

charger_manager_remove() and the err_reg_extcon probe error path call
regulator_put() before tearing down the power_supply sysfs entries
(power_supply_unregister()). charger_manager_remove() also calls
try_charger_enable(cm, false) after the regulator_put() loop. A concurrent
write to a charger's externally_control sysfs attribute that lands between
regulator_put() and power_supply_unregister() can run
charger_externally_control_store() and call try_charger_enable(), which,
when charging is enabled, dereferences the already-freed consumer handle
via regulator_enable(), regulator_disable(), regulator_is_enabled() and
regulator_force_disable(). When charging is enabled, try_charger_enable(cm,
false) in .remove() also dereferences the freed handles directly. Both
leave use-after-free windows.

Move power_supply_unregister() ahead of the regulator_put() loop on both
paths. It ends in device_unregister(), which removes the sysfs attribute
groups and drains active kernfs operations, so on return no
externally_control store can be running or start again; it does not touch
the regulator consumers, and psy->desc references the devm-managed
cm->charger_psy_desc, which outlives the call. Move try_charger_enable(cm,
false) before the regulator_put() loop in .remove() as well, so the
synchronous disable also runs on valid handles.

This does not address the separate extcon-notifier path, which needs its
own synchronization design.

Found by an in-house static analysis tool.

Fixes: 3950c7865cd7 ("charger-manager: Add support sysfs entry for charger")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/power/supply/charger-manager.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/power/supply/charger-manager.c b/drivers/power/supply/charger-manager.c
index c49e0e4d02f7..c1df512f32a8 100644
--- a/drivers/power/supply/charger-manager.c
+++ b/drivers/power/supply/charger-manager.c
@@ -1622,11 +1622,11 @@ static int charger_manager_probe(struct platform_device *pdev)
 	return 0;
 
 err_reg_extcon:
+	power_supply_unregister(cm->charger_psy);
+
 	for (i = 0; i < desc->num_charger_regulators; i++)
 		regulator_put(desc->charger_regulators[i].consumer);
 
-	power_supply_unregister(cm->charger_psy);
-
 	return ret;
 }
 
@@ -1644,12 +1644,12 @@ static void charger_manager_remove(struct platform_device *pdev)
 	cancel_work_sync(&setup_polling);
 	cancel_delayed_work_sync(&cm_monitor_work);
 
-	for (i = 0 ; i < desc->num_charger_regulators ; i++)
-		regulator_put(desc->charger_regulators[i].consumer);
+	try_charger_enable(cm, false);
 
 	power_supply_unregister(cm->charger_psy);
 
-	try_charger_enable(cm, false);
+	for (i = 0 ; i < desc->num_charger_regulators ; i++)
+		regulator_put(desc->charger_regulators[i].consumer);
 }
 
 static const struct platform_device_id charger_manager_id[] = {
-- 
2.34.1

Re: [PATCH] power: supply: charger-manager: tear down sysfs and disable charging before freeing regulators

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3561 bytes --]

Hi,

On Sun, Jul 26, 2026 at 05:15:40AM +0000, Fan Wu wrote:
> charger_manager_remove() and the err_reg_extcon probe error path call
> regulator_put() before tearing down the power_supply sysfs entries
> (power_supply_unregister()). charger_manager_remove() also calls
> try_charger_enable(cm, false) after the regulator_put() loop. A concurrent
> write to a charger's externally_control sysfs attribute that lands between
> regulator_put() and power_supply_unregister() can run
> charger_externally_control_store() and call try_charger_enable(), which,
> when charging is enabled, dereferences the already-freed consumer handle
> via regulator_enable(), regulator_disable(), regulator_is_enabled() and
> regulator_force_disable(). When charging is enabled, try_charger_enable(cm,
> false) in .remove() also dereferences the freed handles directly. Both
> leave use-after-free windows.
> 
> Move power_supply_unregister() ahead of the regulator_put() loop on both
> paths. It ends in device_unregister(), which removes the sysfs attribute
> groups and drains active kernfs operations, so on return no
> externally_control store can be running or start again; it does not touch
> the regulator consumers, and psy->desc references the devm-managed
> cm->charger_psy_desc, which outlives the call. Move try_charger_enable(cm,
> false) before the regulator_put() loop in .remove() as well, so the
> synchronous disable also runs on valid handles.
> 
> This does not address the separate extcon-notifier path, which needs its
> own synchronization design.
> 
> Found by an in-house static analysis tool.
> 
> Fixes: 3950c7865cd7 ("charger-manager: Add support sysfs entry for charger")
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:gpt-5.6
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> ---

If the power-supply unregister needs to happen before the regulator
unregister, the regulator also needs to be registered before the
power-supply device to avoid early userspace access to the sysfs
files before the regulator is registered. I.e. this patch should
also move the charger_manager_register_extcon() call.

Greetings,

-- Sebastian

>  drivers/power/supply/charger-manager.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/supply/charger-manager.c b/drivers/power/supply/charger-manager.c
> index c49e0e4d02f7..c1df512f32a8 100644
> --- a/drivers/power/supply/charger-manager.c
> +++ b/drivers/power/supply/charger-manager.c
> @@ -1622,11 +1622,11 @@ static int charger_manager_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_reg_extcon:
> +	power_supply_unregister(cm->charger_psy);
> +
>  	for (i = 0; i < desc->num_charger_regulators; i++)
>  		regulator_put(desc->charger_regulators[i].consumer);
>  
> -	power_supply_unregister(cm->charger_psy);
> -
>  	return ret;
>  }
>  
> @@ -1644,12 +1644,12 @@ static void charger_manager_remove(struct platform_device *pdev)
>  	cancel_work_sync(&setup_polling);
>  	cancel_delayed_work_sync(&cm_monitor_work);
>  
> -	for (i = 0 ; i < desc->num_charger_regulators ; i++)
> -		regulator_put(desc->charger_regulators[i].consumer);
> +	try_charger_enable(cm, false);
>  
>  	power_supply_unregister(cm->charger_psy);
>  
> -	try_charger_enable(cm, false);
> +	for (i = 0 ; i < desc->num_charger_regulators ; i++)
> +		regulator_put(desc->charger_regulators[i].consumer);
>  }
>  
>  static const struct platform_device_id charger_manager_id[] = {
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2] power: supply: charger-manager: register regulators before exposing sysfs

[Fan Wu]

charger_manager_remove() and the err_reg_extcon probe error path free each
charger regulator with regulator_put() before tearing down the power_supply
sysfs entries (power_supply_unregister()). charger_manager_remove() also
calls try_charger_enable(cm, false) after the regulator_put() loop. A
concurrent write to a charger's externally_control sysfs attribute that
lands between regulator_put() and power_supply_unregister() can run
charger_externally_control_store() and call try_charger_enable(), which,
when charging is enabled, dereferences the already-freed consumer handle.
When charging is enabled, try_charger_enable(cm, false) in .remove() also
dereferences the freed handles directly. Both leave use-after-free windows.
Symmetrically, probe registers the sysfs entries (power_supply_register)
before acquiring the regulators (regulator_get, inside
charger_manager_register_extcon), so userspace can reach externally_control
before the regulators are available.

Split charger_manager_register_extcon() on the sync/async boundary:
charger_manager_get_regulators() (regulator_get only, no async producer)
now runs before power_supply_register() so sysfs is not live before
regulators are available, and charger_manager_register_extcon() keeps only
the extcon notifier/work setup, still after power_supply_register() so a
power_supply_register() failure cannot reach extcon setup. This keeps the
sysfs setup/teardown ordering symmetric without introducing an asynchronous
producer on the earlier probe-error path.

Move power_supply_unregister() and try_charger_enable(cm, false) ahead of
the regulator_put() loop on both teardown paths, and adjust err_reg_extcon
(power_supply_unregister() then fall through err_regulator for
regulator_put(); get_regulators self-rolls back on its own failure).

This does not address the separate extcon-notifier-driven deref of the same
handles, which needs its own synchronization design.

Found by an in-house static analysis tool.

Fixes: 3950c7865cd7 ("charger-manager: Add support sysfs entry for charger")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
Changes in v2 (per Sebastian Reichel's review of v1):
- Split charger_manager_register_extcon() so regulator acquisition
  (charger_manager_get_regulators) happens before power_supply_register(),
  while notifier/work setup remains after it. Keeps sysfs setup/teardown
  ordering symmetric without an async producer on the earlier probe-error
  path.
- Add err_regulator label; get_regulators self-rolls back on failure.
- Move try_charger_enable(cm, false) before the regulator_put() loop.

 drivers/power/supply/charger-manager.c | 54 +++++++++++++++++++-------
 1 file changed, 39 insertions(+), 15 deletions(-)

diff --git a/drivers/power/supply/charger-manager.c b/drivers/power/supply/charger-manager.c
index c49e0e4d02f7..1d3f3f2295e0 100644
--- a/drivers/power/supply/charger-manager.c
+++ b/drivers/power/supply/charger-manager.c
@@ -1016,6 +1016,29 @@ static int charger_extcon_init(struct charger_manager *cm,
 	return 0;
 }
 
+static int charger_manager_get_regulators(struct charger_manager *cm)
+{
+	struct charger_desc *desc = cm->desc;
+	struct charger_regulator *charger;
+	int i, ret;
+
+	for (i = 0; i < desc->num_charger_regulators; i++) {
+		charger = &desc->charger_regulators[i];
+		charger->consumer = regulator_get(cm->dev,
+						  charger->regulator_name);
+		if (IS_ERR(charger->consumer)) {
+			dev_err(cm->dev, "Cannot find charger(%s)\n",
+				charger->regulator_name);
+			ret = PTR_ERR(charger->consumer);
+			while (i-- > 0)
+				regulator_put(desc->charger_regulators[i].consumer);
+			return ret;
+		}
+		charger->cm = cm;
+	}
+	return 0;
+}
+
 /**
  * charger_manager_register_extcon - Register extcon device to receive state
  *				     of charger cable.
@@ -1038,15 +1061,6 @@ static int charger_manager_register_extcon(struct charger_manager *cm)
 	for (i = 0; i < desc->num_charger_regulators; i++) {
 		charger = &desc->charger_regulators[i];
 
-		charger->consumer = regulator_get(cm->dev,
-					charger->regulator_name);
-		if (IS_ERR(charger->consumer)) {
-			dev_err(cm->dev, "Cannot find charger(%s)\n",
-				charger->regulator_name);
-			return PTR_ERR(charger->consumer);
-		}
-		charger->cm = cm;
-
 		for (j = 0; j < charger->num_cables; j++) {
 			struct charger_cable *cable = &charger->cables[j];
 
@@ -1582,13 +1596,23 @@ static int charger_manager_probe(struct platform_device *pdev)
 	}
 	psy_cfg.attr_grp = desc->sysfs_groups;
 
+	/*
+	 * Acquire charger regulators before exposing the sysfs entries, so
+	 * userspace cannot reach externally_control before the regulators
+	 * (and charger->cm) are available.  Mirrors the order in remove().
+	 */
+	ret = charger_manager_get_regulators(cm);
+	if (ret < 0)
+		return ret;
+
 	cm->charger_psy = power_supply_register(&pdev->dev,
 						&cm->charger_psy_desc,
 						&psy_cfg);
 	if (IS_ERR(cm->charger_psy)) {
 		dev_err(&pdev->dev, "Cannot register charger-manager with name \"%s\"\n",
 			cm->charger_psy_desc.name);
-		return PTR_ERR(cm->charger_psy);
+		ret = PTR_ERR(cm->charger_psy);
+		goto err_regulator;
 	}
 
 	/* Register extcon device for charger cable */
@@ -1622,11 +1646,11 @@ static int charger_manager_probe(struct platform_device *pdev)
 	return 0;
 
 err_reg_extcon:
+	power_supply_unregister(cm->charger_psy);
+err_regulator:
 	for (i = 0; i < desc->num_charger_regulators; i++)
 		regulator_put(desc->charger_regulators[i].consumer);
 
-	power_supply_unregister(cm->charger_psy);
-
 	return ret;
 }
 
@@ -1644,12 +1668,12 @@ static void charger_manager_remove(struct platform_device *pdev)
 	cancel_work_sync(&setup_polling);
 	cancel_delayed_work_sync(&cm_monitor_work);
 
-	for (i = 0 ; i < desc->num_charger_regulators ; i++)
-		regulator_put(desc->charger_regulators[i].consumer);
+	try_charger_enable(cm, false);
 
 	power_supply_unregister(cm->charger_psy);
 
-	try_charger_enable(cm, false);
+	for (i = 0 ; i < desc->num_charger_regulators ; i++)
+		regulator_put(desc->charger_regulators[i].consumer);
 }
 
 static const struct platform_device_id charger_manager_id[] = {
-- 
2.34.1

Re: [PATCH v2] power: supply: charger-manager: register regulators before exposing sysfs

[Sebastian Reichel]

On Tue, 28 Jul 2026 03:01:23 +0000, Fan Wu wrote:
> charger_manager_remove() and the err_reg_extcon probe error path free each
> charger regulator with regulator_put() before tearing down the power_supply
> sysfs entries (power_supply_unregister()). charger_manager_remove() also
> calls try_charger_enable(cm, false) after the regulator_put() loop. A
> concurrent write to a charger's externally_control sysfs attribute that
> lands between regulator_put() and power_supply_unregister() can run
> charger_externally_control_store() and call try_charger_enable(), which,
> when charging is enabled, dereferences the already-freed consumer handle.
> When charging is enabled, try_charger_enable(cm, false) in .remove() also
> dereferences the freed handles directly. Both leave use-after-free windows.
> Symmetrically, probe registers the sysfs entries (power_supply_register)
> before acquiring the regulators (regulator_get, inside
> charger_manager_register_extcon), so userspace can reach externally_control
> before the regulators are available.
> 
> [...]

Applied, thanks!

[1/1] power: supply: charger-manager: register regulators before exposing sysfs
      commit: c57cb36f76eb7ced45f57af1a890d8f3a6d76342

Best regards,
-- 
Sebastian Reichel <sebastian.reichel@collabora.com>