[PATCH][v2] net: ethtool: not call vzalloc for zero sized memory request

[PATCH][v2] net: ethtool: not call vzalloc for zero sized memory request

[Li RongQing]

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@baidu.com>
Reviewed-by: Wang Li <wangli39@baidu.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(-)

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)))
@@ -1897,11 +1902,15 @@ static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
 		return -EFAULT;
 
 	stats.n_stats = n_stats;
-	data = vzalloc(array_size(n_stats, sizeof(u64)));
-	if (n_stats && !data)
-		return -ENOMEM;
 
-	ops->get_ethtool_stats(dev, &stats, data);
+	if (n_stats) {
+		data = vzalloc(array_size(n_stats, sizeof(u64)));
+		if (!data)
+			return -ENOMEM;
+		ops->get_ethtool_stats(dev, &stats, data);
+	} else {
+		data = NULL;
+	}
 
 	ret = -EFAULT;
 	if (copy_to_user(useraddr, &stats, sizeof(stats)))
@@ -1941,16 +1950,21 @@ static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr)
 		return -EFAULT;
 
 	stats.n_stats = n_stats;
-	data = vzalloc(array_size(n_stats, sizeof(u64)));
-	if (n_stats && !data)
-		return -ENOMEM;
 
-	if (dev->phydev && !ops->get_ethtool_phy_stats) {
-		ret = phy_ethtool_get_stats(dev->phydev, &stats, data);
-		if (ret < 0)
-			return ret;
+	if (n_stats) {
+		data = vzalloc(array_size(n_stats, sizeof(u64)));
+		if (!data)
+			return -ENOMEM;
+
+		if (dev->phydev && !ops->get_ethtool_phy_stats) {
+			ret = phy_ethtool_get_stats(dev->phydev, &stats, data);
+			if (ret < 0)
+				goto out;
+		} else {
+			ops->get_ethtool_phy_stats(dev, &stats, data);
+		}
 	} else {
-		ops->get_ethtool_phy_stats(dev, &stats, data);
+		data = NULL;
 	}
 
 	ret = -EFAULT;
-- 
2.16.2

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

[Michal Kubecek]

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@baidu.com>
> Reviewed-by: Wang Li <wangli39@baidu.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.

Michal

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

[Li,Rongqing]

> -----邮件原件-----
> 发件人: Michal Kubecek [mailto:mkubecek@suse.cz]
> 发送时间: 2019年3月29日 17:29
> 收件人: Li,Rongqing <lirongqing@baidu.com>
> 抄送: netdev@vger.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@baidu.com>
> > Reviewed-by: Wang Li <wangli39@baidu.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

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

[Michal Kubecek]

On Fri, Mar 29, 2019 at 09:35:38AM +0000, Li,Rongqing wrote:
> 
> 
> > -----邮件原件-----
> > 发件人: Michal Kubecek [mailto:mkubecek@suse.cz]
> > 发送时间: 2019年3月29日 17:29
> > 收件人: Li,Rongqing <lirongqing@baidu.com>
> > 抄送: netdev@vger.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@baidu.com>
> > > Reviewed-by: Wang Li <wangli39@baidu.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@suse.cz>

Michal

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

[David Miller]

From: Li RongQing <lirongqing@baidu.com>
Date: Fri, 29 Mar 2019 09:18:02 +0800

> 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@baidu.com>
> Reviewed-by: Wang Li <wangli39@baidu.com>
> ---
> v1->v2: not call get_ethtool_stats if n_stats is 0 

Applied and queued up for -stable, thanks.