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@xxxxxxxxx> wrote:
> On Thu, May 30, 2013 at 4:28 PM, Benjamin Tissoires
> <benjamin.tissoires@xxxxxxxxx> wrote:
>> 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.
>
> 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
--
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/