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

[Kohei Enju <kohei@enjuk.jp>]

On Wed, 18 Feb 2026 06:39:43 +0100, Przemek Kitszel wrote:

> 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

Cool, and thank you for reviewing!

> changing the reported max number of queues, what finally means screens
> of zero stats printed, unfortunate, but better be correct that pretty ;)

Indeed.

> 
> > 
> > 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)

I agree.

> 
> 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>

The root cause of the OOB this time is that iavf_get_sset_count() uses
real_num_tx_queues for allocating buffers, and iavf_get_ethtool_stats()
uses num_active_queues for writing [1].

This may warrant more thought, but I was thinking that using
num_active_queues in all related functions would also be acceptable.

That's because, thanks to netdev_lock(), we can ensure num_active_queues
is unchanged within each ioctl call (ETHTOOL_GSSET_INFO,
ETHTOOL_GSTRINGS, ETHTOOL_GSTATS).
In other words, num_active_queues is effectively immutable for the
duration of a single ioctl, avoiding OOB.

[1] 
Thread 1 (ethtool -L)       Thread 2 (work)        Thread 3 (ethtool -S)
netdev_lock()
iavf_set_channels()
...
iavf_alloc_queues()
-> alloc {tx,rx}_rings
-> num_active_queues = 8
iavf_schedule_finish_config()
netdev_unlock()
                                                   netdev_lock()
                                                   iavf_get_sset_count()
                                                   real_num_tx_queues: 1
                                                   -> buffer for 1 queue
                                                   iavf_get_ethtool_stats()
                                                   num_active_queues: 8
                                                   -> out-of-bounds!
                                                   netdev_unlock()
                            iavf_finish_config()
                            netdev_lock()
                            -> real_num_tx_queues = 8
                            netdev_unlock()