[RFC/RFT 0/2] power_supply: Fix NULL pointer dereference from uevent

[RFC/RFT 0/2] power_supply: Fix NULL pointer dereference from uevent

[Krzysztof Kozlowski]

Hi,


This is an idea to fix issue in bq27x00 driver (and probably in others)
reported by H. Nikolaus Schaller [0].

The fixes are marked RFC/RFT because:
1. I do not have bq27x00-like device. I confirmed this and tested on
   modified drivers (max77693, ACPI AC). These drivers are not
   impacted by the issue but one can easily adjust them to reproduce
   the problem.
2. The first uevent coming from power_supply_register() is now
   different because it won't contain device properties. I am
   not sure how this impacts userspace.

Comments are welcomed.


[0] https://lkml.org/lkml/2015/5/18/152


Best regards,
Krzysztof

Krzysztof Kozlowski (2):
  power_supply: Fix NULL pointer dereference during bq27x00_battery
    probe
  power_supply: Fix possible NULL pointer dereference on early uevent

 drivers/power/power_supply_core.c | 59 +++++++++++++++++++++++++++++++++++----
 include/linux/power_supply.h      |  1 +
 2 files changed, 54 insertions(+), 6 deletions(-)

-- 
1.9.1

[RFC/RFT 1/2] power_supply: Fix NULL pointer dereference during bq27x00_battery probe

[Krzysztof Kozlowski]

Power supply is often registered during probe of a driver. The
power_supply_register() returns pointer to newly allocated structure as
return value. However before returning the power_supply_register()
calls back the get_property() method provided by the driver through
uevent.

In that time the driver probe is still in progress and driver did not
assigned pointer to power supply to its local variables. This leads to
NULL pointer dereference from get_property() function.
Starting from bq27x00_battery_probe():
  di->bat = power_supply_register()
    device_add()
      kobject_uevent()
        power_supply_uevent()
          power_supply_show_property()
            power_supply_get_property()
              bq27x00_battery_get_property()
                dereference of (di->bat) which is NULL here

The first uevent of power supply (the one coming from device creation)
should not call back to the driver. To prevent that from happening,
increment the atomic use counter at the end of power_supply_register().
This means that power_supply_get_property() will return -ENODEV.

IMPORTANT:
The patch has impact on this first uevent sent from power supply because
it will not contain properties from power supply.

The uevent with properties will be sent later after indicating that
power supply has changed. This also has a race now, but will be fixed in
other patches.

Reported-by: H. Nikolaus Schaller <hns@goldelico.com>
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Fixes: 297d716f6260 ("power_supply: Change ownership from driver to core")
---
 drivers/power/power_supply_core.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 2ed4a4a6b3c5..15da277e0e8d 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -659,7 +659,6 @@ __power_supply_register(struct device *parent,
 	dev->release = power_supply_dev_release;
 	dev_set_drvdata(dev, psy);
 	psy->desc = desc;
-	atomic_inc(&psy->use_cnt);
 	if (cfg) {
 		psy->drv_data = cfg->drv_data;
 		psy->of_node = cfg->of_node;
@@ -700,6 +699,16 @@ __power_supply_register(struct device *parent,
 	if (rc)
 		goto create_triggers_failed;
 
+	/*
+	 * Update use_cnt after any uevents (most notably from device_add()).
+	 * We are here still during driver's probe but
+	 * the power_supply_uevent() calls back driver's get_property
+	 * method so:
+	 * 1. Driver did not assigned the returned struct power_supply,
+	 * 2. Driver could not finish initialization (anything in its probe
+	 *    after calling power_supply_register()).
+	 */
+	atomic_inc(&psy->use_cnt);
 	power_supply_changed(psy);
 
 	return psy;
-- 
1.9.1

[RFC/RFT 2/2] power_supply: Fix possible NULL pointer dereference on early uevent

[Krzysztof Kozlowski]

Don't call the power_supply_changed() from power_supply_register() when
parent is still probing because it may lead to accessing parent too
early.

In bq27x00_battery this caused NULL pointer exception because uevent of
power_supply_changed called back the the get_property() method provided
by the driver. The get_property() method accessed pointer which should
be returned by power_supply_register().

Starting from bq27x00_battery_probe():
  di->bat = power_supply_register()
    power_supply_changed()
      kobject_uevent()
        power_supply_uevent()
          power_supply_show_property()
            power_supply_get_property()
              bq27x00_battery_get_property()
                dereference of di->bat which is NULL here

The dereference of di->bat (value returned by power_supply_register())
is the currently visible problem. However calling back the methods
provided by driver before ending the probe may lead to accessing other
driver-related data which is not yet initialized.

The call to power_supply_changed() is postponed till probing ends -
mutex of parent device is released.

Reported-by: H. Nikolaus Schaller <hns@goldelico.com>
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Fixes: 297d716f6260 ("power_supply: Change ownership from driver to core")
---
 drivers/power/power_supply_core.c | 48 +++++++++++++++++++++++++++++++++++----
 include/linux/power_supply.h      |  1 +
 2 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 15da277e0e8d..3a36d2940803 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -30,6 +30,8 @@ EXPORT_SYMBOL_GPL(power_supply_notifier);
 
 static struct device_type power_supply_dev_type;
 
+#define POWER_SUPPLY_DEFERRED_REGISTER_TIME	msecs_to_jiffies(10)
+
 static bool __power_supply_is_supplied_by(struct power_supply *supplier,
 					 struct power_supply *supply)
 {
@@ -121,6 +123,30 @@ void power_supply_changed(struct power_supply *psy)
 }
 EXPORT_SYMBOL_GPL(power_supply_changed);
 
+/*
+ * Notify that power supply was registered after parent finished the probing.
+ *
+ * Often power supply is registered from driver's probe function. However
+ * calling power_supply_changed() directly from power_supply_register()
+ * would lead to execution of get_property() function provided by the driver
+ * too early - before the probe ends.
+ *
+ * Avoid that by waiting on parent's mutex.
+ */
+static void power_supply_deferred_register_work(struct work_struct *work)
+{
+	struct power_supply *psy = container_of(work, struct power_supply,
+						deferred_register_work);
+
+	if (psy->dev.parent)
+		mutex_lock(&psy->dev.parent->mutex);
+
+	power_supply_changed(psy);
+
+	if (psy->dev.parent)
+		mutex_unlock(&psy->dev.parent->mutex);
+}
+
 #ifdef CONFIG_OF
 #include <linux/of.h>
 
@@ -645,6 +671,8 @@ __power_supply_register(struct device *parent,
 	struct power_supply *psy;
 	int rc;
 
+	WARN(!parent, "Expected proper parent device\n");
+
 	psy = kzalloc(sizeof(*psy), GFP_KERNEL);
 	if (!psy)
 		return ERR_PTR(-ENOMEM);
@@ -671,6 +699,8 @@ __power_supply_register(struct device *parent,
 		goto dev_set_name_failed;
 
 	INIT_WORK(&psy->changed_work, power_supply_changed_work);
+	INIT_DELAYED_WORK(&psy->deferred_register_work,
+			  power_supply_deferred_register_work);
 
 	rc = power_supply_check_supplies(psy);
 	if (rc) {
@@ -709,7 +739,10 @@ __power_supply_register(struct device *parent,
 	 *    after calling power_supply_register()).
 	 */
 	atomic_inc(&psy->use_cnt);
-	power_supply_changed(psy);
+
+	queue_delayed_work(system_power_efficient_wq,
+			   &psy->deferred_register_work,
+			   POWER_SUPPLY_DEFERRED_REGISTER_TIME);
 
 	return psy;
 
@@ -729,7 +762,8 @@ dev_set_name_failed:
 
 /**
  * power_supply_register() - Register new power supply
- * @parent:	Device to be a parent of power supply's device
+ * @parent:	Device to be a parent of power supply's device, usually
+ *		the device which probe function calls this
  * @desc:	Description of power supply, must be valid through whole
  *		lifetime of this power supply
  * @cfg:	Run-time specific configuration accessed during registering,
@@ -750,7 +784,8 @@ EXPORT_SYMBOL_GPL(power_supply_register);
 
 /**
  * power_supply_register() - Register new non-waking-source power supply
- * @parent:	Device to be a parent of power supply's device
+ * @parent:	Device to be a parent of power supply's device, usually
+ *		the device which probe function calls this
  * @desc:	Description of power supply, must be valid through whole
  *		lifetime of this power supply
  * @cfg:	Run-time specific configuration accessed during registering,
@@ -779,7 +814,8 @@ static void devm_power_supply_release(struct device *dev, void *res)
 
 /**
  * power_supply_register() - Register managed power supply
- * @parent:	Device to be a parent of power supply's device
+ * @parent:	Device to be a parent of power supply's device, usually
+ *		the device which probe function calls this
  * @desc:	Description of power supply, must be valid through whole
  *		lifetime of this power supply
  * @cfg:	Run-time specific configuration accessed during registering,
@@ -814,7 +850,8 @@ EXPORT_SYMBOL_GPL(devm_power_supply_register);
 
 /**
  * power_supply_register() - Register managed non-waking-source power supply
- * @parent:	Device to be a parent of power supply's device
+ * @parent:	Device to be a parent of power supply's device, usually
+ *		the device which probe function calls this
  * @desc:	Description of power supply, must be valid through whole
  *		lifetime of this power supply
  * @cfg:	Run-time specific configuration accessed during registering,
@@ -858,6 +895,7 @@ void power_supply_unregister(struct power_supply *psy)
 {
 	WARN_ON(atomic_dec_return(&psy->use_cnt));
 	cancel_work_sync(&psy->changed_work);
+	cancel_delayed_work_sync(&psy->deferred_register_work);
 	sysfs_remove_link(&psy->dev.kobj, "powers");
 	power_supply_remove_triggers(psy);
 	psy_unregister_cooler(psy);
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 75a1dd8dc56e..a80f1fd01ddb 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -237,6 +237,7 @@ struct power_supply {
 	/* private */
 	struct device dev;
 	struct work_struct changed_work;
+	struct delayed_work deferred_register_work;
 	spinlock_t changed_lock;
 	bool changed;
 	atomic_t use_cnt;
-- 
1.9.1

Re: [RFC/RFT 2/2] power_supply: Fix possible NULL pointer dereference on early uevent

[Krzysztof Kozlowski]

2015-05-19 14:26 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> Don't call the power_supply_changed() from power_supply_register() when
> parent is still probing because it may lead to accessing parent too
> early.
>
> In bq27x00_battery this caused NULL pointer exception because uevent of
> power_supply_changed called back the the get_property() method provided
> by the driver. The get_property() method accessed pointer which should
> be returned by power_supply_register().
>
> Starting from bq27x00_battery_probe():
>   di->bat = power_supply_register()
>     power_supply_changed()
>       kobject_uevent()
>         power_supply_uevent()
>           power_supply_show_property()
>             power_supply_get_property()
>               bq27x00_battery_get_property()
>                 dereference of di->bat which is NULL here
>
> The dereference of di->bat (value returned by power_supply_register())
> is the currently visible problem. However calling back the methods
> provided by driver before ending the probe may lead to accessing other
> driver-related data which is not yet initialized.
>
> The call to power_supply_changed() is postponed till probing ends -
> mutex of parent device is released.
>
> Reported-by: H. Nikolaus Schaller <hns@goldelico.com>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Fixes: 297d716f6260 ("power_supply: Change ownership from driver to core")
> ---
>  drivers/power/power_supply_core.c | 48 +++++++++++++++++++++++++++++++++++----
>  include/linux/power_supply.h      |  1 +
>  2 files changed, 44 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> index 15da277e0e8d..3a36d2940803 100644
> --- a/drivers/power/power_supply_core.c
> +++ b/drivers/power/power_supply_core.c
> @@ -30,6 +30,8 @@ EXPORT_SYMBOL_GPL(power_supply_notifier);
>
>  static struct device_type power_supply_dev_type;
>
> +#define POWER_SUPPLY_DEFERRED_REGISTER_TIME    msecs_to_jiffies(10)
> +
>  static bool __power_supply_is_supplied_by(struct power_supply *supplier,
>                                          struct power_supply *supply)
>  {
> @@ -121,6 +123,30 @@ void power_supply_changed(struct power_supply *psy)
>  }
>  EXPORT_SYMBOL_GPL(power_supply_changed);
>
> +/*
> + * Notify that power supply was registered after parent finished the probing.
> + *
> + * Often power supply is registered from driver's probe function. However
> + * calling power_supply_changed() directly from power_supply_register()
> + * would lead to execution of get_property() function provided by the driver
> + * too early - before the probe ends.
> + *
> + * Avoid that by waiting on parent's mutex.
> + */
> +static void power_supply_deferred_register_work(struct work_struct *work)
> +{
> +       struct power_supply *psy = container_of(work, struct power_supply,
> +                                               deferred_register_work);

There is an obvious error here. It should be
"deferred_register_work.work". I will make a resend.

Krzysztof