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

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