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

[Sebastian Reichel <sebastian.reichel@collabora.com>]

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