linux-kernel - Re: [PATCH] phy: Fix error handling in tegra_xusb_port

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <p4rkf6bfo76njky6ovyk2w5rgqugjrubnc7npfkj5vvpivufs5@6b4yzxoapbeq>
Date: Thu, 9 Jan 2025 14:37:12 +0100
From: Thierry Reding <thierry.reding@...il.com>
To: Ma Ke <make24@...as.ac.cn>
Cc: jckuo@...dia.com, vkoul@...nel.org, kishon@...nel.org, 
	jonathanh@...dia.com, linux-phy@...ts.infradead.org, linux-tegra@...r.kernel.org, 
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] phy: Fix error handling in tegra_xusb_port_init

On Tue, Jan 07, 2025 at 04:51:29PM +0800, Ma Ke wrote:
> The reference count of the device incremented in device_initialize() 
> is not decremented properly when device_add() fails. Change 
> device_unregister() to a put_device() call before returning from the 
> function 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.

While the patch looks correct, the commit message seems confusing to me.

device_unregister() will also call put_device() after first calling
device_del(). So the reference count /is/ properly balanced.

Instead, the kerneldoc comment for device_add() says this:

 * NOTE: _Never_ directly free @dev after calling this function, even
 * if it returned an error! Always use put_device() to give up your
 * reference instead.
 *
 * Rule of thumb is: 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.

So the real reason that this patch is correct is because
device_unregister() should only be called after device_add() succeeded
because device_del() undoes what device_add() does if successful. In
this case we only need to undo what device_initialize() does, and that
is what put_device() will do.

Thierry

> 
> Cc: stable@...r.kernel.org
> Fixes: 53d2a715c240 ("phy: Add Tegra XUSB pad controller support")
> Signed-off-by: Ma Ke <make24@...as.ac.cn>
> ---
>  drivers/phy/tegra/xusb.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
> index 79d4814d758d..c89df95aa6ca 100644
> --- a/drivers/phy/tegra/xusb.c
> +++ b/drivers/phy/tegra/xusb.c
> @@ -548,16 +548,16 @@ static int tegra_xusb_port_init(struct tegra_xusb_port *port,
>  
>  	err = dev_set_name(&port->dev, "%s-%u", name, index);
>  	if (err < 0)
> -		goto unregister;
> +		goto put_device;
>  
>  	err = device_add(&port->dev);
>  	if (err < 0)
> -		goto unregister;
> +		goto put_device;
>  
>  	return 0;
>  
> -unregister:
> -	device_unregister(&port->dev);
> +put_device:
> +	put_device(&port->dev);
>  	return err;
>  }
>  
> -- 
> 2.25.1
> 

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)