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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 30 Apr 2019 10:14:37 -0500
From:   Tyler Hicks <tyhicks@...onical.com>
To:     "Tobin C. Harding" <tobin@...nel.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Ido Schimmel <idosch@...lanox.com>,
        Alexander Duyck <alexander.h.duyck@...el.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Wang Hai <wanghai26@...wei.com>,
        YueHaibing <yuehaibing@...wei.com>,
        Amritha Nambiar <amritha.nambiar@...el.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] bridge: Fix error path for kobject_init_and_add()

On 2019-04-30 10:28:15, Tobin C. Harding wrote:
> Currently error return from kobject_init_and_add() is not followed by a
> call to kobject_put().  This means there is a memory leak.
> 
> Add call to kobject_put() in error path of kobject_init_and_add().
> 
> Signed-off-by: Tobin C. Harding <tobin@...nel.org>
> ---
>  net/bridge/br_if.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 41f0a696a65f..e5c8c9941c51 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -607,8 +607,10 @@ int br_add_if(struct net_bridge *br, struct net_device *dev,
>  
>  	err = kobject_init_and_add(&p->kobj, &brport_ktype, &(dev->dev.kobj),
>  				   SYSFS_BRIDGE_PORT_ATTR);
> -	if (err)
> +	if (err) {
> +		kobject_put(&p->kobj);
>  		goto err1;
> +	}

I think this is duplicating the code under the err2 label and doing so
in a way that would introduce a double free.

If the refcount hits 0 in the kobject_put(), release_nbp() is called
which calls kfree() on the p pointer. However, the p pointer is never
set to NULL like what's done under the err2 label. Once we're back in
br_add_if(), kfree() is called on the p pointer again just before
returning.

I think it would be better if you just jumped to the err2 label instead
of err1. err1 will no longer be used so, unfortunately, you'll have to
refactor all the labels at the same time.

Tyler

>  
>  	err = br_sysfs_addif(p);
>  	if (err)
> -- 
> 2.21.0
> 

Powered by blists - more mailing lists