Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

ethtool: Avoid overflowing userspace buffer on stats query

The ethtool -S command operates across three ioctl calls:
ETHTOOL_GSSET_INFO for the size, ETHTOOL_GSTRINGS for the names, and
ETHTOOL_GSTATS for the values.

If the number of stats changes between these calls (e.g., due to device
reconfiguration), userspace's buffer allocation will be incorrect,
potentially leading to buffer overflow.

Drivers are generally expected to maintain stable stat counts, but some
drivers (e.g., mlx5, bnx2x, bna, ksz884x) use dynamic counters, making
this scenario possible.

Some drivers try to handle this internally:
- bnad_get_ethtool_stats() returns early in case stats.n_stats is not
equal to the driver's stats count.
- micrel/ksz884x also makes sure not to write anything beyond
stats.n_stats and overflow the buffer.

However, both use stats.n_stats which is already assigned with the value
returned from get_sset_count(), hence won't solve the issue described
here.

Change ethtool_get_strings(), ethtool_get_stats(),
ethtool_get_phy_stats() to not return anything in case of a mismatch
between userspace's size and get_sset_size(), to prevent buffer
overflow.
The returned n_stats value will be equal to zero, to reflect that
nothing has been returned.

This could result in one of two cases when using upstream ethtool,
depending on when the size change is detected:
1. When detected in ethtool_get_strings():
# ethtool -S eth2
no stats available

2. When detected in get stats, all stats will be reported as zero.

Both cases are presumably transient, and a subsequent ethtool call
should succeed.

Other than the overflow avoidance, these two cases are very evident (no
output/cleared stats), which is arguably better than presenting
incorrect/shifted stats.
I also considered returning an error instead of a "silent" response, but
that seems more destructive towards userspace apps.

Notes:
- This patch does not claim to fix the inherent race, it only makes sure
that we do not overflow the userspace buffer, and makes for a more
predictable behavior.

- RTNL lock is held during each ioctl, the race window exists between
the separate ioctl calls when the lock is released.

- Userspace ethtool always fills stats.n_stats, but it is likely that
these stats ioctls are implemented in other userspace applications
which might not fill it. The added code checks that it's not zero,
to prevent any regressions.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
Signed-off-by: Gal Pressman <gal@nvidia.com>
Link: https://patch.msgid.link/20251208121901.3203692-1-gal@nvidia.com
Signed-off-by: Paolo Abeni <pabeni@redhat.com>

authored by

Gal Pressman and committed by
Paolo Abeni
7b07be1f 885bebac

+24 -6
+24 -6
net/ethtool/ioctl.c
··· 2383 2383 return -ENOMEM; 2384 2384 WARN_ON_ONCE(!ret); 2385 2385 2386 - gstrings.len = ret; 2386 + if (gstrings.len && gstrings.len != ret) 2387 + gstrings.len = 0; 2388 + else 2389 + gstrings.len = ret; 2387 2390 2388 2391 if (gstrings.len) { 2389 2392 data = vzalloc(array_size(gstrings.len, ETH_GSTRING_LEN)); ··· 2512 2509 if (copy_from_user(&stats, useraddr, sizeof(stats))) 2513 2510 return -EFAULT; 2514 2511 2515 - stats.n_stats = n_stats; 2512 + if (stats.n_stats && stats.n_stats != n_stats) 2513 + stats.n_stats = 0; 2514 + else 2515 + stats.n_stats = n_stats; 2516 2516 2517 - if (n_stats) { 2518 - data = vzalloc(array_size(n_stats, sizeof(u64))); 2517 + if (stats.n_stats) { 2518 + data = vzalloc(array_size(stats.n_stats, sizeof(u64))); 2519 2519 if (!data) 2520 2520 return -ENOMEM; 2521 2521 ops->get_ethtool_stats(dev, &stats, data); ··· 2530 2524 if (copy_to_user(useraddr, &stats, sizeof(stats))) 2531 2525 goto out; 2532 2526 useraddr += sizeof(stats); 2533 - if (n_stats && copy_to_user(useraddr, data, array_size(n_stats, sizeof(u64)))) 2527 + if (stats.n_stats && 2528 + copy_to_user(useraddr, data, 2529 + array_size(stats.n_stats, sizeof(u64)))) 2534 2530 goto out; 2535 2531 ret = 0; 2536 2532 ··· 2568 2560 return -EOPNOTSUPP; 2569 2561 2570 2562 n_stats = phy_ops->get_sset_count(phydev); 2563 + if (stats->n_stats && stats->n_stats != n_stats) { 2564 + stats->n_stats = 0; 2565 + return 0; 2566 + } 2571 2567 2572 2568 ret = ethtool_vzalloc_stats_array(n_stats, data); 2573 2569 if (ret) ··· 2592 2580 return -EOPNOTSUPP; 2593 2581 2594 2582 n_stats = ops->get_sset_count(dev, ETH_SS_PHY_STATS); 2583 + if (stats->n_stats && stats->n_stats != n_stats) { 2584 + stats->n_stats = 0; 2585 + return 0; 2586 + } 2595 2587 2596 2588 ret = ethtool_vzalloc_stats_array(n_stats, data); 2597 2589 if (ret) ··· 2632 2616 } 2633 2617 2634 2618 useraddr += sizeof(stats); 2635 - if (copy_to_user(useraddr, data, array_size(stats.n_stats, sizeof(u64)))) 2619 + if (stats.n_stats && 2620 + copy_to_user(useraddr, data, 2621 + array_size(stats.n_stats, sizeof(u64)))) 2636 2622 ret = -EFAULT; 2637 2623 2638 2624 out: