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@xxxxxxxxxxx>:
> 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@xxxxxxxxxxxxx>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/