[PATCH] HID: multitouch: prevent memleak with the allocated name

[PATCH] HID: multitouch: prevent memleak with the allocated name

[Benjamin Tissoires]

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(-)

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);
 }
-- 
1.8.2.1

[PATCH] HID: multitouch: prevent memleak with the allocated name

[Andy Shevchenko]

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().

We switch to devm_kzalloc that manages resources when driver is removed.

Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reported-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-multitouch.c |   37 +++++++++++++------------------------
 1 files changed, 13 insertions(+), 24 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index d99b959..1f5876e 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -261,14 +261,6 @@ static struct mt_class mt_classes[] = {
 	{ }
 };
 
-static void mt_free_input_name(struct hid_input *hi)
-{
-	struct hid_device *hdev = hi->report->device;
-
-	if (hi->input->name != hdev->name)
-		kfree(hi->input->name);
-}
-
 static ssize_t mt_show_quirks(struct device *dev,
 			   struct device_attribute *attr,
 			   char *buf)
@@ -412,10 +404,12 @@ static void mt_pen_report(struct hid_device *hid, struct hid_report *report)
 static void mt_pen_input_configured(struct hid_device *hdev,
 					struct hid_input *hi)
 {
-	char *name = kzalloc(strlen(hi->input->name) + 5, GFP_KERNEL);
-	if (name) {
-		sprintf(name, "%s Pen", hi->input->name);
-		mt_free_input_name(hi);
+	char *name;
+
+	if (hdev->name) {
+		name = devm_kzalloc(&hdev->dev, strlen(hdev->name) + 5,
+							GFP_KERNEL);
+		sprintf(name, "%s Pen", hdev->name);
 		hi->input->name = name;
 	}
 
@@ -925,16 +919,18 @@ static void mt_post_parse(struct mt_device *td)
 static void mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
 {
 	struct mt_device *td = hid_get_drvdata(hdev);
-	char *name = kstrdup(hdev->name, GFP_KERNEL);
-
-	if (name)
-		hi->input->name = name;
 
 	if (hi->report->id == td->mt_report_id)
 		mt_touch_input_configured(hdev, hi);
 
 	if (hi->report->id == td->pen_report_id)
 		mt_pen_input_configured(hdev, hi);
+
+	if (!hi->input->name) {
+		hi->input->name = devm_kzalloc(&hdev->dev,
+					strlen(hdev->name) + 1, GFP_KERNEL);
+		strcpy(hi->input->name, hdev->name);
+	}
 }
 
 static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
@@ -993,7 +989,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 	if (ret)
-		goto hid_fail;
+		goto fail;
 
 	ret = sysfs_create_group(&hdev->dev.kobj, &mt_attribute_group);
 
@@ -1005,9 +1001,6 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	return 0;
 
-hid_fail:
-	list_for_each_entry(hi, &hdev->inputs, list)
-		mt_free_input_name(hi);
 fail:
 	kfree(td->fields);
 	kfree(td);
@@ -1037,14 +1030,10 @@ static int mt_resume(struct hid_device *hdev)
 static void mt_remove(struct hid_device *hdev)
 {
 	struct mt_device *td = hid_get_drvdata(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);
-
 	kfree(td);
 	hid_set_drvdata(hdev, NULL);
 }
-- 
1.7.7.6

Re: [PATCH] HID: multitouch: prevent memleak with the allocated name

[Benjamin Tissoires]

Hi Andy,

On Wed, May 29, 2013 at 10:12 PM, Andy Shevchenko
<andy.shevchenko@gmail.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().
>
> We switch to devm_kzalloc that manages resources when driver is removed.
>
> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reported-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/hid-multitouch.c |   37 +++++++++++++------------------------
>  1 files changed, 13 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index d99b959..1f5876e 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -261,14 +261,6 @@ static struct mt_class mt_classes[] = {
>         { }
>  };
>
> -static void mt_free_input_name(struct hid_input *hi)
> -{
> -       struct hid_device *hdev = hi->report->device;
> -
> -       if (hi->input->name != hdev->name)
> -               kfree(hi->input->name);
> -}
> -
>  static ssize_t mt_show_quirks(struct device *dev,
>                            struct device_attribute *attr,
>                            char *buf)
> @@ -412,10 +404,12 @@ static void mt_pen_report(struct hid_device *hid, struct hid_report *report)
>  static void mt_pen_input_configured(struct hid_device *hdev,
>                                         struct hid_input *hi)
>  {
> -       char *name = kzalloc(strlen(hi->input->name) + 5, GFP_KERNEL);
> -       if (name) {
> -               sprintf(name, "%s Pen", hi->input->name);
> -               mt_free_input_name(hi);
> +       char *name;
> +
> +       if (hdev->name) {

hdev->name is always not null, so no need to check this (hint: it
contains hid->name when allocated).

> +               name = devm_kzalloc(&hdev->dev, strlen(hdev->name) + 5,
> +                                                       GFP_KERNEL);

Does devm_kzalloc always return a valid pointer? If not, you should
just use devm_kzalloc instead of kzalloc and keep the old ordering of
allocation, test, and snprintf.

> +               sprintf(name, "%s Pen", hdev->name);
>                 hi->input->name = name;
>         }
>
> @@ -925,16 +919,18 @@ static void mt_post_parse(struct mt_device *td)
>  static void mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
>  {
>         struct mt_device *td = hid_get_drvdata(hdev);
> -       char *name = kstrdup(hdev->name, GFP_KERNEL);
> -
> -       if (name)
> -               hi->input->name = name;
>
>         if (hi->report->id == td->mt_report_id)
>                 mt_touch_input_configured(hdev, hi);
>
>         if (hi->report->id == td->pen_report_id)
>                 mt_pen_input_configured(hdev, hi);
> +
> +       if (!hi->input->name) {

will never happen, so can be dropped.

> +               hi->input->name = devm_kzalloc(&hdev->dev,
> +                                       strlen(hdev->name) + 1, GFP_KERNEL);
> +               strcpy(hi->input->name, hdev->name);
> +       }
>  }
>
>  static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
> @@ -993,7 +989,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>
>         ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>         if (ret)
> -               goto hid_fail;
> +               goto fail;
>
>         ret = sysfs_create_group(&hdev->dev.kobj, &mt_attribute_group);
>
> @@ -1005,9 +1001,6 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>
>         return 0;
>
> -hid_fail:
> -       list_for_each_entry(hi, &hdev->inputs, list)
> -               mt_free_input_name(hi);
>  fail:
>         kfree(td->fields);
>         kfree(td);
> @@ -1037,14 +1030,10 @@ static int mt_resume(struct hid_device *hdev)
>  static void mt_remove(struct hid_device *hdev)
>  {
>         struct mt_device *td = hid_get_drvdata(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);
> -
>         kfree(td);
>         hid_set_drvdata(hdev, NULL);
>  }
> --
> 1.7.7.6
>

Cheers,
Benjamin

Re: [PATCH] HID: multitouch: prevent memleak with the allocated name

[Andy Shevchenko]

On Thu, May 30, 2013 at 4:28 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Wed, May 29, 2013 at 10:12 PM, Andy Shevchenko
> <andy.shevchenko@gmail.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().
>>
>> We switch to devm_kzalloc that manages resources when driver is removed.

Thanks for review. See my answers below.

>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c

>> @@ -412,10 +404,12 @@ static void mt_pen_report(struct hid_device *hid, struct hid_report *report)
>>  static void mt_pen_input_configured(struct hid_device *hdev,
>>                                         struct hid_input *hi)
>>  {
>> -       char *name = kzalloc(strlen(hi->input->name) + 5, GFP_KERNEL);
>> -       if (name) {
>> -               sprintf(name, "%s Pen", hi->input->name);
>> -               mt_free_input_name(hi);
>> +       char *name;
>> +
>> +       if (hdev->name) {
>
> hdev->name is always not null, so no need to check this (hint: it
> contains hid->name when allocated).

Okay, I'll fix it.

>> +               name = devm_kzalloc(&hdev->dev, strlen(hdev->name) + 5,
>> +                                                       GFP_KERNEL);
>
> Does devm_kzalloc always return a valid pointer? If not, you should
> just use devm_kzalloc instead of kzalloc and keep the old ordering of
> allocation, test, and snprintf.

Good point, will fix.

>> @@ -925,16 +919,18 @@ static void mt_post_parse(struct mt_device *td)
>>  static void mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
>>  {
>>         struct mt_device *td = hid_get_drvdata(hdev);
>> -       char *name = kstrdup(hdev->name, GFP_KERNEL);
>> -
>> -       if (name)
>> -               hi->input->name = name;
>>
>>         if (hi->report->id == td->mt_report_id)
>>                 mt_touch_input_configured(hdev, hi);
>>
>>         if (hi->report->id == td->pen_report_id)
>>                 mt_pen_input_configured(hdev, hi);
>> +
>> +       if (!hi->input->name) {
>
> will never happen, so can be dropped.

Why not? As far as I understood this logic the input->name is assigned
accordingly to what device is configured. Like input->name =
hdev->name, except for pen it equals to hdev->name + " Pen". The last
one is assigned in the mt_pen_input_configure. Otherwise input->name
is NULL. Am I correct?

--
With Best Regards,
Andy Shevchenko

Re: [PATCH] HID: multitouch: prevent memleak with the allocated name

[Benjamin Tissoires]

On Sat, Jun 1, 2013 at 1:33 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, May 30, 2013 at 4:28 PM, Benjamin Tissoires
> <benjamin.tissoires@gmail.com> wrote:
>> On Wed, May 29, 2013 at 10:12 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.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().
>>>
>>> We switch to devm_kzalloc that manages resources when driver is removed.
>
> Thanks for review. See my answers below.
>
>>> --- a/drivers/hid/hid-multitouch.c
>>> +++ b/drivers/hid/hid-multitouch.c
>
>>> @@ -412,10 +404,12 @@ static void mt_pen_report(struct hid_device *hid, struct hid_report *report)
>>>  static void mt_pen_input_configured(struct hid_device *hdev,
>>>                                         struct hid_input *hi)
>>>  {
>>> -       char *name = kzalloc(strlen(hi->input->name) + 5, GFP_KERNEL);
>>> -       if (name) {
>>> -               sprintf(name, "%s Pen", hi->input->name);
>>> -               mt_free_input_name(hi);
>>> +       char *name;
>>> +
>>> +       if (hdev->name) {
>>
>> hdev->name is always not null, so no need to check this (hint: it
>> contains hid->name when allocated).
>
> Okay, I'll fix it.

thanks. And sorry, I thought the test was against hi->input->name,
thus my "hint" which was exactly the same as what you tested.
Anyway, hdev->name is still never null, so the remark still applies.
>
>>> +               name = devm_kzalloc(&hdev->dev, strlen(hdev->name) + 5,
>>> +                                                       GFP_KERNEL);
>>
>> Does devm_kzalloc always return a valid pointer? If not, you should
>> just use devm_kzalloc instead of kzalloc and keep the old ordering of
>> allocation, test, and snprintf.
>
> Good point, will fix.
>
>>> @@ -925,16 +919,18 @@ static void mt_post_parse(struct mt_device *td)
>>>  static void mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
>>>  {
>>>         struct mt_device *td = hid_get_drvdata(hdev);
>>> -       char *name = kstrdup(hdev->name, GFP_KERNEL);
>>> -
>>> -       if (name)
>>> -               hi->input->name = name;
>>>
>>>         if (hi->report->id == td->mt_report_id)
>>>                 mt_touch_input_configured(hdev, hi);
>>>
>>>         if (hi->report->id == td->pen_report_id)
>>>                 mt_pen_input_configured(hdev, hi);
>>> +
>>> +       if (!hi->input->name) {
>>
>> will never happen, so can be dropped.
>
> Why not? As far as I understood this logic the input->name is assigned
> accordingly to what device is configured. Like input->name =
> hdev->name, except for pen it equals to hdev->name + " Pen". The last
> one is assigned in the mt_pen_input_configure. Otherwise input->name
> is NULL. Am I correct?

No. When the hid_input struct is allocated in hid-input.c, the field
hi->input->name is set to hdev->name (my previous "hint" should be
here).

The whole dirty part of the patch when I included the "Pen" in the
name was because of that: hi->input->name is never null, and it was
difficult to change this in hid-core.c without any side effects in the
other hid drivers.

Cheers,
Benjamin

Re: [PATCH] HID: multitouch: prevent memleak with the allocated name

[Andy Shevchenko]

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

Re: [PATCH] HID: multitouch: prevent memleak with the allocated name

[Benjamin Tissoires]

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

Re: [PATCH] HID: multitouch: prevent memleak with the allocated name

[Jiri Kosina]

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,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: multitouch: prevent memleak with the allocated name

[Andy Shevchenko]

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