Re: [PATCH net-next v3 1/6] vxlan: vnifilter: validate the VXLAN_VNIFILTER_ENTRY nest

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [High] The commit message says more than the patch does.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, and later patches
  in the series fix it (baseline decision: discard).

--

vxlan: vnifilter: validate the VXLAN_VNIFILTER_ENTRY nest

This patch links the VXLAN_VNIFILTER_ENTRY nest in vni_filter_policy to
vni_filter_entry_policy with NLA_POLICY_NESTED(). The entry attributes are
then checked by nlmsg_parse() in vxlan_vnifilter_process() before any entry
is dispatched. The goal is to reject a message that carries an invalid
entry as a whole.

Link: https://lore.kernel.org/netdev/20260921150904.65a704eb@kernel.org/

> Link the nest to vni_filter_entry_policy with NLA_POLICY_NESTED() so the
> whole message is validated up front, before any entry is acted on. A
> message carrying an invalid entry is now rejected as a unit and installs
> nothing.

[Severity: High]
Is this claim accurate? NLA_POLICY_NESTED() moves only the attribute
policy checks (type, length, unknown attributes) ahead of dispatch. Two
other checks that make an entry invalid still run one entry at a time in
vxlan_process_vni_filter():

drivers/net/vxlan/vxlan_vnifilter.c:vxlan_process_vni_filter() {
    ...
	if (!vni_start && !vni_end) {
		NL_SET_ERR_MSG_ATTR(extack, nlvnifilter,
				    "vni start nor end found in vni entry");
		return -EINVAL;
	}
    ...
	if (vxlan_addr_multicast(&group) && !vxlan->default_dst.remote_ifindex) {
		NL_SET_ERR_MSG(extack,
			       "Local interface required for multicast remote group");

		return -EINVAL;
	}
    ...
}

An empty VXLAN_VNIFILTER_ENTRY nest gets through the new policy, because
validate_nla() returns before it applies the nested policy:

lib/nlattr.c:validate_nla() {
    ...
	case NLA_NESTED:
		/* a nested attributes is allowed to be empty; if its not,
		 * it must have a size of at least NLA_HDRLEN.
		 */
		if (attrlen == 0)
			break;
    ...
}

An entry with only START=0, or with only GROUP, also gets through.

vxlan_vnifilter_process() applies entries in order, stops at the first
error and does not roll anything back:

	nlmsg_for_each_attr_type(attr, VXLAN_VNIFILTER_ENTRY, nlh,
				 sizeof(*tmsg), rem) {
		err = vxlan_process_vni_filter(vxlan, attr, nlh->nlmsg_type,
					       extack);
		vnis++;
		if (err)
			break;
	}

Take an RTM_NEWTUNNEL to a vnifilter device with entry1 {START=100} and
entry2 {}. Entry2 could also be {START=0}, or {START=200, GROUP=239.1.1.1}
on a device with no remote_ifindex. That message passes nlmsg_parse().
Then:

vxlan_vnifilter_process()
  vxlan_process_vni_filter(entry1)
    vxlan_vni_add_del()
      vxlan_vni_add()
        inserts VNI 100, vxlan_vnifilter_notify(..., RTM_NEWTUNNEL)
  vxlan_process_vni_filter(entry2)
    returns -EINVAL

RTM_DELTUNNEL works the same way: earlier deletes are applied and notified.

Isn't this the same "first entry valid, second invalid" case the commit
message says is now fixed?

Later in the series, the vxlan_vnifilter_check_msg() pre-pass from "vxlan:
vnifilter: bound the number of VNIs one request may touch" counts an empty
entry as one VNI. It does neither of these checks, so the case can still
happen at the end of the series.

Should the commit message say that only policy-level invalid entries are
now all-or-nothing? The alternative is to run the START/END presence check
and the multicast/remote_ifindex check over every entry before any entry is
applied.

> diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
> index dd94085e08865..d391ec5796610 100644
> --- a/drivers/net/vxlan/vxlan_vnifilter.c
> +++ b/drivers/net/vxlan/vxlan_vnifilter.c
> @@ -467,7 +467,7 @@ static const struct nla_policy vni_filter_entry_policy[VXLAN_VNIFILTER_ENTRY_MAX
>  };
>  
>  static const struct nla_policy vni_filter_policy[VXLAN_VNIFILTER_MAX + 1] = {
> -	[VXLAN_VNIFILTER_ENTRY] = { .type = NLA_NESTED },
> +	[VXLAN_VNIFILTER_ENTRY] = NLA_POLICY_NESTED(vni_filter_entry_policy),
>  };

[Severity: High]
This isn't a bug introduced by this patch, but at this commit the entry
policy that is now linked still has no range check on START and END:

	[VXLAN_VNIFILTER_ENTRY_START] = { .type = NLA_U32 },
	[VXLAN_VNIFILTER_ENTRY_END] = { .type = NLA_U32 },

vxlan_process_vni_filter() hands any values other than both-zero to
vxlan_vni_add_del(). That function loops with a signed counter:

	int v, err = 0;
	...
	for (v = start_vni; v <= end_vni; v++) {

With START=0xffffffff and no END, v starts at -1. Compared with the __u32
end_vni it becomes 0xffffffff, and v++ then climbs up from 0.

Can this loop run for a very long time under RTNL? Each iteration allocates
a vxlan_vni_node with per-CPU stats and sends a notification. The loop only
stops on an error.

VNIs >= VXLAN_N_VID are also accepted and stored, so they alias on the
wire.

Later patches in this series fix this. "vxlan: vnifilter: reject VNIs
outside the 24-bit space" adds NLA_POLICY_FULL_RANGE(NLA_U32,
&vni_filter_vni_range) to START and END, and makes the loop counter u32.
"vxlan: vnifilter: bound the number of VNIs one request may touch" caps the
total span at VXLAN_VNI_FILTER_MSG_MAX before dispatch.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927215209.2581830-1-alishmery18%40gmail.com