Re: [PATCH net] ice: validate FDIR action queue index against VF VSI queue count
From: Simon Horman <horms@kernel.org>
Date: 2026-06-02 15:51:09
Also in:
intel-wired-lan
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.
quoted hunk ↗ jump to hunk
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,
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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)?