Re: [PATCH net 2/8] ice: fix stats array overflow when VF requests more queues

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ice: fix stats array overflow when VF requests more queues

When a VF increases its queue count via VIRTCHNL_OP_REQUEST_QUEUES,
ice_vc_request_qs_msg() sets vf->num_req_qs and triggers a VF reset.
The reset calls ice_vf_reconfig_vsi(), which does ice_vsi_decfg()
followed by ice_vsi_cfg(). This patch fixes a stats array overflow by
calling ice_vsi_realloc_stat_arrays() in ice_vf_reconfig_vsi() before
ice_vsi_decfg().

> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index 837b71b7b2b7c..fc78176a2a8d0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -3015,7 +3015,7 @@ ice_vsi_rebuild_set_coalesce(struct ice_vsi *vsi,
>   * ice_vsi_realloc_stat_arrays - Frees unused stat structures or alloc new ones
>   * @vsi: VSI pointer
>   */
> -static int
> +int
>  ice_vsi_realloc_stat_arrays(struct ice_vsi *vsi)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but making this function public and
using it in more paths exposes a potential NULL pointer dereference.

Does this code safely handle ADQ channel VSIs?

For ADQ channel VSIs (where vsi->type == ICE_VSI_CHNL),
ice_vsi_alloc_stat_arrays() bypasses stats allocation, leaving
pf->vsi_stats[vsi->idx] as NULL.

During a PF reset (ICE_RESET_PFR), ice_rebuild_channels() iterates over
ICE_VSI_CHNL VSIs and calls ice_vsi_rebuild(), which triggers
ice_vsi_realloc_stat_arrays().

This will unconditionally dereference vsi_stat:

drivers/net/ethernet/intel/ice/ice_lib.c:ice_vsi_realloc_stat_arrays() {
	...
	vsi_stat = pf->vsi_stats[vsi->idx];
	
	if (req_txq < prev_txq) {
		for (i = req_txq; i < prev_txq; i++) {
			if (vsi_stat->tx_ring_stats[i]) {
				...

Can this result in a NULL pointer dereference when rebuilding channel VSIs?

[Severity: High]
This is another pre-existing issue, but the error handling when reallocating
arrays might lead to out-of-bounds reads and memory corruption.

When a VF requests fewer queues (req_txq < prev_txq), the function successfully
shrinks vsi_stat->tx_ring_stats using krealloc_array().

However, if the subsequent reallocation for rx_ring_stats fails (e.g., due to
memory pressure returning -ENOMEM), the function returns an error and aborts
before vsi->alloc_txq is updated to the new smaller size:

drivers/net/ethernet/intel/ice/ice_lib.c:ice_vsi_realloc_stat_arrays() {
	...
	vsi_stat->tx_ring_stats = krealloc_array(...);
	if (!vsi_stat->tx_ring_stats) { ... }

	...
	vsi_stat->rx_ring_stats = krealloc_array(...);
	if (!vsi_stat->rx_ring_stats) {
		vsi_stat->rx_ring_stats = rx_ring_stats;
		return -ENOMEM;
	}
	...
}

Later, during VSI teardown in ice_vsi_free_stats(), ice_for_each_alloc_txq(vsi, i)
iterates up to the stale, larger vsi->alloc_txq value.

Is it possible for this to read past the bounds of the newly shrunk
tx_ring_stats array and pass non-zero garbage to kfree_rcu()?