* Re: [PATCH] power: supply: charger-manager: tear down sysfs and disable charging before freeing regulators
2026-07-26 5:15 [PATCH] power: supply: charger-manager: tear down sysfs and disable charging before freeing regulators Fan Wu
@ 2026-07-26 20:07 ` Sebastian Reichel
2026-07-28 3:01 ` [PATCH v2] power: supply: charger-manager: register regulators before exposing sysfs Fan Wu
1 sibling, 0 replies; 4+ messages in thread
From: Sebastian Reichel @ 2026-07-26 20:07 UTC (permalink / raw)
To: Fan Wu; +Cc: linux-pm, linux-kernel, myungjoo.ham, stable
[-- 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 --]
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH v2] power: supply: charger-manager: register regulators before exposing sysfs
2026-07-26 5:15 [PATCH] power: supply: charger-manager: tear down sysfs and disable charging before freeing regulators Fan Wu
2026-07-26 20:07 ` Sebastian Reichel
@ 2026-07-28 3:01 ` Fan Wu
2026-07-30 22:09 ` Sebastian Reichel
1 sibling, 1 reply; 4+ messages in thread
From: Fan Wu @ 2026-07-28 3:01 UTC (permalink / raw)
To: sre; +Cc: linux-pm, linux-kernel, myungjoo.ham, Fan Wu, stable
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
^ permalink raw reply [flat|nested] 4+ messages in thread