| Message ID | 1369817109-4277-1-git-send-email-benjamin.tissoires@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
On Wed, May 29, 2013 at 11:45 AM, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > mt_free_input_name() was never called during .remove(): > hid_hw_stop() removes the hid_input items in hdev->inputs, and so the > list is therefore empty after the call. In the end, we never free the > special names that has been allocated during .probe(). > > Restore the original name before freeing it to avoid acessing already > freed pointer. > > I just spotted this one yesterday... My guess is that this way is safe (without > a locking mechanism to prevent accessing hi->input->name), but I'm not 100% sure. Hi Jiri, Benjamin. What do you think about patch I just sent? P.S. Benjamin, I re-used your commit message, I think you have no objections. -- With Best Regards, Andy Shevchenko -- 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
Hi Andy, On Wed, May 29, 2013 at 10:12 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, May 29, 2013 at 11:45 AM, Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: >> mt_free_input_name() was never called during .remove(): >> hid_hw_stop() removes the hid_input items in hdev->inputs, and so the >> list is therefore empty after the call. In the end, we never free the >> special names that has been allocated during .probe(). >> >> Restore the original name before freeing it to avoid acessing already >> freed pointer. >> > >> I just spotted this one yesterday... My guess is that this way is safe (without >> a locking mechanism to prevent accessing hi->input->name), but I'm not 100% sure. > > Hi Jiri, Benjamin. > > What do you think about patch I just sent? Thanks for looking at this. My very first concern is that none of the HID part has been devm-ized (this is on my todo-if-I-have-some-time list). The input susbsystem has been devm-ized quite recently, so this should be possible now. So, basically, I honestly don't know if using part of devm mallocs in hid-multitouch is safe and if it will work as expected. Maybe Jiri will have a better idea. I have a few comments on your patch if Jiri wants to include it. > > P.S. Benjamin, I re-used your commit message, I think you have no objections. No objections :) Cheers, Benjamin -- 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 Wed, 29 May 2013, Benjamin Tissoires wrote: > mt_free_input_name() was never called during .remove(): > hid_hw_stop() removes the hid_input items in hdev->inputs, and so the > list is therefore empty after the call. In the end, we never free the > special names that has been allocated during .probe(). > > Restore the original name before freeing it to avoid acessing already > freed pointer. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> I have now applied this one and will send it to Linus for next -rc. If we are going down the path of using devm API, as proposed by Andy, that will require much more throgough review of interaction with input subsystem, so definitely not a late -rc regression fix material. Thanks,
On Wed, Jun 12, 2013 at 12:15 PM, Jiri Kosina <jkosina@suse.cz> wrote: [] > If we are going down the path of using devm API, as proposed by Andy, that > will require much more throgough review of interaction with input > subsystem, so definitely not a late -rc regression fix material. Agree. I will prepare later better approach with devm_* usage. -- With Best Regards, Andy Shevchenko -- 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-multitouch.c b/drivers/hid/hid-multitouch.c index d99b959..cb0e361 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -264,9 +264,12 @@ static struct mt_class mt_classes[] = { static void mt_free_input_name(struct hid_input *hi) { struct hid_device *hdev = hi->report->device; + const char *name = hi->input->name; - if (hi->input->name != hdev->name) - kfree(hi->input->name); + if (name != hdev->name) { + hi->input->name = hdev->name; + kfree(name); + } } static ssize_t mt_show_quirks(struct device *dev, @@ -1040,11 +1043,11 @@ static void mt_remove(struct hid_device *hdev) struct hid_input *hi; sysfs_remove_group(&hdev->dev.kobj, &mt_attribute_group); - hid_hw_stop(hdev); - list_for_each_entry(hi, &hdev->inputs, list) mt_free_input_name(hi); + hid_hw_stop(hdev); + kfree(td); hid_set_drvdata(hdev, NULL); }
mt_free_input_name() was never called during .remove(): hid_hw_stop() removes the hid_input items in hdev->inputs, and so the list is therefore empty after the call. In the end, we never free the special names that has been allocated during .probe(). Restore the original name before freeing it to avoid acessing already freed pointer. Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> --- Hi Jiri, I just spotted this one yesterday... My guess is that this way is safe (without a locking mechanism to prevent accessing hi->input->name), but I'm not 100% sure. Cheers, Benjamin drivers/hid/hid-multitouch.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)