Re: [PATCH net] ice: validate FDIR action queue index against VF VSI queue count

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@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.
Full review at: https://sashiko.dev
---
ice: validate FDIR action queue index against VF VSI queue count

This patch adds a bounds check on the VF-supplied queue index in
ice_vc_fdir_parse_action() against vsi->num_rxq for both
VIRTCHNL_ACTION_QUEUE and VIRTCHNL_ACTION_Q_REGION, so a VF cannot
direct matched FDIR traffic to queues outside its own VSI.

> diff --git a/drivers/net/ethernet/intel/ice/virt/fdir.c b/drivers/net/ethernet/intel/ice/virt/fdir.c
> index 4f1f3442e52c..1bc524f3616c 100644
> --- a/drivers/net/ethernet/intel/ice/virt/fdir.c
> +++ b/drivers/net/ethernet/intel/ice/virt/fdir.c
> @@ -1152,6 +1152,7 @@ ice_vc_fdir_parse_action(struct ice_vf *vf, struct virtchnl_fdir_add *fltr,

[ ... ]

> @@ -1176,11 +1181,21 @@ ice_vc_fdir_parse_action(struct ice_vf *vf, struct virtchnl_fdir_add *fltr,
>  			break;
>  		case VIRTCHNL_ACTION_QUEUE:
>  			dest_num++;
> +			if (action->act_conf.queue.index >= vsi->num_rxq) {
> +				dev_dbg(dev, "Invalid queue index %u for VF %d\n",
> +					action->act_conf.queue.index, vf->vf_id);
> +				return -EINVAL;
> +			}
>  			input->dest_ctl = ICE_FLTR_PRGM_DESC_DEST_DIRECT_PKT_QINDEX;
>  			input->q_index = action->act_conf.queue.index;
>  			break;
>  		case VIRTCHNL_ACTION_Q_REGION:
>  			dest_num++;
> +			if (action->act_conf.queue.index >= vsi->num_rxq) {
> +				dev_dbg(dev, "Invalid queue index %u for VF %d\n",
> +					action->act_conf.queue.index, vf->vf_id);
> +				return -EINVAL;
> +			}
>  			input->dest_ctl = ICE_FLTR_PRGM_DESC_DEST_DIRECT_PKT_QGROUP;
>  			input->q_index = action->act_conf.queue.index;
>  			input->q_region = action->act_conf.queue.region;

[High]
For the VIRTCHNL_ACTION_Q_REGION case, does this check fully close the
steering hole the commit message describes?

The commit message states that the patch validates the action queue
index for both action types, but the Q_REGION action targets a range
of 2^q_region consecutive queues starting at queue.index, and
action->act_conf.queue.region is still copied verbatim into
input->q_region:

	input->q_index = action->act_conf.queue.index;
	input->q_region = action->act_conf.queue.region;

ice_fdir_fltr documents q_region as:

	/* queue region size (=2^q_region) */
	u8 q_region;

and ice_fdir_get_prgm_desc() programs it into the descriptor's TO_Q
field for the QGROUP destination:

	if (input->dest_ctl ==
	    ICE_FLTR_PRGM_DESC_DEST_DIRECT_PKT_QGROUP)
		fdir_fltr_ctx.toq = input->q_region;
	fdir_fltr_ctx.drop = ICE_FXD_FLTR_QW0_DROP_NO;
	fdir_fltr_ctx.qindex = input->q_index;

ICE_FXD_FLTR_QW0_TO_Q_M is a 3-bit field (0x7), so q_region can be up
to 7, i.e. up to 128 queues starting at q_index.

With the new check, a VF can still set queue.index = vsi->num_rxq - 1
(which passes "index >= vsi->num_rxq") together with queue.region up
to 7, causing the hardware to spread matched traffic across queues
beginning at the last valid VF queue and extending past the end of
the VF's VSI into queues owned by another VF or the PF.

Should the Q_REGION case additionally bound queue.region and verify
that queue.index + (1 << queue.region) <= vsi->num_rxq (and possibly
require queue.index to be aligned to 2^queue.region as is conventional
for hardware queue groups)?