Re: [PATCH] power: supply: ds2760_battery: fix NULL pointer dereference in w1_ds2760_remove_slave()

[Sebastian Reichel <sre@kernel.org>]

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

Hi,

On Sat, Aug 08, 2026 at 03:44:58PM -0600, Ivy Lopez wrote:
> w1_ds2760_add_slave()'s failure paths (di_alloc_failed, batt_failed,
> workqueue_failed) are all empty labels that just return the error
> code, with no explicit unwind. This works for di itself and the
> registered power supply, both are devm-managed against the w1 slave
> device and get cleaned up automatically. But di->monitor_wqueue and
> di->pm_notifier are not devm-managed, and are only ever set up in
> the success tail of the function, after the workqueue allocation and
> power supply registration have both succeeded.
> 
> The w1 core's BUS_NOTIFY_ADD_DEVICE handling in w1_family_notify()
> logs and returns on a failing add_slave(), but does not prevent the
> w1 slave device from later being removed from the bus, which
> triggers BUS_NOTIFY_DEL_DEVICE and an unconditional call to
> remove_slave(). This means w1_ds2760_remove_slave() is effectively
> the only failure-unwind path for a partially initialized di, and
> needs to treat every field as potentially never having been set.
> 
> Currently it does not: it unconditionally calls
> destroy_workqueue(di->monitor_wqueue), which crashes with a NULL
> pointer dereference if add_slave() failed before or during the
> workqueue allocation (e.g. on a power_supply_register() failure, as
> seen when a colliding sysfs name from a misdetected slave device
> causes registration to fail).
> 
> sl->family_data can also be NULL if add_slave() failed at its own
> allocation, before family_data was ever set, which would crash on
> the very first dereference in remove_slave().
> 
> Fix both: return early if di is NULL, and only call
> destroy_workqueue() if monitor_wqueue was actually allocated.
> unregister_pm_notifier() and cancel_delayed_work_sync() are safe to
> call unconditionally: the former is a no-op if the notifier was
> never registered, and the latter operates on the embedded
> delayed_work struct, which is always validly initialized by the
> time remove_slave() can run with a non-NULL di.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217832
> Signed-off-by: Ivy Lopez <skunkolee@gmail.com>

This should get a Fixes tag.

> ---
>  drivers/power/supply/ds2760_battery.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/ds2760_battery.c b/drivers/power/supply/ds2760_battery.c
> index 142c7492c3c2..3c2433f6b5e9 100644
> --- a/drivers/power/supply/ds2760_battery.c
> +++ b/drivers/power/supply/ds2760_battery.c
> @@ -723,9 +723,13 @@ static void w1_ds2760_remove_slave(struct w1_slave *sl)
>  {
>  	struct ds2760_device_info *di = sl->family_data;
>  
> +	if (!di)
> +		return;
> +
>  	unregister_pm_notifier(&di->pm_notifier);
>  	cancel_delayed_work_sync(&di->monitor_work);
> -	destroy_workqueue(di->monitor_wqueue);
> +	if (di->monitor_wqueue)
> +		destroy_workqueue(di->monitor_wqueue);

Considering the driver has mostly been converted to device managed
resources already, I think it makes sense to simply use
devm_alloc_ordered_workqueue() in the probe function and thus avoid
this problem. Also while at it switch INIT_DELAYED_WORK to
devm_delayed_work_autocancel() to get rid of
w1_ds2760_remove_slave(). Just make sure to do this *after*
allocating the workqueue.

Last but not least use devm_add_action_or_reset() for the
unregister_pm_notifier and simply drop the complete
w1_ds2760_remove_slave() function.

Greetings,

-- Sebastian

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