* [PATCH][v2] net: ethtool: not call vzalloc for zero sized memory request
@ 2019-03-29 1:18 Li RongQing
2019-03-29 9:29 ` Michal Kubecek
2019-03-29 20:42 ` David Miller
0 siblings, 2 replies; 5+ messages in thread
From: Li RongQing @ 2019-03-29 1:18 UTC (permalink / raw)
To: netdev, Michal Kubecek
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH][v2] net: ethtool: not call vzalloc for zero sized memory request
2019-03-29 1:18 [PATCH][v2] net: ethtool: not call vzalloc for zero sized memory request Li RongQing
@ 2019-03-29 9:29 ` Michal Kubecek
2019-03-29 9:35 ` 答复: " Li,Rongqing
2019-03-29 20:42 ` David Miller
1 sibling, 1 reply; 5+ messages in thread
From: Michal Kubecek @ 2019-03-29 9:29 UTC (permalink / raw)
To: Li RongQing; +Cc: netdev
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
^ permalink raw reply [flat|nested] 5+ messages in thread* 答复: [PATCH][v2] net: ethtool: not call vzalloc for zero sized memory request
2019-03-29 9:29 ` Michal Kubecek
@ 2019-03-29 9:35 ` Li,Rongqing
2019-03-29 10:06 ` Michal Kubecek
0 siblings, 1 reply; 5+ messages in thread
From: Li,Rongqing @ 2019-03-29 9:35 UTC (permalink / raw)
To: Michal Kubecek; +Cc: netdev
> -----邮件原件-----
> 发件人: 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: 答复: [PATCH][v2] net: ethtool: not call vzalloc for zero sized memory request
2019-03-29 9:35 ` 答复: " Li,Rongqing
@ 2019-03-29 10:06 ` Michal Kubecek
0 siblings, 0 replies; 5+ messages in thread
From: Michal Kubecek @ 2019-03-29 10:06 UTC (permalink / raw)
To: Li,Rongqing; +Cc: netdev
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][v2] net: ethtool: not call vzalloc for zero sized memory request
2019-03-29 1:18 [PATCH][v2] net: ethtool: not call vzalloc for zero sized memory request Li RongQing
2019-03-29 9:29 ` Michal Kubecek
@ 2019-03-29 20:42 ` David Miller
1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2019-03-29 20:42 UTC (permalink / raw)
To: lirongqing; +Cc: netdev, mkubecek
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-04-01 18:05 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-29 1:18 [PATCH][v2] net: ethtool: not call vzalloc for zero sized memory request Li RongQing
2019-03-29 9:29 ` Michal Kubecek
2019-03-29 9:35 ` 答复: " Li,Rongqing
2019-03-29 10:06 ` Michal Kubecek
2019-03-29 20:42 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.