[PATCH iwl-net v2] ice: validate VF FDIR queue and queue region actions

[PATCH iwl-net v2] ice: validate VF FDIR queue and queue region actions

[Aleksandr Loktionov]

ice_vc_fdir_parse_action() copies the VF-supplied queue index and, for
VIRTCHNL_ACTION_Q_REGION, the queue region size straight into the Flow
Director filter.  Both end up in the programming descriptor (QINDEX and
TO_Q), which the hardware interprets relative to the VF's VSI, so a VF
can steer matched packets to queues beyond its own range, i.e. queues
belonging to another VF or to the PF.

For VIRTCHNL_ACTION_QUEUE reject an index that is not below the VSI's
Rx queue count.

For VIRTCHNL_ACTION_Q_REGION the filter covers 2^region queues starting
at the index.  Reject a region that does not fit the 3-bit TO_Q field
(it would be silently truncated) and require the whole range to fit
within the VSI's Rx queues.  No alignment of the index is required for
this to hold: however the hardware combines the index with the hash
bits, no queue it selects from the region is at or above
index + 2^region.

Fixes: 0ce332fd62f6 ("ice: Add FDIR pattern action parser for VF")
Fixes: 346bf2504397 ("ice: Add new actions support for VF FDIR")
Cc: stable@vger.kernel.org
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
v1 -> v2:
 - also bound queue.region and require index + 2^region <= num_rxq for
   VIRTCHNL_ACTION_Q_REGION (Simon Horman / Sashiko)
 - reword commit message to describe the upstream code path
 - retarget to iwl-net

 drivers/net/ethernet/intel/ice/virt/fdir.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/virt/fdir.c b/drivers/net/ethernet/intel/ice/virt/fdir.c
index 4f1f344..4456f49 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,
 	struct virtchnl_filter_action_set *as = &fltr->rule_cfg.action_set;
 	struct device *dev = ice_pf_to_dev(vf->pf);
 	struct ice_fdir_fltr *input = &conf->input;
+	struct ice_vsi *vsi;
 	u32 dest_num = 0;
 	u32 mark_num = 0;
 	int i;
@@ -1162,6 +1163,10 @@ ice_vc_fdir_parse_action(struct ice_vf *vf, struct virtchnl_fdir_add *fltr,
 		return -EINVAL;
 	}
 
+	vsi = ice_get_vf_vsi(vf);
+	if (!vsi)
+		return -EINVAL;
+
 	for (i = 0; i < as->count; i++) {
 		struct virtchnl_filter_action *action = &as->actions[i];
 
@@ -1176,11 +1181,25 @@ 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.region >
+			    FIELD_MAX(ICE_FXD_FLTR_QW0_TO_Q_M) ||
+			    action->act_conf.queue.index +
+			    BIT(action->act_conf.queue.region) > vsi->num_rxq) {
+				dev_dbg(dev, "Invalid queue region %u/%u for VF %d\n",
+					action->act_conf.queue.index,
+					action->act_conf.queue.region, 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;
-- 
2.52.0

Re: [PATCH iwl-net v2] ice: validate VF FDIR queue and queue region actions

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ice: memory leak of pkt_buf and prof in ice_vc_add_fdir_fltr error path
--

commit 6477a5eb13b0a0dff01249d4a775aff5939e63e5
Author: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

ice: validate VF FDIR queue and queue region actions

This commit validates the VF-supplied queue index and queue region in
ice_vc_fdir_parse_action(), rejecting invalid values to prevent a VF from
steering matched packets to queues beyond its own range.

> diff --git a/drivers/net/ethernet/intel/ice/virt/fdir.c b/drivers/net/ethernet/intel/ice/virt/fdir.c
> --- a/drivers/net/ethernet/intel/ice/virt/fdir.c
> +++ b/drivers/net/ethernet/intel/ice/virt/fdir.c
[ ... ]
> @@ -1176,11 +1181,25 @@ 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;
> +			}

[Severity: High]
This problem wasn't introduced by this patch, but does returning here leak
memory if conf->pkt_buf and conf->prof were previously allocated?

In ice_vc_add_fdir_fltr(), if ice_vc_fdir_parse_action() fails, it jumps to
err_free_conf which frees the outer structure but not the inner ones:

ice_vc_add_fdir_fltr() {
    ...
    ret = ice_vc_fdir_parse_action(vf, fltr, conf);
    if (ret) {
        ...
        goto err_free_conf;
    }
    ...
err_free_conf:
    devm_kfree(dev, conf);
    ...
}

Earlier in the request handling, if this was a raw flow,
ice_vc_fdir_parse_raw() allocates pkt_buf and prof and assigns them to the
conf structure:

ice_vc_fdir_parse_raw() {
    ...
    pkt_buf = kzalloc(pkt_len, GFP_KERNEL);
    ...
    conf->pkt_buf = pkt_buf;
    ...
}

Could an untrusted VF exhaust host memory by repeatedly sending requests
with invalid queue parameters that trigger this return path?

>  			input->dest_ctl = ICE_FLTR_PRGM_DESC_DEST_DIRECT_PKT_QINDEX;
>  			input->q_index = action->act_conf.queue.index;
>  			break;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013144435.1406126-1-aleksandr.loktionov@intel.com?part=1