| Message ID | 1438042590-17719-1-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
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 <k.kozlowski@samsung.com> > Reported-by: H.J. Lu <hjl.tools@gmail.com> > Fixes: 297d716f6260 ("power_supply: Change ownership from driver to core") > Cc: <stable@vger.kernel.org> Applied to for-4.2/upstream-fixes, thanks.
On Wed, Jul 29, 2015 at 03:07:04PM +0200, Jiri Kosina wrote: > 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 <k.kozlowski@samsung.com> > > Reported-by: H.J. Lu <hjl.tools@gmail.com> > > Fixes: 297d716f6260 ("power_supply: Change ownership from driver to core") > > Cc: <stable@vger.kernel.org> > > 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.
2015-07-30 2:46 GMT+09:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>: > On Wed, Jul 29, 2015 at 03:07:04PM +0200, Jiri Kosina wrote: >> 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 <k.kozlowski@samsung.com> >> > Reported-by: H.J. Lu <hjl.tools@gmail.com> >> > Fixes: 297d716f6260 ("power_supply: Change ownership from driver to core") >> > Cc: <stable@vger.kernel.org> >> >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 30, 2015 at 08:42:12AM +0900, Krzysztof Kozlowski wrote: > 2015-07-30 2:46 GMT+09:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>: > > On Wed, Jul 29, 2015 at 03:07:04PM +0200, Jiri Kosina wrote: > >> 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 <k.kozlowski@samsung.com> > >> > Reported-by: H.J. Lu <hjl.tools@gmail.com> > >> > Fixes: 297d716f6260 ("power_supply: Change ownership from driver to core") > >> > Cc: <stable@vger.kernel.org> > >> > >> 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); 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.
On Wed, 29 Jul 2015, Dmitry Torokhov wrote: > > 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.
2015-08-01 21:11 GMT+09:00 Jiri Kosina <jkosina@suse.com>: > On Wed, 29 Jul 2015, Dmitry Torokhov wrote: > >> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */
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 <k.kozlowski@samsung.com> Reported-by: H.J. Lu <hjl.tools@gmail.com> Fixes: 297d716f6260 ("power_supply: Change ownership from driver to core") Cc: <stable@vger.kernel.org> --- drivers/hid/hid-input.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)