Re: [PATCH net 06/10] ice: fix stats array overflow via proper realloc

[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 via proper realloc

This drops ice_vsi_alloc_stat_arrays() and makes
ice_vsi_realloc_stat_arrays() handle the initial allocation as well, so
ice_vsi_cfg_def() now goes through the realloc path.  The array sizes are
tracked in two new fields, tx_ring_stats_len and rx_ring_stats_len, which
the free path uses instead of vsi->alloc_txq/alloc_rxq.

Closes: https://redhat.atlassian.net/browse/RHEL-164321

> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index fc91b6665f905..83da5ba6eeb17 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -328,6 +328,8 @@ enum ice_vsi_state {
>  struct ice_vsi_stats {
>  	struct ice_ring_stats **tx_ring_stats;  /* Tx ring stats array */
>  	struct ice_ring_stats **rx_ring_stats;  /* Rx ring stats array */
> +	u16 tx_ring_stats_len;
> +	u16 rx_ring_stats_len;
>  };
>  
>  /* struct that defines a VSI, associated with a dev */

> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index e48ee5940f173..c71849119d04e 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c

[ ... ]

> @@ -513,51 +512,6 @@ static irqreturn_t ice_msix_clean_rings(int __always_unused irq, void *data)

[ ... ]

> -	vsi_stat->tx_ring_stats =
> -		kzalloc_objs(*vsi_stat->tx_ring_stats, vsi->alloc_txq);
> -	if (!vsi_stat->tx_ring_stats)
> -		goto err_alloc_tx;
> -
> -	vsi_stat->rx_ring_stats =
> -		kzalloc_objs(*vsi_stat->rx_ring_stats, vsi->alloc_rxq);
> -	if (!vsi_stat->rx_ring_stats)
> -		goto err_alloc_rx;

[ ... ]

> @@ -2316,11 +2270,19 @@ static int ice_vsi_realloc_stat_arrays(struct ice_vsi *vsi)
>  	struct ice_ring_stats **rx_ring_stats;
>  	struct ice_vsi_stats *vsi_stat;
>  	struct ice_pf *pf = vsi->back;
> -	u16 prev_txq = vsi->alloc_txq;
> -	u16 prev_rxq = vsi->alloc_rxq;
> +	u16 prev_txq, prev_rxq;
> +
> +	if (vsi->type == ICE_VSI_CHNL)
> +		return 0;
>  
>  	vsi_stat = pf->vsi_stats[vsi->idx];
> +	if (!vsi_stat) {
> +		vsi_stat = kzalloc_obj(*vsi_stat);
> +		if (!vsi_stat)
> +			return -ENOMEM;
> +	}
>  
> +	prev_txq = vsi_stat->tx_ring_stats_len;
>  	if (req_txq < prev_txq) {
>  		for (int i = req_txq; i < prev_txq; i++) {
>  			if (vsi_stat->tx_ring_stats[i]) {

[Severity: High]

This isn't a bug introduced by this patch, but can the array still end up
smaller than what the consumers iterate for a VF VSI?  The replaced
ice_vsi_alloc_stat_arrays() sized the arrays from vsi->alloc_txq /
vsi->alloc_rxq, while ice_vsi_realloc_stat_arrays() sizes them from the
req_* values:

drivers/net/ethernet/intel/ice/ice_lib.c:ice_vsi_realloc_stat_arrays() {
	u16 req_txq = vsi->req_txq ? vsi->req_txq : vsi->alloc_txq;
	u16 req_rxq = vsi->req_rxq ? vsi->req_rxq : vsi->alloc_rxq;
	...
}

For ICE_VSI_VF, ice_vsi_set_num_qs() does not consult vsi->req_txq /
vsi->req_rxq at all:

	case ICE_VSI_VF:
		if (vf->num_req_qs)
			vf->num_vf_qs = vf->num_req_qs;
		vsi->alloc_txq = vf->num_vf_qs;
		vsi->alloc_rxq = vf->num_vf_qs;

So the two counts have independent writers.  vsi->req_txq / vsi->req_rxq
are set on a VF VSI only by ice_sriov_set_msix_vec_count() (the
sriov_vf_msix_count sysfs write) and I could not find anything that clears
them again for a VF VSI -- neither ice_vsi_decfg() nor ice_vsi_alloc_def()
touches them, and the only vsi->req_txq = 0 is on the PF mqprio path in
ice_main.c.

Given that, does this sequence still overflow tx_ring_stats[] /
rx_ring_stats[]?

1) host writes sriov_vf_msix_count, so ice_sriov_set_msix_vec_count() sets
   vsi->req_txq = vsi->req_rxq = 2 and they stay at 2
2) the guest sends VIRTCHNL_OP_REQUEST_QUEUES for 16 queues;
   ice_vc_request_qs_msg() bounds it only by ICE_MAX_RSS_QS_PER_VF and the
   free queue count, sets vf->num_req_qs and calls ice_reset_vf()
3) ice_reset_vf() -> ice_vf_reconfig_vsi() -> ice_vsi_cfg() ->
   ice_vsi_cfg_def(); ice_vsi_alloc_def() -> ice_vsi_set_num_qs() sets
   alloc_txq = alloc_rxq = 16
4) ice_vsi_realloc_stat_arrays() then shrinks to req_txq = 2 and records
   tx_ring_stats_len = 2

ice_vsi_alloc_ring_stats() still walks 0..alloc_txq-1 with no bound check
against the new length fields:

drivers/net/ethernet/intel/ice/ice_lib.c:ice_vsi_alloc_ring_stats() {
	ice_for_each_alloc_txq(vsi, i) {
		...
		ring_stats = tx_ring_stats[i];
		if (!ring_stats) {
			...
			WRITE_ONCE(tx_ring_stats[i], ring_stats);
		}
		ring->ring_stats = ring_stats;
	}
}

That is both an out-of-bounds read and an out-of-bounds pointer store for
i >= 2, which looks like the same slab-out-of-bounds signature quoted in
the commit message.  ice_qp_reset_stats() in ice_base.c indexes
vsi_stat->tx_ring_stats[q_idx] / rx_ring_stats[q_idx] the same way without
consulting the new lengths.

Would using the freshly computed vsi->alloc_txq / vsi->alloc_rxq (or
max(req_*, alloc_*)) for the sizing close this?  The req_* preference looks
only meaningful for the pre-ice_vsi_cfg_def() call in ice_vsi_rebuild(),
where alloc_* is still stale.

>  
> @@ -2379,7 +2353,7 @@ static int ice_vsi_cfg_def(struct ice_vsi *vsi)
>  		return ret;
>  
>  	/* allocate memory for Tx/Rx ring stat pointers */
> -	ret = ice_vsi_alloc_stat_arrays(vsi);
> +	ret = ice_vsi_realloc_stat_arrays(vsi);
>  	if (ret)
>  		goto unroll_vsi_alloc;
>