Re: [PATCH net-next v5 4/8] net: macb: use the current queue number for stats

[Paolo Valerio <pvalerio@redhat.com>]

Hi Nicolai,

thanks for the reviews.

On 16 Mar 2026 at 05:30:48 PM, Nicolai Buchwitz <nb@tipi-net.de> wrote:

> On 13.3.2026 21:14, Paolo Valerio wrote:
>> gem_get_ethtool_stats calculates the size of the statistics
>> data to copy always considering maximum number of queues.
>> 
>> The patch makes sure the statistics are copied only for the
>> active queues as returned in the string set count op.
>> 
>> Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
>> ---
>>  drivers/net/ethernet/cadence/macb_main.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c 
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index 06ad8c8ec036..fbeaa85b4a9c 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -3528,7 +3528,7 @@ static void gem_get_ethtool_stats(struct 
>> net_device *dev,
>>  	spin_lock_irq(&bp->stats_lock);
>>  	gem_update_stats(bp);
>>  	memcpy(data, &bp->ethtool_stats, sizeof(u64)
>> -			* (GEM_STATS_LEN + QUEUE_STATS_LEN * MACB_MAX_QUEUES));
>> +			* (GEM_STATS_LEN + QUEUE_STATS_LEN * bp->num_queues));
>
> This is an out-of-bounds write, not just a cosmetic change.

yes, the commit message is not perfectly phrased (and as you
noted it lacks the Fixes tag), but this was not intended as a cosmetic
change. I noticed the problem while adding page pool statistics to
ethtool (later removed) and they were corrupted because of this.

> gem_get_sset_count() returns GEM_STATS_LEN + QUEUE_STATS_LEN *
> bp->num_queues, and ethtool allocates the data buffer based on that 
> count.
> The old memcpy with MACB_MAX_QUEUES (8) writes past the end of the
> buffer on any GEM instance with fewer than 8 hardware queues.
>
> KASAN confirms on RP1 (1 queue) without this patch applied:
>
>    BUG: KASAN: vmalloc-out-of-bounds in gem_get_ethtool_stats+0x50/0x78
>    Write of size 760 at addr ffffffc0822e7000 by task ethtool/922
>
> The overflow stays within the vzalloc page slack, so the practical
> impact is low - but it's still an out-of-bounds write that exists in
> the current upstream code. Might be worth splitting this out as a
> standalone fix targeting net with a Fixes: tag, and updating the commit
> message accordingly?
>

makes sense. Will do.

Thanks!

>>  	spin_unlock_irq(&bp->stats_lock);
>>  }
>
> Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
>
> Thanks
> Nicolai