[PATCH] HID: hid-input: Fix accessing freed memory during driver unbind

During unbinding the driver was dereferencing a pointer to memory
already freed by power_supply_unregister().

Driver was freeing its internal description of battery through pointers
stored in power_supply structure. However, because the core owns the
power supply instance, after calling power_supply_unregister() the
driver cannot access these members.

Fix this by using resource-managed allocations so internal data will be
freed by pointers stored in resource-managed core.

Signed-off-by: Krzysztof Kozlowski <redacted>
Reported-by: H.J. Lu <redacted>
Fixes: 297d716f6260 ("power_supply: Change ownership from driver to core")
Cc: <redacted>
---
 drivers/hid/hid-input.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 14aebe483219..5429a8497d51 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c

@@ -408,15 +408,14 @@ static bool hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
 	if (dev->battery != NULL)
 		goto out;	/* already initialized? */
 
-	psy_desc = kzalloc(sizeof(*psy_desc), GFP_KERNEL);
+	psy_desc = devm_kzalloc(&dev->dev, sizeof(*psy_desc), GFP_KERNEL);
 	if (psy_desc == NULL)
 		goto out;
 
-	psy_desc->name = kasprintf(GFP_KERNEL, "hid-%s-battery", dev->uniq);
-	if (psy_desc->name == NULL) {
-		kfree(psy_desc);
+	psy_desc->name = devm_kasprintf(&dev->dev, GFP_KERNEL,
+					"hid-%s-battery", dev->uniq);
+	if (psy_desc->name == NULL)
 		goto out;
-	}
 
 	psy_desc->type = POWER_SUPPLY_TYPE_BATTERY;
 	psy_desc->properties = hidinput_battery_props;

@@ -449,8 +448,6 @@ static bool hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
 	if (IS_ERR(dev->battery)) {
 		hid_warn(dev, "can't register power supply: %ld\n",
 				PTR_ERR(dev->battery));
-		kfree(psy_desc->name);
-		kfree(psy_desc);
 		dev->battery = NULL;
 	} else {
 		power_supply_powers(dev->battery, &dev->dev);

@@ -466,8 +463,6 @@ static void hidinput_cleanup_battery(struct hid_device *dev)
 		return;
 
 	power_supply_unregister(dev->battery);
-	kfree(dev->battery->desc->name);
-	kfree(dev->battery->desc);
 	dev->battery = NULL;
 }
 #else  /* !CONFIG_HID_BATTERY_STRENGTH */

-- 
1.9.1

On Tue, 28 Jul 2015, Krzysztof Kozlowski wrote:

During unbinding the driver was dereferencing a pointer to memory
already freed by power_supply_unregister().

Driver was freeing its internal description of battery through pointers
stored in power_supply structure. However, because the core owns the
power supply instance, after calling power_supply_unregister() the
driver cannot access these members.

Fix this by using resource-managed allocations so internal data will be
freed by pointers stored in resource-managed core.

Signed-off-by: Krzysztof Kozlowski <redacted>
Reported-by: H.J. Lu <redacted>
Fixes: 297d716f6260 ("power_supply: Change ownership from driver to core")
Cc: <redacted>

Applied to for-4.2/upstream-fixes, thanks.

-- 
Jiri Kosina
SUSE Labs

On Wed, Jul 29, 2015 at 03:07:04PM +0200, Jiri Kosina wrote:

On Tue, 28 Jul 2015, Krzysztof Kozlowski wrote:

quoted

During unbinding the driver was dereferencing a pointer to memory
already freed by power_supply_unregister().

Driver was freeing its internal description of battery through pointers
stored in power_supply structure. However, because the core owns the
power supply instance, after calling power_supply_unregister() the
driver cannot access these members.

Fix this by using resource-managed allocations so internal data will be
freed by pointers stored in resource-managed core.

Signed-off-by: Krzysztof Kozlowski <redacted>
Reported-by: H.J. Lu <redacted>
Fixes: 297d716f6260 ("power_supply: Change ownership from driver to core")
Cc: <redacted>

Applied to for-4.2/upstream-fixes, thanks.

Wait, what guarantees do we have that this is only called in probe()
paths? Don't we allow hid_hw_start() be deferred to open() calls?

In general we need to be careful with devm* conversions in core code.

Thanks.

-- 
Dmitry

2015-07-30 2:46 GMT+09:00 Dmitry Torokhov [off-list ref]:

On Wed, Jul 29, 2015 at 03:07:04PM +0200, Jiri Kosina wrote:

quoted

On Tue, 28 Jul 2015, Krzysztof Kozlowski wrote:

quoted

During unbinding the driver was dereferencing a pointer to memory
already freed by power_supply_unregister().

Driver was freeing its internal description of battery through pointers
stored in power_supply structure. However, because the core owns the
power supply instance, after calling power_supply_unregister() the
driver cannot access these members.

Fix this by using resource-managed allocations so internal data will be
freed by pointers stored in resource-managed core.

Signed-off-by: Krzysztof Kozlowski <redacted>
Reported-by: H.J. Lu <redacted>
Fixes: 297d716f6260 ("power_supply: Change ownership from driver to core")
Cc: <redacted>

Applied to for-4.2/upstream-fixes, thanks.

Wait, what guarantees do we have that this is only called in probe()
paths? Don't we allow hid_hw_start() be deferred to open() calls?

Indeed, this may be called in other contexts. But this should not
introduce errors except not reclaimable memory (till remove()
happens).

In general we need to be careful with devm* conversions in core code.

Another and less intrusive fix would be:

    char *name = dev->battery->desc->name;
    struct power_supply_desc *psy_desc = dev->battery->desc;
    power_supply_unregister(dev->battery);
    kfree(name);
    kfree(psy_desc);

How about this?

Best regards,
Krzysztof

On Thu, Jul 30, 2015 at 08:42:12AM +0900, Krzysztof Kozlowski wrote:

2015-07-30 2:46 GMT+09:00 Dmitry Torokhov [off-list ref]:

quoted

On Wed, Jul 29, 2015 at 03:07:04PM +0200, Jiri Kosina wrote:

quoted

On Tue, 28 Jul 2015, Krzysztof Kozlowski wrote:

quoted

During unbinding the driver was dereferencing a pointer to memory
already freed by power_supply_unregister().

Driver was freeing its internal description of battery through pointers
stored in power_supply structure. However, because the core owns the
power supply instance, after calling power_supply_unregister() the
driver cannot access these members.

Fix this by using resource-managed allocations so internal data will be
freed by pointers stored in resource-managed core.

Signed-off-by: Krzysztof Kozlowski <redacted>
Reported-by: H.J. Lu <redacted>
Fixes: 297d716f6260 ("power_supply: Change ownership from driver to core")
Cc: <redacted>

Applied to for-4.2/upstream-fixes, thanks.

Wait, what guarantees do we have that this is only called in probe()
paths? Don't we allow hid_hw_start() be deferred to open() calls?

Indeed, this may be called in other contexts. But this should not
introduce errors except not reclaimable memory (till remove()
happens).

quoted

In general we need to be careful with devm* conversions in core code.

Another and less intrusive fix would be:

    char *name = dev->battery->desc->name;
    struct power_supply_desc *psy_desc = dev->battery->desc;
    power_supply_unregister(dev->battery);
    kfree(name);
    kfree(psy_desc);

I would much rather prefer this to the other version as it does not
leave memory hanging around, potentially indefinitely, but ultimately it
is up to Jiri. I only hope that power supply code does not reference
power_supply_desc pointer past unregister (since the device structure
itself may live past the point where power_supply_unregister() returns).

By the way, you do not need name temp, you can do

	kfree(psy_desc->name);
	kfree(psy_desc);

Thanks.

-- 
Dmitry

On Wed, 29 Jul 2015, Dmitry Torokhov wrote:

quoted

Another and less intrusive fix would be:

    char *name = dev->battery->desc->name;
    struct power_supply_desc *psy_desc = dev->battery->desc;
    power_supply_unregister(dev->battery);
    kfree(name);
    kfree(psy_desc);

I would much rather prefer this to the other version as it does not
leave memory hanging around, potentially indefinitely, but ultimately it
is up to Jiri. 

I must have been in some broken state of mind when applying the original 
one, thanks a lot for catching my brainfart, Dmitry!

Kryzstof, could you please send me properly formatted patch with the 
above, on top of your previous fix?

Thanks.

-- 
Jiri Kosina
SUSE Labs

2015-08-01 21:11 GMT+09:00 Jiri Kosina [off-list ref]:

On Wed, 29 Jul 2015, Dmitry Torokhov wrote:

quoted

quoted

Another and less intrusive fix would be:

    char *name = dev->battery->desc->name;
    struct power_supply_desc *psy_desc = dev->battery->desc;
    power_supply_unregister(dev->battery);
    kfree(name);
    kfree(psy_desc);

I would much rather prefer this to the other version as it does not
leave memory hanging around, potentially indefinitely, but ultimately it
is up to Jiri.

I must have been in some broken state of mind when applying the original
one, thanks a lot for catching my brainfart, Dmitry!

Kryzstof, could you please send me properly formatted patch with the
above, on top of your previous fix?

Of course, I'll send next version.

Best regards,
Krzysztof