Re: [PATCH v1 iwl-net] iavf: fix out-of-bounds writes in iavf_get_ethtool_stats()

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 2/14/26 20:14, Kohei Enju wrote:
> iavf incorrectly uses real_num_tx_queues for ETH_SS_STATS. Since the
> value could change in runtime, we should use num_tx_queues instead.
> 

[...]

> Use immutable num_tx_queues in all related functions to avoid the issue.
> 
> [1]
>   BUG: KASAN: vmalloc-out-of-bounds in iavf_add_one_ethtool_stat+0x200/0x270

[...]

Thank you very much Enju-san,
this is a nice improvement!
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

Side note: I'm working on enabling 256 queues for iavf, what means
changing the reported max number of queues, what finally means screens
of zero stats printed, unfortunate, but better be correct that pretty ;)

> 
> Fixes: 64430f70ba6f ("iavf: Fix displaying queue statistics shown by ethtool")
> Signed-off-by: Kohei Enju <kohei@enjuk.jp>
> ---
>   .../net/ethernet/intel/iavf/iavf_ethtool.c    | 31 +++++++++----------
>   1 file changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> index 6ff3842a1ff1..98bec3afc200 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> @@ -313,14 +313,13 @@ static int iavf_get_sset_count(struct net_device *netdev, int sset)
>   {
>   	/* Report the maximum number queues, even if not every queue is
>   	 * currently configured. Since allocation of queues is in pairs,
> -	 * use netdev->real_num_tx_queues * 2. The real_num_tx_queues is set
> -	 * at device creation and never changes.
> +	 * use netdev->num_tx_queues * 2. The num_tx_queues is set at
> +	 * device creation and never changes.
>   	 */
>   
>   	if (sset == ETH_SS_STATS)
>   		return IAVF_STATS_LEN +
> -			(IAVF_QUEUE_STATS_LEN * 2 *
> -			 netdev->real_num_tx_queues);
> +		       (IAVF_QUEUE_STATS_LEN * 2 * netdev->num_tx_queues);
>   	else
>   		return -EINVAL;
>   }
> @@ -345,19 +344,19 @@ static void iavf_get_ethtool_stats(struct net_device *netdev,
>   	iavf_add_ethtool_stats(&data, adapter, iavf_gstrings_stats);
>   
>   	rcu_read_lock();
> -	/* As num_active_queues describe both tx and rx queues, we can use
> -	 * it to iterate over rings' stats.
> +	/* Use num_tx_queues to report stats for the maximum number of queues.
> +	 * Queues beyond num_active_queues will report zero.

<rambling>
zeroing inactive stats is bad behavior, not introduced by you of course, 
but worth to fix (as a followup, not necessarily by you)

unrelated to the prev paragraph,
even if we remove explicit = 0 from the driver, ethtool userspace will
still print (lots of) zeroes for "under-reported" stats, thanks to
multi-syscall stats retrieval, that could be "fixed" in ethtool, but
perhaps does not matter, as users use wrappers anyway
</rambling>

>   	 */
> -	for (i = 0; i < adapter->num_active_queues; i++) {
> -		struct iavf_ring *ring;
> +	for (i = 0; i < netdev->num_tx_queues; i++) {
> +		struct iavf_ring *tx_ring = NULL, *rx_ring = NULL;
>   
> -		/* Tx rings stats */
> -		ring = &adapter->tx_rings[i];
> -		iavf_add_queue_stats(&data, ring);
> +		if (i < adapter->num_active_queues) {
> +			tx_ring = &adapter->tx_rings[i];
> +			rx_ring = &adapter->rx_rings[i];
> +		}
>   
> -		/* Rx rings stats */
> -		ring = &adapter->rx_rings[i];
> -		iavf_add_queue_stats(&data, ring);
> +		iavf_add_queue_stats(&data, tx_ring);
> +		iavf_add_queue_stats(&data, rx_ring);
>   	}
>   	rcu_read_unlock();
>   }
> @@ -376,9 +375,9 @@ static void iavf_get_stat_strings(struct net_device *netdev, u8 *data)
>   	iavf_add_stat_strings(&data, iavf_gstrings_stats);
>   
>   	/* Queues are always allocated in pairs, so we just use
> -	 * real_num_tx_queues for both Tx and Rx queues.
> +	 * num_tx_queues for both Tx and Rx queues.
>   	 */
> -	for (i = 0; i < netdev->real_num_tx_queues; i++) {
> +	for (i = 0; i < netdev->num_tx_queues; i++) {
>   		iavf_add_stat_strings(&data, iavf_gstrings_queue_stats,
>   				      "tx", i);
>   		iavf_add_stat_strings(&data, iavf_gstrings_queue_stats,