[RFT PATCH 0/4] Check return value of power_supply_register

[RFT PATCH 0/4] Check return value of power_supply_register

[Krzysztof Kozlowski]

Hi,


During global rework of power supply register API I found possible
issues related to not checked return value.

The power_supply_register() return value was ignored and during driver
removal the power supply was always unregistered. This theoretically
could lead to invalid memory accesses. However I did not reproduce
the issue (only code analysis).

Affected drivers:
1. twl4030_madc
2. compal-laptop
3. ipaq_micro_battery

I prepared patches but DID NOT test them. Only compilation + sparse
 + smatch + coccicheck.

I am kindly asking for review and testing.


P.S. The acpi/sbs.c driver has the same issue. Before unregistering
power supply it checks if sbs->charger.dev is non-NULL... which will
be non-NULL in most error-paths. However the driver is little more
complicated so fixing this without doing tests would be very
error-prone.


Best regards,
Krzysztof


Krzysztof Kozlowski (4):
  power_supply: twl4030_madc: Check return value of
    power_supply_register
  compal-laptop: Check return value of power_supply_register
  power_supply: ipaq_micro_battery: Fix leaking workqueue
  power_supply: ipaq_micro_battery: Check return values in probe

 drivers/platform/x86/compal-laptop.c |  7 ++++++-
 drivers/power/ipaq_micro_battery.c   | 22 ++++++++++++++++++++--
 drivers/power/twl4030_madc_battery.c |  7 +++++--
 3 files changed, 31 insertions(+), 5 deletions(-)

-- 
1.9.1

[RFT PATCH 1/4] power_supply: twl4030_madc: Check return value of power_supply_register

[Krzysztof Kozlowski]

The return value of power_supply_register() call was not checked and
even on error probe() function returned 0. If registering failed then
during unbind the driver tried to unregister power supply which was not
actually registered.

This could lead to memory corruption because power_supply_unregister()
unconditionally cleans up given power supply.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Fixes: da0a00ebc239 ("power: Add twl4030_madc battery driver.")
Cc: <stable@vger.kernel.org>
---
 drivers/power/twl4030_madc_battery.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/power/twl4030_madc_battery.c b/drivers/power/twl4030_madc_battery.c
index 7ef445a6cfa6..cf907609ec49 100644
--- a/drivers/power/twl4030_madc_battery.c
+++ b/drivers/power/twl4030_madc_battery.c
@@ -192,6 +192,7 @@ static int twl4030_madc_battery_probe(struct platform_device *pdev)
 {
 	struct twl4030_madc_battery *twl4030_madc_bat;
 	struct twl4030_madc_bat_platform_data *pdata = pdev->dev.platform_data;
+	int ret = 0;
 
 	twl4030_madc_bat = kzalloc(sizeof(*twl4030_madc_bat), GFP_KERNEL);
 	if (!twl4030_madc_bat)
@@ -216,9 +217,11 @@ static int twl4030_madc_battery_probe(struct platform_device *pdev)
 
 	twl4030_madc_bat->pdata = pdata;
 	platform_set_drvdata(pdev, twl4030_madc_bat);
-	power_supply_register(&pdev->dev, &twl4030_madc_bat->psy);
+	ret = power_supply_register(&pdev->dev, &twl4030_madc_bat->psy);
+	if (ret < 0)
+		kfree(twl4030_madc_bat);
 
-	return 0;
+	return ret;
 }
 
 static int twl4030_madc_battery_remove(struct platform_device *pdev)
-- 
1.9.1

[RFT PATCH 2/4] compal-laptop: Check return value of power_supply_register

[Krzysztof Kozlowski]

The return value of power_supply_register() call was not checked and
even on error probe() function returned 0. If registering failed then
during unbind the driver tried to unregister power supply which was not
actually registered.

This could lead to memory corruption because power_supply_unregister()
unconditionally cleans up given power supply.

Fix this by checking return status of power_supply_register() call. In
case of failure, unregister the hwmon device and fail the probe. Add a
fixme note about missing hwmon_device_unregister() in driver removal.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Fixes: 9be0fcb5ed46 ("compal-laptop: add JHL90, battery & hwmon interface")
Cc: <stable@vger.kernel.org>
---
 drivers/platform/x86/compal-laptop.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/compal-laptop.c b/drivers/platform/x86/compal-laptop.c
index 15c0fab2bfa1..cf55a9246f12 100644
--- a/drivers/platform/x86/compal-laptop.c
+++ b/drivers/platform/x86/compal-laptop.c
@@ -1036,12 +1036,16 @@ static int compal_probe(struct platform_device *pdev)
 
 	/* Power supply */
 	initialize_power_supply_data(data);
-	power_supply_register(&compal_device->dev, &data->psy);
+	err = power_supply_register(&compal_device->dev, &data->psy);
+	if (err < 0)
+		goto psy_err;
 
 	platform_set_drvdata(pdev, data);
 
 	return 0;
 
+psy_err:
+	hwmon_device_unregister(hwmon_dev);
 remove:
 	sysfs_remove_group(&pdev->dev.kobj, &compal_platform_attr_group);
 	return err;
@@ -1072,6 +1076,7 @@ static int compal_remove(struct platform_device *pdev)
 
 	data = platform_get_drvdata(pdev);
 	power_supply_unregister(&data->psy);
+	/* FIXME: missing hwmon_device_unregister() */
 
 	sysfs_remove_group(&pdev->dev.kobj, &compal_platform_attr_group);
 
-- 
1.9.1

[RFT PATCH 3/4] power_supply: ipaq_micro_battery: Fix leaking workqueue

[Krzysztof Kozlowski]

Driver allocates singlethread workqueue in probe but it is not destroyed
during removal.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Fixes: 00a588f9d27f ("power: add driver for battery reading on iPaq h3xxx")
Cc: <stable@vger.kernel.org>
---
 drivers/power/ipaq_micro_battery.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/power/ipaq_micro_battery.c b/drivers/power/ipaq_micro_battery.c
index 9d694605cdb7..698cf1636bb8 100644
--- a/drivers/power/ipaq_micro_battery.c
+++ b/drivers/power/ipaq_micro_battery.c
@@ -251,6 +251,7 @@ static int micro_batt_remove(struct platform_device *pdev)
 	power_supply_unregister(&micro_ac_power);
 	power_supply_unregister(&micro_batt_power);
 	cancel_delayed_work_sync(&mb->update);
+	destroy_workqueue(mb->wq);
 
 	return 0;
 }
-- 
1.9.1

[RFT PATCH 4/4] power_supply: ipaq_micro_battery: Check return values in probe

[Krzysztof Kozlowski]

The return values of create_singlethread_workqueue() and
power_supply_register() calls were not checked and even on error probe()
function returned 0.

1. If allocation of workqueue failed (returning NULL) then further
   accesses could lead to NULL pointer dereference. The
   queue_delayed_work() expects workqueue to be non-NULL.

2. If registration of power supply failed then during unbind the driver
   tried to unregister power supply which was not actually registered.
   This could lead to memory corruption because
   power_supply_unregister() unconditionally cleans up given power
   supply.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Fixes: 00a588f9d27f ("power: add driver for battery reading on iPaq h3xxx")
Cc: <stable@vger.kernel.org>
---
 drivers/power/ipaq_micro_battery.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/power/ipaq_micro_battery.c b/drivers/power/ipaq_micro_battery.c
index 698cf1636bb8..96b15e003f3f 100644
--- a/drivers/power/ipaq_micro_battery.c
+++ b/drivers/power/ipaq_micro_battery.c
@@ -226,6 +226,7 @@ static struct power_supply micro_ac_power = {
 static int micro_batt_probe(struct platform_device *pdev)
 {
 	struct micro_battery *mb;
+	int ret;
 
 	mb = devm_kzalloc(&pdev->dev, sizeof(*mb), GFP_KERNEL);
 	if (!mb)
@@ -233,14 +234,30 @@ static int micro_batt_probe(struct platform_device *pdev)
 
 	mb->micro = dev_get_drvdata(pdev->dev.parent);
 	mb->wq = create_singlethread_workqueue("ipaq-battery-wq");
+	if (!mb->wq)
+		return -ENOMEM;
+
 	INIT_DELAYED_WORK(&mb->update, micro_battery_work);
 	platform_set_drvdata(pdev, mb);
 	queue_delayed_work(mb->wq, &mb->update, 1);
-	power_supply_register(&pdev->dev, &micro_batt_power);
-	power_supply_register(&pdev->dev, &micro_ac_power);
+
+	ret = power_supply_register(&pdev->dev, &micro_batt_power);
+	if (ret < 0)
+		goto batt_err;
+
+	ret = power_supply_register(&pdev->dev, &micro_ac_power);
+	if (ret < 0)
+		goto ac_err;
 
 	dev_info(&pdev->dev, "iPAQ micro battery driver\n");
 	return 0;
+
+ac_err:
+	power_supply_unregister(&micro_ac_power);
+batt_err:
+	cancel_delayed_work_sync(&mb->update);
+	destroy_workqueue(mb->wq);
+	return ret;
 }
 
 static int micro_batt_remove(struct platform_device *pdev)
-- 
1.9.1

Re: [RFT PATCH 1/4] power_supply: twl4030_madc: Check return value of power_supply_register

[Belisko Marek]

Hi Krzystof,

On Tue, Jan 27, 2015 at 12:30 PM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> The return value of power_supply_register() call was not checked and
> even on error probe() function returned 0. If registering failed then
> during unbind the driver tried to unregister power supply which was not
> actually registered.
>
> This could lead to memory corruption because power_supply_unregister()
> unconditionally cleans up given power supply.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Fixes: da0a00ebc239 ("power: Add twl4030_madc battery driver.")
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/power/twl4030_madc_battery.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/power/twl4030_madc_battery.c b/drivers/power/twl4030_madc_battery.c
> index 7ef445a6cfa6..cf907609ec49 100644
> --- a/drivers/power/twl4030_madc_battery.c
> +++ b/drivers/power/twl4030_madc_battery.c
> @@ -192,6 +192,7 @@ static int twl4030_madc_battery_probe(struct platform_device *pdev)
>  {
>         struct twl4030_madc_battery *twl4030_madc_bat;
>         struct twl4030_madc_bat_platform_data *pdata = pdev->dev.platform_data;
> +       int ret = 0;
>
>         twl4030_madc_bat = kzalloc(sizeof(*twl4030_madc_bat), GFP_KERNEL);
>         if (!twl4030_madc_bat)
> @@ -216,9 +217,11 @@ static int twl4030_madc_battery_probe(struct platform_device *pdev)
>
>         twl4030_madc_bat->pdata = pdata;
>         platform_set_drvdata(pdev, twl4030_madc_bat);
> -       power_supply_register(&pdev->dev, &twl4030_madc_bat->psy);
> +       ret = power_supply_register(&pdev->dev, &twl4030_madc_bat->psy);
> +       if (ret < 0)
> +               kfree(twl4030_madc_bat);
I post update twl4030_madc to iio some time ago [1] where this change
was incorporated.
I plan to respin in next week so we can drop this one (or take and
I'll rebase on top).
>
> -       return 0;
> +       return ret;
>  }
>
>  static int twl4030_madc_battery_remove(struct platform_device *pdev)
> --
> 1.9.1
>

[1] - https://lkml.org/lkml/2014/3/5/284

BR,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com

Re: [RFT PATCH 1/4] power_supply: twl4030_madc: Check return value of power_supply_register

[Krzysztof Kozlowski]

On wto, 2015-01-27 at 22:06 +0100, Belisko Marek wrote:
> Hi Krzystof,
> 
> On Tue, Jan 27, 2015 at 12:30 PM, Krzysztof Kozlowski
> <k.kozlowski@samsung.com> wrote:
> > The return value of power_supply_register() call was not checked and
> > even on error probe() function returned 0. If registering failed then
> > during unbind the driver tried to unregister power supply which was not
> > actually registered.
> >
> > This could lead to memory corruption because power_supply_unregister()
> > unconditionally cleans up given power supply.
> >
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > Fixes: da0a00ebc239 ("power: Add twl4030_madc battery driver.")
> > Cc: <stable@vger.kernel.org>
> > ---
> >  drivers/power/twl4030_madc_battery.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/power/twl4030_madc_battery.c b/drivers/power/twl4030_madc_battery.c
> > index 7ef445a6cfa6..cf907609ec49 100644
> > --- a/drivers/power/twl4030_madc_battery.c
> > +++ b/drivers/power/twl4030_madc_battery.c
> > @@ -192,6 +192,7 @@ static int twl4030_madc_battery_probe(struct platform_device *pdev)
> >  {
> >         struct twl4030_madc_battery *twl4030_madc_bat;
> >         struct twl4030_madc_bat_platform_data *pdata = pdev->dev.platform_data;
> > +       int ret = 0;
> >
> >         twl4030_madc_bat = kzalloc(sizeof(*twl4030_madc_bat), GFP_KERNEL);
> >         if (!twl4030_madc_bat)
> > @@ -216,9 +217,11 @@ static int twl4030_madc_battery_probe(struct platform_device *pdev)
> >
> >         twl4030_madc_bat->pdata = pdata;
> >         platform_set_drvdata(pdev, twl4030_madc_bat);
> > -       power_supply_register(&pdev->dev, &twl4030_madc_bat->psy);
> > +       ret = power_supply_register(&pdev->dev, &twl4030_madc_bat->psy);
> > +       if (ret < 0)
> > +               kfree(twl4030_madc_bat);
> I post update twl4030_madc to iio some time ago [1] where this change
> was incorporated.
> I plan to respin in next week so we can drop this one (or take and
> I'll rebase on top).

Hi,

I am fine with both solutions (especially that I am not able to test my
change). However there is one benefit of my fix: after applying it could
be backported to stable kernels (3.11+?).

Best regards,
Krzysztof

> >
> > -       return 0;
> > +       return ret;
> >  }
> >
> >  static int twl4030_madc_battery_remove(struct platform_device *pdev)
> > --
> > 1.9.1
> >
> 
> [1] - https://lkml.org/lkml/2014/3/5/284
> 
> BR,
> 
> marek
> 

Re: [RFT PATCH 2/4] compal-laptop: Check return value of power_supply_register

[Darren Hart]

On Tue, Jan 27, 2015 at 12:30:19PM +0100, Krzysztof Kozlowski wrote:
> The return value of power_supply_register() call was not checked and
> even on error probe() function returned 0. If registering failed then
> during unbind the driver tried to unregister power supply which was not
> actually registered.
> 
> This could lead to memory corruption because power_supply_unregister()
> unconditionally cleans up given power supply.
> 
> Fix this by checking return status of power_supply_register() call. In
> case of failure, unregister the hwmon device and fail the probe. Add a
> fixme note about missing hwmon_device_unregister() in driver removal.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Fixes: 9be0fcb5ed46 ("compal-laptop: add JHL90, battery & hwmon interface")
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/platform/x86/compal-laptop.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/compal-laptop.c b/drivers/platform/x86/compal-laptop.c
> index 15c0fab2bfa1..cf55a9246f12 100644
> --- a/drivers/platform/x86/compal-laptop.c
> +++ b/drivers/platform/x86/compal-laptop.c
> @@ -1036,12 +1036,16 @@ static int compal_probe(struct platform_device *pdev)
>  
>  	/* Power supply */
>  	initialize_power_supply_data(data);
> -	power_supply_register(&compal_device->dev, &data->psy);
> +	err = power_supply_register(&compal_device->dev, &data->psy);
> +	if (err < 0)
> +		goto psy_err;
>  
>  	platform_set_drvdata(pdev, data);
>  
>  	return 0;
>  
> +psy_err:
> +	hwmon_device_unregister(hwmon_dev);
>  remove:
>  	sysfs_remove_group(&pdev->dev.kobj, &compal_platform_attr_group);
>  	return err;
> @@ -1072,6 +1076,7 @@ static int compal_remove(struct platform_device *pdev)
>  
>  	data = platform_get_drvdata(pdev);
>  	power_supply_unregister(&data->psy);
> +	/* FIXME: missing hwmon_device_unregister() */

Is this FIXME a leftover? Is there a reason we can't fix this now instead of
adding a FIXME?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [RFT PATCH 2/4] compal-laptop: Check return value of power_supply_register

[Krzysztof Kozlowski]

On pią, 2015-02-06 at 18:42 -0800, Darren Hart wrote:
> On Tue, Jan 27, 2015 at 12:30:19PM +0100, Krzysztof Kozlowski wrote:
> > The return value of power_supply_register() call was not checked and
> > even on error probe() function returned 0. If registering failed then
> > during unbind the driver tried to unregister power supply which was not
> > actually registered.
> > 
> > This could lead to memory corruption because power_supply_unregister()
> > unconditionally cleans up given power supply.
> > 
> > Fix this by checking return status of power_supply_register() call. In
> > case of failure, unregister the hwmon device and fail the probe. Add a
> > fixme note about missing hwmon_device_unregister() in driver removal.
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > Fixes: 9be0fcb5ed46 ("compal-laptop: add JHL90, battery & hwmon interface")
> > Cc: <stable@vger.kernel.org>
> > ---
> >  drivers/platform/x86/compal-laptop.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/platform/x86/compal-laptop.c b/drivers/platform/x86/compal-laptop.c
> > index 15c0fab2bfa1..cf55a9246f12 100644
> > --- a/drivers/platform/x86/compal-laptop.c
> > +++ b/drivers/platform/x86/compal-laptop.c
> > @@ -1036,12 +1036,16 @@ static int compal_probe(struct platform_device *pdev)
> >  
> >  	/* Power supply */
> >  	initialize_power_supply_data(data);
> > -	power_supply_register(&compal_device->dev, &data->psy);
> > +	err = power_supply_register(&compal_device->dev, &data->psy);
> > +	if (err < 0)
> > +		goto psy_err;
> >  
> >  	platform_set_drvdata(pdev, data);
> >  
> >  	return 0;
> >  
> > +psy_err:
> > +	hwmon_device_unregister(hwmon_dev);
> >  remove:
> >  	sysfs_remove_group(&pdev->dev.kobj, &compal_platform_attr_group);
> >  	return err;
> > @@ -1072,6 +1076,7 @@ static int compal_remove(struct platform_device *pdev)
> >  
> >  	data = platform_get_drvdata(pdev);
> >  	power_supply_unregister(&data->psy);
> > +	/* FIXME: missing hwmon_device_unregister() */
> 
> Is this FIXME a leftover? Is there a reason we can't fix this now instead of
> adding a FIXME?

This is not a leftover. I think this should be fixed but:
1. I cannot test this driver,
2. I am not such familiar with hwmon API,
so my fix could be wrong and introduce more errors than fixes.

If you also think hwmon_device_unregister() is needed then I could send
new version of patch. Also I would be happy if someone else fixed this.

Best regards,
Krzysztof