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@xxxxxxxxx> 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@xxxxxxxxx>
> Reported-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/