netdev - 答复: [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: Windows password security audit tool. GUI, reports in PDF.

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

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



> -----邮件原件-----
> 发件人: 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

thanks

-RongQing

> Michal