* [PATCH] net: ethtool: not call vzalloc for zero sized memory request
@ 2019-03-28 6:01 Li RongQing
2019-03-28 9:09 ` Michal Kubecek
0 siblings, 1 reply; 5+ messages in thread
From: Li RongQing @ 2019-03-28 6:01 UTC (permalink / raw)
To: netdev
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 __ethtool_get_strings which always uses
the 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>
---
net/core/ethtool.c | 37 ++++++++++++++++++++++++++-----------
1 file changed, 26 insertions(+), 11 deletions(-)
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index b1eb32419732..3e971a36e37c 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,9 +1902,14 @@ 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;
+
+ if (n_stats) {
+ data = vzalloc(array_size(n_stats, sizeof(u64)));
+ if (!data)
+ return -ENOMEM;
+ } else {
+ data = NULL;
+ }
ops->get_ethtool_stats(dev, &stats, data);
@@ -1941,14 +1951,19 @@ 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 (n_stats) {
+ data = vzalloc(array_size(n_stats, sizeof(u64)));
+ if (!data)
+ return -ENOMEM;
+ } else {
+ data = NULL;
+ }
if (dev->phydev && !ops->get_ethtool_phy_stats) {
ret = phy_ethtool_get_stats(dev->phydev, &stats, data);
if (ret < 0)
- return ret;
+ goto out;
} else {
ops->get_ethtool_phy_stats(dev, &stats, data);
}
--
2.16.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] net: ethtool: not call vzalloc for zero sized memory request
2019-03-28 6:01 [PATCH] net: ethtool: not call vzalloc for zero sized memory request Li RongQing
@ 2019-03-28 9:09 ` Michal Kubecek
2019-03-28 9:51 ` 答复: " Li,Rongqing
0 siblings, 1 reply; 5+ messages in thread
From: Michal Kubecek @ 2019-03-28 9:09 UTC (permalink / raw)
To: Li RongQing; +Cc: netdev
On Thu, Mar 28, 2019 at 02:01:09PM +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 __ethtool_get_strings which always uses
> the 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>
> ---
> net/core/ethtool.c | 37 ++++++++++++++++++++++++++-----------
> 1 file changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index b1eb32419732..3e971a36e37c 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
...
> @@ -1897,9 +1902,14 @@ 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;
> +
> + if (n_stats) {
> + data = vzalloc(array_size(n_stats, sizeof(u64)));
> + if (!data)
> + return -ENOMEM;
> + } else {
> + data = NULL;
> + }
>
> ops->get_ethtool_stats(dev, &stats, data);
>
You avoid the vzalloc() call here but you still pass null data pointer
to device's get_ethtool_stats() handler which seems to contradict what
the commit message says. Is it really what you want? (The same applies
to ethtool_get_phy_stats() below.)
Michal
> @@ -1941,14 +1951,19 @@ 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 (n_stats) {
> + data = vzalloc(array_size(n_stats, sizeof(u64)));
> + if (!data)
> + return -ENOMEM;
> + } else {
> + data = NULL;
> + }
>
> if (dev->phydev && !ops->get_ethtool_phy_stats) {
> ret = phy_ethtool_get_stats(dev->phydev, &stats, data);
> if (ret < 0)
> - return ret;
> + goto out;
> } else {
> ops->get_ethtool_phy_stats(dev, &stats, data);
> }
> --
> 2.16.2
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* 答复: [PATCH] net: ethtool: not call vzalloc for zero sized memory request
2019-03-28 9:09 ` Michal Kubecek
@ 2019-03-28 9:51 ` Li,Rongqing
2019-03-28 10:25 ` Michal Kubecek
0 siblings, 1 reply; 5+ messages in thread
From: Li,Rongqing @ 2019-03-28 9:51 UTC (permalink / raw)
To: Michal Kubecek; +Cc: netdev@vger.kernel.org
> -----邮件原件-----
> 发件人: Michal Kubecek [mailto:mkubecek@suse.cz]
> 发送时间: 2019年3月28日 17:09
> 收件人: Li,Rongqing <lirongqing@baidu.com>
> 抄送: netdev@vger.kernel.org
> 主题: Re: [PATCH] net: ethtool: not call vzalloc for zero sized memory request
>
> On Thu, Mar 28, 2019 at 02:01:09PM +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 __ethtool_get_strings which always uses the 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>
> > ---
> > net/core/ethtool.c | 37 ++++++++++++++++++++++++++-----------
> > 1 file changed, 26 insertions(+), 11 deletions(-)
> >
> > diff --git a/net/core/ethtool.c b/net/core/ethtool.c index
> > b1eb32419732..3e971a36e37c 100644
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> ...
> > @@ -1897,9 +1902,14 @@ 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;
> > +
> > + if (n_stats) {
> > + data = vzalloc(array_size(n_stats, sizeof(u64)));
> > + if (!data)
> > + return -ENOMEM;
> > + } else {
> > + data = NULL;
> > + }
> >
> > ops->get_ethtool_stats(dev, &stats, data);
> >
>
> You avoid the vzalloc() call here but you still pass null data pointer to device's
> get_ethtool_stats() handler which seems to contradict what the commit
> message says. Is it really what you want? (The same applies to
> ethtool_get_phy_stats() below.)
>
I keep it deliberately
ops->get_ethtool_stats(dev, &stats, data) have three parameter,
if n_stats is 0 [we assume the returning 0 of ops->get_sset_count is correct, or
get_sset_count should be fixed], data is NULL, get_ethtool_stats () maybe still
store the data into its seconds parameter, stats, even if I did not find which drivers
is like that
but __ethtool_get_strings is different, only one place to store data, so
if count is 0, __ethtool_get_strings does not need to be called
-RongQing
> Michal
>
> > @@ -1941,14 +1951,19 @@ 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 (n_stats) {
> > + data = vzalloc(array_size(n_stats, sizeof(u64)));
> > + if (!data)
> > + return -ENOMEM;
> > + } else {
> > + data = NULL;
> > + }
> >
> > if (dev->phydev && !ops->get_ethtool_phy_stats) {
> > ret = phy_ethtool_get_stats(dev->phydev, &stats, data);
> > if (ret < 0)
> > - return ret;
> > + goto out;
> > } else {
> > ops->get_ethtool_phy_stats(dev, &stats, data);
> > }
> > --
> > 2.16.2
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: 答复: [PATCH] net: ethtool: not call vzalloc for zero sized memory request
2019-03-28 9:51 ` 答复: " Li,Rongqing
@ 2019-03-28 10:25 ` Michal Kubecek
2019-03-28 11:00 ` 答复: " Li,Rongqing
0 siblings, 1 reply; 5+ messages in thread
From: Michal Kubecek @ 2019-03-28 10:25 UTC (permalink / raw)
To: Li,Rongqing; +Cc: netdev@vger.kernel.org
On Thu, Mar 28, 2019 at 09:51:56AM +0000, Li,Rongqing wrote:
> > -----邮件原件-----
> > 发件人: Michal Kubecek [mailto:mkubecek@suse.cz]
> > 发送时间: 2019年3月28日 17:09
> > 收件人: Li,Rongqing <lirongqing@baidu.com>
> > 抄送: netdev@vger.kernel.org
> > 主题: Re: [PATCH] net: ethtool: not call vzalloc for zero sized memory request
> >
> > On Thu, Mar 28, 2019 at 02:01:09PM +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 __ethtool_get_strings which always uses the 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>
> > > ---
> > > net/core/ethtool.c | 37 ++++++++++++++++++++++++++-----------
> > > 1 file changed, 26 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c index
> > > b1eb32419732..3e971a36e37c 100644
> > > --- a/net/core/ethtool.c
> > > +++ b/net/core/ethtool.c
> > ...
> > > @@ -1897,9 +1902,14 @@ 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;
> > > +
> > > + if (n_stats) {
> > > + data = vzalloc(array_size(n_stats, sizeof(u64)));
> > > + if (!data)
> > > + return -ENOMEM;
> > > + } else {
> > > + data = NULL;
> > > + }
> > >
> > > ops->get_ethtool_stats(dev, &stats, data);
> > >
> >
> > You avoid the vzalloc() call here but you still pass null data pointer to device's
> > get_ethtool_stats() handler which seems to contradict what the commit
> > message says. Is it really what you want? (The same applies to
> > ethtool_get_phy_stats() below.)
> >
>
>
> I keep it deliberately
>
> ops->get_ethtool_stats(dev, &stats, data) have three parameter,
> if n_stats is 0 [we assume the returning 0 of ops->get_sset_count is correct, or
> get_sset_count should be fixed], data is NULL, get_ethtool_stats () maybe still
> store the data into its seconds parameter, stats, even if I did not find which drivers
> is like that
stats is a local variable declared as
struct ethtool_stats stats;
where struct ethtool_stats which looks like
struct ethtool_stats {
__u32 cmd;
__u32 n_stats;
__u64 data[0];
};
so that storing anything to stats.data would result in stack corruption.
This is why ethtool_ops::get_ethtool_stats() has the third parameter
telling it where to put the statistics.
There isn't much point touching cmd and n_stats should be set to the
same value as we got from ethtool_ops::get_sset_counts earlier, i.e.
zero in our case.
Michal
^ permalink raw reply [flat|nested] 5+ messages in thread
* 答复: 答复: [PATCH] net: ethtool: not call vzalloc for zero sized memory request
2019-03-28 10:25 ` Michal Kubecek
@ 2019-03-28 11:00 ` Li,Rongqing
0 siblings, 0 replies; 5+ messages in thread
From: Li,Rongqing @ 2019-03-28 11:00 UTC (permalink / raw)
To: Michal Kubecek; +Cc: netdev@vger.kernel.org
> -----邮件原件-----
> 发件人: Michal Kubecek [mailto:mkubecek@suse.cz]
> 发送时间: 2019年3月28日 18:26
> 收件人: Li,Rongqing <lirongqing@baidu.com>
> 抄送: netdev@vger.kernel.org
> 主题: Re: 答复: [PATCH] net: ethtool: not call vzalloc for zero sized memory
> request
>
> On Thu, Mar 28, 2019 at 09:51:56AM +0000, Li,Rongqing wrote:
> > > -----邮件原件-----
> > > 发件人: Michal Kubecek [mailto:mkubecek@suse.cz]
> > > 发送时间: 2019年3月28日 17:09
> > > 收件人: Li,Rongqing <lirongqing@baidu.com>
> > > 抄送: netdev@vger.kernel.org
> > > 主题: Re: [PATCH] net: ethtool: not call vzalloc for zero sized memory
> > > request
> > >
> > > On Thu, Mar 28, 2019 at 02:01:09PM +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 __ethtool_get_strings which always uses the 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>
> > > > ---
> > > > net/core/ethtool.c | 37 ++++++++++++++++++++++++++-----------
> > > > 1 file changed, 26 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c index
> > > > b1eb32419732..3e971a36e37c 100644
> > > > --- a/net/core/ethtool.c
> > > > +++ b/net/core/ethtool.c
> > > ...
> > > > @@ -1897,9 +1902,14 @@ 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;
> > > > +
> > > > + if (n_stats) {
> > > > + data = vzalloc(array_size(n_stats, sizeof(u64)));
> > > > + if (!data)
> > > > + return -ENOMEM;
> > > > + } else {
> > > > + data = NULL;
> > > > + }
> > > >
> > > > ops->get_ethtool_stats(dev, &stats, data);
> > > >
> > >
> > > You avoid the vzalloc() call here but you still pass null data
> > > pointer to device's
> > > get_ethtool_stats() handler which seems to contradict what the
> > > commit message says. Is it really what you want? (The same applies
> > > to
> > > ethtool_get_phy_stats() below.)
> > >
> >
> >
> > I keep it deliberately
> >
> > ops->get_ethtool_stats(dev, &stats, data) have three parameter,
> > if n_stats is 0 [we assume the returning 0 of ops->get_sset_count is
> > correct, or get_sset_count should be fixed], data is NULL,
> > get_ethtool_stats () maybe still store the data into its seconds
> > parameter, stats, even if I did not find which drivers is like that
>
> stats is a local variable declared as
>
> struct ethtool_stats stats;
>
> where struct ethtool_stats which looks like
>
> struct ethtool_stats {
> __u32 cmd;
> __u32 n_stats;
> __u64 data[0];
> };
>
> so that storing anything to stats.data would result in stack corruption.
> This is why ethtool_ops::get_ethtool_stats() has the third parameter telling it
> where to put the statistics.
>
> There isn't much point touching cmd and n_stats should be set to the same
> value as we got from ethtool_ops::get_sset_counts earlier, i.e.
> zero in our case.
>
Ok, I will send v2 which does not call ops->get_ethtool_stats, if n_stats is 0
Thanks
-RongQing
> Michal
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-03-28 11:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-28 6:01 [PATCH] net: ethtool: not call vzalloc for zero sized memory request Li RongQing
2019-03-28 9:09 ` Michal Kubecek
2019-03-28 9:51 ` 答复: " Li,Rongqing
2019-03-28 10:25 ` Michal Kubecek
2019-03-28 11:00 ` 答复: " Li,Rongqing
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).