netdev - Re: 答复: [PATCH][v2] net: ethtool: not call vzalloc for zero sized memory request

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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 29 Mar 2019 11:06:56 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     "Li,Rongqing" <lirongqing@...du.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: 答复: [PATCH][v2] net:
 ethtool: not call vzalloc for zero sized memory request

On Fri, Mar 29, 2019 at 09:35:38AM +0000, Li,Rongqing wrote:
> 
> 
> > -----邮件原件-----
> > 发件人: Michal Kubecek [mailto:mkubecek@...e.cz]
> > 发送时间: 2019年3月29日 17:29
> > 收件人: Li,Rongqing <lirongqing@...du.com>
> > 抄送: netdev@...r.kernel.org
> > 主题: Re: [PATCH][v2] net: ethtool: not call vzalloc for zero sized memory
> > request
> > 
> > On Fri, Mar 29, 2019 at 09:18:02AM +0800, Li RongQing wrote:
> > > NULL or ZERO_SIZE_PTR will be returned for zero sized memory request,
> > > and derefencing them will lead to a segfault
> > >
> > > so it is unnecessory to call vzalloc for zero sized memory request and
> > > not call functions which maybe derefence the NULL allocated memory
> > >
> > > this also fixes a possible memory leak if phy_ethtool_get_stats
> > > returns error, memory should be freed before exit
> > >
> > > Signed-off-by: Li RongQing <lirongqing@...du.com>
> > > Reviewed-by: Wang Li <wangli39@...du.com>
> > > ---
> > > v1->v2: not call get_ethtool_stats if n_stats is 0
> > >
> > >  net/core/ethtool.c | 46
> > > ++++++++++++++++++++++++++++++----------------
> > >  1 file changed, 30 insertions(+), 16 deletions(-)
> > 
> > This looks correct, I have just one minor comment below.
> > 
> > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c index
> > > b1eb32419732..36ed619faf36 100644
> > > --- a/net/core/ethtool.c
> > > +++ b/net/core/ethtool.c
> > > @@ -1797,11 +1797,16 @@ static int ethtool_get_strings(struct net_device
> > *dev, void __user *useraddr)
> > >  	WARN_ON_ONCE(!ret);
> > >
> > >  	gstrings.len = ret;
> > > -	data = vzalloc(array_size(gstrings.len, ETH_GSTRING_LEN));
> > > -	if (gstrings.len && !data)
> > > -		return -ENOMEM;
> > >
> > > -	__ethtool_get_strings(dev, gstrings.string_set, data);
> > > +	if (gstrings.len) {
> > > +		data = vzalloc(array_size(gstrings.len, ETH_GSTRING_LEN));
> > > +		if (!data)
> > > +			return -ENOMEM;
> > > +
> > > +		__ethtool_get_strings(dev, gstrings.string_set, data);
> > > +	} else {
> > > +		data = NULL;
> > > +	}
> > >
> > >  	ret = -EFAULT;
> > >  	if (copy_to_user(useraddr, &gstrings, sizeof(gstrings)))
> > 
> > If gstrings.len is zero, there is no need to set data pointer as it's not going to be
> > used so that you could omit the else branch. It's the same in the other two
> > functions.
> > 
> I think it is must, 
> since data is from stack, and not init, once it is passed into vfree, will cause panic

Ah, right, you would be still going through the vfree() call. So you
need either to set it to NULL in the "else" branch or initialize to NULL
where it's declared. Let's leave the patch as it is.

Reviewed-by: Michal Kubecek <mkubecek@...e.cz>

Michal