Re: [PATCH net-next 02/10] cxgb4: flower: refuse a tunnel key wider than the VNI

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] cxgb4: unvalidated tunnel key mask in
  cxgb4_validate_flow_match()
- [Low] The commit message says the bug is already reachable today: "An
  exact key above bit 23 already selects the wrong VNI".

Pre-existing issues:
- [Medium] This bug was not introduced by this patch, but it produces the
  same symptom the commit message gives as its motivation ("The rule…

--

cxgb4: flower: refuse a tunnel key wider than the VNI

This patch adds a check to cxgb4_validate_flow_match(). Flower rules whose
enc_key_id key has bits set above bit 23 are now rejected. Before this, the
driver silently truncated such keys into the 24 bit vni bitfield.

> An exact key above bit 23 already selects the wrong VNI.

[Severity: Low]
The commit message says the truncation can already be hit today. Should
this carry a Fixes: tag, and possibly go through net?

At the baseline, cls_flower parses TCA_FLOWER_KEY_ENC_KEY_ID with
TCA_FLOWER_UNSPEC as the mask, so the mask is all ones. After that,
cxgb4_process_flow_match() stores the key and the mask into the 24 bit
fields:

drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c:cxgb4_process_flow_match() {
    ...
		fs->val.vni = be32_to_cpu(match.key->keyid);
		fs->mask.vni = be32_to_cpu(match.mask->keyid);
		if (fs->mask.vni) {
			fs->val.encap_vld = 1;
			fs->mask.encap_vld = 1;
		}
    ...
}

The field is declared as uint32_t vni:ENCAP_VNI_BITWIDTH in struct
ch_filter_tuple. So enc_key_id 0x01000005 becomes vni 0x5 with a vni mask
of 0xffffff. The rule's action then applies to VNI 5 traffic.

The truncation seems to date back to:

Fixes: 98f3697f8d41 ("cxgb4: add tc flower match support for tunnel VNI")

No later patch in the series touches drivers/net/ethernet/chelsio/, so
nothing else adds the tag.

> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> index 3307e50426819..d9a509be5c517 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> @@ -316,6 +316,17 @@ static int cxgb4_validate_flow_match(struct netlink_ext_ack *extack,
>  		return -EOPNOTSUPP;
>  	}
>  
> +	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_KEYID)) {
> +		struct flow_match_enc_keyid match;
> +
> +		flow_rule_match_enc_keyid(rule, &match);
> +		if (be32_to_cpu(match.key->keyid) >> ENCAP_VNI_BITWIDTH) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Tunnel key does not fit the 24 bit VNI");
> +			return -EOPNOTSUPP;
> +		}
> +	}

[Severity: Medium]
Does this check cover the case the commit message describes? It only
looks at match.key->keyid. The hazard named in the changelog is a mask
that only covers bits above 23.

cls_flower stores the key already ANDed with the mask. So a rule with
enc_key_id 0 and a mask of 0xff000000 reaches
cxgb4_validate_flow_match() with match.key->keyid == 0 and
match.mask->keyid == cpu_to_be32(0xff000000). Shifting the key gives 0,
and the rule is accepted.

cxgb4_process_flow_match() then does:

	fs->mask.vni = be32_to_cpu(match.mask->keyid);

This truncates into the 24 bit vni bitfield of struct
ch_filter_tuple and leaves fs->mask.vni == 0. The if (fs->mask.vni)
block is skipped, so fs->val.encap_vld and fs->mask.encap_vld stay 0.
__cxgb4_set_filter() then never reaches t4_alloc_encap_mac_filt().

The hardware filter ends up with no tunnel constraint at all. It
matches any traffic its other keys allow, including non-encapsulated
packets. That is the failure mode the commit message says this patch
prevents, and it opens up once the later patch in the series lets
flower accept partial enc_key_id masks.

Simply testing match.mask->keyid >> ENCAP_VNI_BITWIDTH would not work
either. Exact matches today arrive with an all ones mask from
TCA_FLOWER_UNSPEC, so every existing rule would be rejected.

Would it make sense to also refuse a non-zero mask whose low
ENCAP_VNI_BITWIDTH bits are all zero? Or is there a better way to
express "match any VNI" for this hardware?

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. It
causes the same symptom the commit message names ("The rule would match
any traffic its other keys allow"), but it depends on the firmware
configuration.

validate_filter() in cxgb4_filter.c checks encap_vld only against fconf
VNIC_ID_F and the chip version:

	    unsupported(fconf, VNIC_ID_F, fs->val.encap_vld,
			fs->mask.encap_vld) ||
    ...
	if (fs->val.encap_vld &&
	    CHELSIO_CHIP_VERSION(adapter->params.chip) < CHELSIO_T6)
		return -EOPNOTSUPP;

It never checks that iconf (TP_INGRESS_CONFIG) has USE_ENC_IDX_F set.

In __cxgb4_set_filter(), the VNI is programmed only in the USE_ENC_IDX_F
branch:

	iconf = adapter->params.tp.ingress_config;
	if (iconf & VNIC_F) {
		f->fs.val.ovlan = (fs->val.pf << 13) | fs->val.vf;
		f->fs.mask.ovlan = (fs->mask.pf << 13) | fs->mask.vf;
		f->fs.val.ovlan_vld = fs->val.pfvf_vld;
		f->fs.mask.ovlan_vld = fs->mask.pfvf_vld;
	} else if (iconf & USE_ENC_IDX_F) {
		if (f->fs.val.encap_vld) {
			...
			ret = t4_alloc_encap_mac_filt(adapter, pi->viid,
			...

If iconf has VNIC_F, ovlan_vld is overwritten with pfvf_vld, which is 0
for a flower rule. If iconf has neither bit, nothing is programmed.

set_filter_wr() has no VNI field, so tunnel information reaches hardware
only through ovlan/ovlan_vld. In both cases the FW_FILTER_WR is sent
without any VNI constraint.

On a T6 with such a firmware config, could a rule like "flower skip_sw
enc_key_id 5 action drop" be reported as offloaded with a VNI match, but
drop all ingress traffic on the port that matches its other keys?

Flower doesn't reach the hash filter path for these rules, since
is_filter_exact_match() returns false when fs->mask.encap_vld is set.

One possible fix would be for validate_filter() to reject encap_vld
unless (iconf & USE_ENC_IDX_F).

No later patch in the series touches drivers/net/ethernet/chelsio/.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109131039.959438-1-acassen%40corp.free.fr