2022-04-22 18:20:18

by José Expósito

Subject: [PATCH 2/3] HID: logitech-hidpp: Fix double free on managed resource

As described in the documentation for devm_input_allocate_device():

Managed input devices do not need to be explicitly unregistered or
freed as it will be done automatically when owner device unbinds from
its driver (or binding fails).

However this driver was explicitly freeing the input device, allocated
using devm_input_allocate_device() through hidpp_allocate_input().

Remove the call to input_free_device() to avoid a possible double free
error.

Fixes: c39e3d5fc9dd3 ("HID: logitech-hidpp: late bind the input device on wireless connection")
Signed-off-by: José Expósito <[email protected]>
---
drivers/hid/hid-logitech-hidpp.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 81de88ab2ecc..9c00a781ab57 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -3957,11 +3957,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
}

hidpp_populate_input(hidpp, input);
-
- ret = input_register_device(input);
- if (ret)
- input_free_device(input);
-
+ input_register_device(input);
hidpp->delayed_input = input;
}

--
2.25.1

2022-04-23 16:44:51

by Hans de Goede

Subject: Re: [PATCH 2/3] HID: logitech-hidpp: Fix double free on managed resource

Hi,

On 4/22/22 18:17, José Expósito wrote:
> As described in the documentation for devm_input_allocate_device():
>
> Managed input devices do not need to be explicitly unregistered or
> freed as it will be done automatically when owner device unbinds from
> its driver (or binding fails).
>
> However this driver was explicitly freeing the input device, allocated
> using devm_input_allocate_device() through hidpp_allocate_input().
>
> Remove the call to input_free_device() to avoid a possible double free
> error.

Actually calling input_free_device() on a devm allocated input device
is fine. The input subsystem has chosen to not have a
separate devm_input_free_device(), instead input_free_device() knows
if a device is allocated through devm and then also frees the devres
tied to it:

void input_free_device(struct input_dev *dev)
{
if (dev) {
if (dev->devres_managed)
WARN_ON(devres_destroy(dev->dev.parent,
devm_input_device_release,
devm_input_device_match,
dev));
input_put_device(dev);
}
}


>
> Fixes: c39e3d5fc9dd3 ("HID: logitech-hidpp: late bind the input device on wireless connection")
> Signed-off-by: José Expósito <[email protected]>
> ---
> drivers/hid/hid-logitech-hidpp.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 81de88ab2ecc..9c00a781ab57 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -3957,11 +3957,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> }
>
> hidpp_populate_input(hidpp, input);
> -
> - ret = input_register_device(input);
> - if (ret)
> - input_free_device(input);
> -

The original code does look wrong there though, since the input device
is free-ed it should not be stored in hidpp->delayed_input, so this should be comes:

ret = input_register_device(input);
if (ret) {
input_free_device(input);
return;
}


Regards,

Hans


> hidpp->delayed_input = input;
> }
>

2022-04-24 10:46:39

by José Expósito

Subject: Re: [PATCH 2/3] HID: logitech-hidpp: Fix double free on managed resource

On Sat, Apr 23, 2022 at 01:41:30PM +0200, Hans de Goede wrote:
> Hi,
>
> On 4/22/22 18:17, Jos? Exp?sito wrote:
> > As described in the documentation for devm_input_allocate_device():
> >
> > Managed input devices do not need to be explicitly unregistered or
> > freed as it will be done automatically when owner device unbinds from
> > its driver (or binding fails).
> >
> > However this driver was explicitly freeing the input device, allocated
> > using devm_input_allocate_device() through hidpp_allocate_input().
> >
> > Remove the call to input_free_device() to avoid a possible double free
> > error.
>
> Actually calling input_free_device() on a devm allocated input device
> is fine. The input subsystem has chosen to not have a
> separate devm_input_free_device(), instead input_free_device() knows
> if a device is allocated through devm and then also frees the devres
> tied to it:
>
> void input_free_device(struct input_dev *dev)
> {
> if (dev) {
> if (dev->devres_managed)
> WARN_ON(devres_destroy(dev->dev.parent,
> devm_input_device_release,
> devm_input_device_match,
> dev));
> input_put_device(dev);
> }
> }

Hi Hans,

Thanks for the code review.

Obviously, I completely misunderstood these functions, my bad.
Thanks for the explanation.

Please ignore the patchset.

Jose

> >
> > Fixes: c39e3d5fc9dd3 ("HID: logitech-hidpp: late bind the input device on wireless connection")
> > Signed-off-by: Jos? Exp?sito <[email protected]>
> > ---
> > drivers/hid/hid-logitech-hidpp.c | 6 +-----
> > 1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 81de88ab2ecc..9c00a781ab57 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -3957,11 +3957,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> > }
> >
> > hidpp_populate_input(hidpp, input);
> > -
> > - ret = input_register_device(input);
> > - if (ret)
> > - input_free_device(input);
> > -
>
> The original code does look wrong there though, since the input device
> is free-ed it should not be stored in hidpp->delayed_input, so this should be comes:
>
> ret = input_register_device(input);
> if (ret) {
> input_free_device(input);
> return;
> }
>
>
> Regards,
>
> Hans
>
>
> > hidpp->delayed_input = input;
> > }
> >
>
>