| Message ID | 1553822282-6658-1-git-send-email-lirongqing@baidu.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v2] net: ethtool: not call vzalloc for zero sized memory request | expand |
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
> -----邮件原件----- > 发件人: 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
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
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.
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;