[PATCH] power: supply: ds2760_battery: fix NULL pointer dereference in w1_ds2760_remove_slave()
Ivy Lopez <[email protected]>
| Newsgroups | gmane.linux.kernel,gmane.linux.power-management.general |
|---|---|
| Message-ID | <[email protected]> |
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 <[email protected]> --- 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