[PATCH] Input: fix error handling in input_register_device()

[PATCH] Input: fix error handling in input_register_device()

[Ma Ke]

Once device_add() failed, we should call put_device() to decrement
reference count for cleanup. Or it could cause memory leak.

As comment of device_add() says, 'if device_add() succeeds, you should
call device_del() when you want to get rid of it. If device_add() has
not succeeded, use only put_device() to drop the reference count'.

Found by code review.

Cc: stable@vger.kernel.org
Fixes: 0cd587735205 ("Input: preallocate memory to hold event values")
Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
 drivers/input/input.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index c9e3ac64bcd0..2e70f346dadc 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -2424,6 +2424,7 @@ int input_register_device(struct input_dev *dev)
 err_device_del:
 	device_del(&dev->dev);
 err_devres_free:
+	put_device(&dev->dev);
 	devres_free(devres);
 	return error;
 }
-- 
2.25.1

Re: [PATCH] Input: fix error handling in input_register_device()

[Greg KH]

On Thu, Mar 13, 2025 at 04:38:03PM +0800, Ma Ke wrote:
> Once device_add() failed, we should call put_device() to decrement
> reference count for cleanup. Or it could cause memory leak.
> 
> As comment of device_add() says, 'if device_add() succeeds, you should
> call device_del() when you want to get rid of it. If device_add() has
> not succeeded, use only put_device() to drop the reference count'.
> 
> Found by code review.
> 
> Cc: stable@vger.kernel.org
> Fixes: 0cd587735205 ("Input: preallocate memory to hold event values")
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> ---
>  drivers/input/input.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index c9e3ac64bcd0..2e70f346dadc 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -2424,6 +2424,7 @@ int input_register_device(struct input_dev *dev)
>  err_device_del:
>  	device_del(&dev->dev);
>  err_devres_free:
> +	put_device(&dev->dev);

Did you test this?  This is not going to work properly for all places
that call this label.

While these types of fixes are nice to have, PLEASE TEST THEM!!!!

Stop doing these and verify, by actual tests, that they do what you
think they are doing before sending any more of them.  As it is, this
stream of "fixes" is just too full of bugs for any of us to trust them
at all.

thanks,

greg k-h

[PATCH] Input: Fix error handling in input_register_device()

[Ma Ke]

When device_add(&dev->dev) failed, calling put_device() to explicitly
release dev->dev. Otherwise, it could cause double free problem.

As comment of device_add() says, if device_add() succeeds, you should
call device_del() when you want to get rid of it. If device_add() has
not succeeded, use only put_device() to drop the reference count.

Found by code review.

Cc: stable@vger.kernel.org
Fixes: 0cd587735205 ("Input: preallocate memory to hold event values")
Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
 drivers/input/input.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 7f0477e04ad2..a0a36aa90ecc 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -2456,8 +2456,10 @@ int input_register_device(struct input_dev *dev)
 		input_dev_poller_finalize(dev->poller);
 
 	error = device_add(&dev->dev);
-	if (error)
+	if (error) {
+		put_device(&dev->dev);
 		goto err_devres_free;
+	}
 
 	path = kobject_get_path(&dev->dev.kobj, GFP_KERNEL);
 	pr_info("%s as %s\n",
-- 
2.25.1

Re: [PATCH] Input: Fix error handling in input_register_device()

[Dmitry Torokhov]

Hi,

On Sun, Jan 05, 2025 at 05:24:48PM +0800, Ma Ke wrote:
> When device_add(&dev->dev) failed, calling put_device() to explicitly
> release dev->dev. Otherwise, it could cause double free problem.

How exactly allegedly missing put would cause double free?

> 
> As comment of device_add() says, if device_add() succeeds, you should
> call device_del() when you want to get rid of it. If device_add() has
> not succeeded, use only put_device() to drop the reference count.

As explained in the kerneldoc for input_register_device(), in case of
the failure caller must call input_free_device() which will do the
required "put" as well as will handle devm-allocated input devices
properly.

Adding call to put_device() as proposed by this patch will indeed
introduce double-free.

> 
> Found by code review.
> 
> Cc: stable@vger.kernel.org
> Fixes: 0cd587735205 ("Input: preallocate memory to hold event values")
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> ---
>  drivers/input/input.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 7f0477e04ad2..a0a36aa90ecc 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -2456,8 +2456,10 @@ int input_register_device(struct input_dev *dev)
>  		input_dev_poller_finalize(dev->poller);
>  
>  	error = device_add(&dev->dev);
> -	if (error)
> +	if (error) {
> +		put_device(&dev->dev);
>  		goto err_devres_free;
> +	}
>  
>  	path = kobject_get_path(&dev->dev.kobj, GFP_KERNEL);
>  	pr_info("%s as %s\n",
> -- 
> 2.25.1
> 

Thanks.

-- 
Dmitry