diff mbox series

[1/6] power: supply: ab8500: Fix external_power_changed race

Message ID 20230415160734.70475-2-hdegoede@redhat.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series power: supply: Fix external_power_changed race in several drivers | expand

Commit Message

Hans de Goede April 15, 2023, 4:07 p.m. UTC
ab8500_btemp_external_power_changed() dereferences di->btemp_psy,
which gets sets in ab8500_btemp_probe() like this:

        di->btemp_psy = devm_power_supply_register(dev, &ab8500_btemp_desc,
                                                   &psy_cfg);

As soon as devm_power_supply_register() has called device_add()
the external_power_changed callback can get called. So there is a window
where ab8500_btemp_external_power_changed() may get called while
di->btemp_psy has not been set yet leading to a NULL pointer dereference.

Fixing this is easy. The external_power_changed callback gets passed
the power_supply which will eventually get stored in di->btemp_psy,
so ab8500_btemp_external_power_changed() can simply directly use
the passed in psy argument which is always valid.

And the same applies to ab8500_fg_external_power_changed().

Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note this has been compile tested only
---
 drivers/power/supply/ab8500_btemp.c | 6 ++----
 drivers/power/supply/ab8500_fg.c    | 6 ++----
 2 files changed, 4 insertions(+), 8 deletions(-)

Comments

Linus Walleij April 21, 2023, 8:39 a.m. UTC | #1
On Sat, Apr 15, 2023 at 6:07 PM Hans de Goede <hdegoede@redhat.com> wrote:

> ab8500_btemp_external_power_changed() dereferences di->btemp_psy,
> which gets sets in ab8500_btemp_probe() like this:
>
>         di->btemp_psy = devm_power_supply_register(dev, &ab8500_btemp_desc,
>                                                    &psy_cfg);
>
> As soon as devm_power_supply_register() has called device_add()
> the external_power_changed callback can get called. So there is a window
> where ab8500_btemp_external_power_changed() may get called while
> di->btemp_psy has not been set yet leading to a NULL pointer dereference.
>
> Fixing this is easy. The external_power_changed callback gets passed
> the power_supply which will eventually get stored in di->btemp_psy,
> so ab8500_btemp_external_power_changed() can simply directly use
> the passed in psy argument which is always valid.
>
> And the same applies to ab8500_fg_external_power_changed().
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Oh sweet!
Thanks for fixing this Hans.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/power/supply/ab8500_btemp.c b/drivers/power/supply/ab8500_btemp.c
index 307ee6f71042..6f83e99d2eb7 100644
--- a/drivers/power/supply/ab8500_btemp.c
+++ b/drivers/power/supply/ab8500_btemp.c
@@ -624,10 +624,8 @@  static int ab8500_btemp_get_ext_psy_data(struct device *dev, void *data)
  */
 static void ab8500_btemp_external_power_changed(struct power_supply *psy)
 {
-	struct ab8500_btemp *di = power_supply_get_drvdata(psy);
-
-	class_for_each_device(power_supply_class, NULL,
-		di->btemp_psy, ab8500_btemp_get_ext_psy_data);
+	class_for_each_device(power_supply_class, NULL, psy,
+			      ab8500_btemp_get_ext_psy_data);
 }
 
 /* ab8500 btemp driver interrupts and their respective isr */
diff --git a/drivers/power/supply/ab8500_fg.c b/drivers/power/supply/ab8500_fg.c
index 41a7bff9ac37..53560fbb6dcd 100644
--- a/drivers/power/supply/ab8500_fg.c
+++ b/drivers/power/supply/ab8500_fg.c
@@ -2407,10 +2407,8 @@  static int ab8500_fg_init_hw_registers(struct ab8500_fg *di)
  */
 static void ab8500_fg_external_power_changed(struct power_supply *psy)
 {
-	struct ab8500_fg *di = power_supply_get_drvdata(psy);
-
-	class_for_each_device(power_supply_class, NULL,
-		di->fg_psy, ab8500_fg_get_ext_psy_data);
+	class_for_each_device(power_supply_class, NULL, psy,
+			      ab8500_fg_get_ext_psy_data);
 }
 
 /**