[PATCH] ethtool: Zero memory allocated for statistics

[PATCH] ethtool: Zero memory allocated for statistics

[Vlad Tsyrklevich]

Zero allocations before they're passed to drivers to be filled out with
statistics. While many drivers always correctly fill out the entire
allocated space, under some failure conditions some drivers will not
clear the allocated space appropriately. Unprivileged users could
induce some of these failure conditions to leak kernel memory. Instead
of fixing drivers one by one, the best solution is to eliminate the
possibility of driver errors leaking kernel memory entirely.

Given that ethtool_get_stats(), ethtool_get_phy_stats(), and
ethtool_get_tunable() are accessible without CAP_NET_ADMIN they are the
most important to clear to avoid memory leaks. ethtool_self_test() and
ethtool_get_any_eeprom() require CAP_NET_ADMIN but were also included
for completeness.

Some examples of driver methods that could fail to fill out memory:
enic_get_ethtool_stats(), cp_get_ethtool_stats(),
mv88e6xxx_get_ethtool_stats(), bnx2x_self_test(), be_self_test(), etc.

Signed-off-by: Vlad Tsyrklevich <vlad@tsyrklevich.net>
---
 net/core/ethtool.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 9774898..7202915 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1538,7 +1538,7 @@ static int ethtool_get_any_eeprom(struct net_device *dev, void __user *useraddr,
 	if (eeprom.offset + eeprom.len > total_len)
 		return -EINVAL;
 
-	data = kmalloc(PAGE_SIZE, GFP_USER);
+	data = kzalloc(PAGE_SIZE, GFP_USER);
 	if (!data)
 		return -ENOMEM;
 
@@ -1775,7 +1775,7 @@ static int ethtool_self_test(struct net_device *dev, char __user *useraddr)
 		return -EFAULT;
 
 	test.len = test_len;
-	data = kmalloc(test_len * sizeof(u64), GFP_USER);
+	data = kcalloc(test_len, sizeof(u64), GFP_USER);
 	if (!data)
 		return -ENOMEM;
 
@@ -1907,7 +1907,7 @@ static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
 		return -EFAULT;
 
 	stats.n_stats = n_stats;
-	data = kmalloc(n_stats * sizeof(u64), GFP_USER);
+	data = kcalloc(n_stats, sizeof(u64), GFP_USER);
 	if (!data)
 		return -ENOMEM;
 
@@ -1946,7 +1946,7 @@ static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr)
 		return -EFAULT;
 
 	stats.n_stats = n_stats;
-	data = kmalloc_array(n_stats, sizeof(u64), GFP_USER);
+	data = kcalloc(n_stats, sizeof(u64), GFP_USER);
 	if (!data)
 		return -ENOMEM;
 
@@ -2269,7 +2269,7 @@ static int ethtool_get_tunable(struct net_device *dev, void __user *useraddr)
 	ret = ethtool_tunable_valid(&tuna);
 	if (ret)
 		return ret;
-	data = kmalloc(tuna.len, GFP_USER);
+	data = kzalloc(tuna.len, GFP_USER);
 	if (!data)
 		return -ENOMEM;
 	ret = ops->get_tunable(dev, &tuna, data);
-- 
2.7.0

Re: [PATCH] ethtool: Zero memory allocated for statistics

[David Miller]

From: Vlad Tsyrklevich <vlad@tsyrklevich.net>
Date: Fri, 14 Oct 2016 11:59:18 +0200

> enic_get_ethtool_stats()

Looknig merely at this shows the real problem.

We don't propagate and handle errors for this method.

And that's what we should fix, making the get_ethtool_stats() method
return an integer error.

Then ethtool_get_stats() should return any non-zero value provided by
ops->get_ethtool_stats() and not attempt to copy any bytes of 'data'
to userspace in that case.

Re: [PATCH] ethtool: Zero memory allocated for statistics

[Vlad Tsyrklevich]

I agree that we should propagate those errors and I'll prepare a new
change to do so for phy_driver.get_stats(), ethtool_ops.self_test(),
and ethtool_ops.get_ethtool_stats(). However, I still think this
change should be adopted. 3/5 of the cases here are reachable without
any special capabilities and programming defensively at the ethtool
interface can eliminate an entire class of potential driver bugs
instead of fixing them one by one. For example, get_eeprom()
propagates errors but with a brief grep I found that
qlcnic_get_eeprom() will return 0 incorrectly even though it read
nothing for some NICs. Deeper bugs are undoubtedly laying around.

On Sat, Oct 15, 2016 at 5:11 PM, Vlad Tsyrklevich <vlad@tsyrklevich.net> wrote:
> I agree that we should propagate those errors and I'll prepare a new change
> to do so for phy_driver.get_stats(), ethtool_ops.self_test(), and
> ethtool_ops.get_ethtool_stats(). However, I still think this change should
> be adopted. 3/5 of the cases here are reachable without any special
> capabilities and programming defensively at the ethtool interface can
> eliminate an entire class of potential driver bugs instead of fixing them
> one by one. For example, get_eeprom() propagates errors but with a brief
> grep I found that qlcnic_get_eeprom() will return 0 incorrectly even though
> it read nothing for some NICs. Deeper bugs are undoubtedly laying around.
>
> On Sat, Oct 15, 2016 at 3:21 AM David Miller <davem@davemloft.net> wrote:
>>
>> From: Vlad Tsyrklevich <vlad@tsyrklevich.net>
>> Date: Fri, 14 Oct 2016 11:59:18 +0200
>>
>> > enic_get_ethtool_stats()
>>
>> Looknig merely at this shows the real problem.
>>
>> We don't propagate and handle errors for this method.
>>
>> And that's what we should fix, making the get_ethtool_stats() method
>> return an integer error.
>>
>> Then ethtool_get_stats() should return any non-zero value provided by
>> ops->get_ethtool_stats() and not attempt to copy any bytes of 'data'
>> to userspace in that case.

Re: [PATCH] ethtool: Zero memory allocated for statistics

[David Miller]

From: Vlad Tsyrklevich <vlad@tsyrklevich.net>
Date: Sat, 15 Oct 2016 15:11:08 +0000

> I agree that we should propagate those errors and I'll prepare a new change
> to do so for phy_driver.get_stats(), ethtool_ops.self_test(), and
> ethtool_ops.get_ethtool_stats(). However, I still think this change should
> be adopted. 3/5 of the cases here are reachable without any special
> capabilities and programming defensively at the ethtool interface can
> eliminate an entire class of potential driver bugs instead of fixing them
> one by one. For example, get_eeprom() propagates errors but with a brief
> grep I found that qlcnic_get_eeprom() will return 0 incorrectly even though
> it read nothing for some NICs. Deeper bugs are undoubtedly laying around.

I'm all for defensive program when practical.

But statistics gathering is highly performance sensitive for many
important use cases, so I'm not ready to add a whole bzero() here
unless absolutely, positively, necessary.

Thanks.