This series fixes a potential NULL dereference in ethtool_get_phy_stats
while also attempting to refactor/split said function into multiple
helpers so that it's easier to reason about what's going on.
I've taken Andrew Lunn's suggestions on the previous version of this
patch and added a bit of my own.
Changes since v1:
- Remove an extra newline in the first patch
- Move WARN_ON_ONCE into the if check as it already returns the
result of the comparison
- Actually split ethtool_get_phy_stats instead of attempting to
refactor it
Daniil Tatianin (3):
net/ethtool/ioctl: return -EOPNOTSUPP if we have no phy stats
net/ethtool/ioctl: remove if n_stats checks from ethtool_get_phy_stats
net/ethtool/ioctl: split ethtool_get_phy_stats into multiple helpers
net/ethtool/ioctl.c | 107 +++++++++++++++++++++++++++++---------------
1 file changed, 70 insertions(+), 37 deletions(-)
--
2.25.1
Now that we always early return if we don't have any stats we can remove
these checks as they're no longer necessary.
Signed-off-by: Daniil Tatianin <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
net/ethtool/ioctl.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index e8a294b38b7b..3379af21c29f 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2101,28 +2101,24 @@ static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr)
stats.n_stats = n_stats;
- if (n_stats) {
- data = vzalloc(array_size(n_stats, sizeof(u64)));
- if (!data)
- return -ENOMEM;
+ data = vzalloc(array_size(n_stats, sizeof(u64)));
+ if (!data)
+ return -ENOMEM;
- if (phydev && !ops->get_ethtool_phy_stats &&
- phy_ops && phy_ops->get_stats) {
- ret = phy_ops->get_stats(phydev, &stats, data);
- if (ret < 0)
- goto out;
- } else {
- ops->get_ethtool_phy_stats(dev, &stats, data);
- }
+ if (phydev && !ops->get_ethtool_phy_stats &&
+ phy_ops && phy_ops->get_stats) {
+ ret = phy_ops->get_stats(phydev, &stats, data);
+ if (ret < 0)
+ goto out;
} else {
- data = NULL;
+ ops->get_ethtool_phy_stats(dev, &stats, data);
}
ret = -EFAULT;
if (copy_to_user(useraddr, &stats, sizeof(stats)))
goto out;
useraddr += sizeof(stats);
- if (n_stats && copy_to_user(useraddr, data, array_size(n_stats, sizeof(u64))))
+ if (copy_to_user(useraddr, data, array_size(n_stats, sizeof(u64))))
goto out;
ret = 0;
--
2.25.1
It's not very useful to copy back an empty ethtool_stats struct and
return 0 if we didn't actually have any stats. This also allows for
further simplification of this function in the future commits.
Signed-off-by: Daniil Tatianin <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
net/ethtool/ioctl.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 57e7238a4136..e8a294b38b7b 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2093,7 +2093,8 @@ static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr)
return n_stats;
if (n_stats > S32_MAX / sizeof(u64))
return -ENOMEM;
- WARN_ON_ONCE(!n_stats);
+ if (WARN_ON_ONCE(!n_stats))
+ return -EOPNOTSUPP;
if (copy_from_user(&stats, useraddr, sizeof(stats)))
return -EFAULT;
--
2.25.1
Hello:
This series was applied to netdev/net.git (master)
by David S. Miller <[email protected]>:
On Mon, 26 Dec 2022 14:48:22 +0300 you wrote:
> This series fixes a potential NULL dereference in ethtool_get_phy_stats
> while also attempting to refactor/split said function into multiple
> helpers so that it's easier to reason about what's going on.
>
> I've taken Andrew Lunn's suggestions on the previous version of this
> patch and added a bit of my own.
>
> [...]
Here is the summary with links:
- [net,v2,1/3] net/ethtool/ioctl: return -EOPNOTSUPP if we have no phy stats
https://git.kernel.org/netdev/net/c/9deb1e9fb88b
- [net,v2,2/3] net/ethtool/ioctl: remove if n_stats checks from ethtool_get_phy_stats
https://git.kernel.org/netdev/net/c/fd4778581d61
- [net,v2,3/3] net/ethtool/ioctl: split ethtool_get_phy_stats into multiple helpers
https://git.kernel.org/netdev/net/c/201ed315f967
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html