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

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

[Ivy Lopez]

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>
---
 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);
 }
 
 #ifdef CONFIG_OF
-- 
2.55.0