Re: [net] vxlan: vnifilter: validate the VNI range in vni_filter_entry_policy

[netdev-bot+sashiko]

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

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

- [High] Incomplete fix: the added netlink range validation bounds only
  the individual VXLAN_VNIFILTER_ENTRY_START/END values (to…

Pre-existing issues:
- [Low] The newly added vni_filter_vni_range leaves .min unset (0), so
  the policy formally advertises VNI 0 as a valid…
- [Low] vxlan_vni_alloc() allocates the vxlan_vni_node with kzalloc_obj()
  (plain GFP_KERNEL, no __GFP_ACCOUNT) at…

--

vxlan: vnifilter: validate the VNI range in vni_filter_entry_policy

This adds netlink range validation to VXLAN_VNIFILTER_ENTRY_START and
VXLAN_VNIFILTER_ENTRY_END so each value is capped at VXLAN_N_VID - 1,
matching the constraint already used by the MDB code in the same driver.
The stated goal is to stop an unprivileged netlink request from driving an
unbounded allocation loop in vxlan_vni_add_del().

> diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
> index dd94085e0886..9e86ac39cf9d 100644
> --- a/drivers/net/vxlan/vxlan_vnifilter.c
> +++ b/drivers/net/vxlan/vxlan_vnifilter.c
> @@ -459,9 +459,15 @@ static int vxlan_vnifilter_dump(struct sk_buff *skb, struct netlink_callback *cb
>  	return err;
>  }
>  
> +static const struct netlink_range_validation vni_filter_vni_range = {
> +	.max = VXLAN_N_VID - 1,
> +};

[Severity: High]
This bounds each endpoint but not the distance between them.  Does that
still leave the scenario described in the changelog reachable?

A single RTM_NEWTUNNEL with START=0 and END=0xFFFFFF passes the new
policy, and vxlan_process_vni_filter() forwards both values unchanged:

	if (vattrs[VXLAN_VNIFILTER_ENTRY_START]) {
		vni_start = nla_get_u32(vattrs[VXLAN_VNIFILTER_ENTRY_START]);
		vni_end = vni_start;
	}

	if (vattrs[VXLAN_VNIFILTER_ENTRY_END])
		vni_end = nla_get_u32(vattrs[VXLAN_VNIFILTER_ENTRY_END]);
	...
	err = vxlan_vni_add_del(vxlan, vni_start, vni_end, &group, cmd,
				extack);

which then runs 2^24 iterations in vxlan_vni_add_del():

	for (v = start_vni; v <= end_vni; v++) {
		switch (cmd) {
		case RTM_NEWTUNNEL:
			err = vxlan_vni_add(vxlan, vg, v, group, extack);

Each iteration reaches vxlan_vni_alloc(), which does one slab allocation
plus one per-CPU stats block:

	vninode = kzalloc_obj(*vninode);
	if (!vninode)
		return NULL;
	vninode->stats = netdev_alloc_pcpu_stats(struct vxlan_vni_stats_pcpu);

struct vxlan_vni_stats_pcpu is 64 bytes per CPU, so on a 4-CPU host that
is roughly 2^24 * (192 + 4 * 64), several gigabytes from one message.
There is still no __GFP_ACCOUNT on either allocation, and no
cond_resched() in the loop.

The RTM_NEWTUNNEL handler is also still registered with flags 0:

	{THIS_MODULE, PF_BRIDGE, RTM_NEWTUNNEL, vxlan_vnifilter_process, NULL, 0},

so the whole loop still executes under the host-global rtnl_lock, which is
the stall the changelog describes.  Nothing else caps growth either:
vg->num_vnis is only adjusted in __vxlan_vni_add_list() and
__vxlan_vni_del_list() and is never compared against a limit.

The one case this patch does remove is end_vni == U32_MAX, where v wraps
and the loop condition never becomes false.  Would it make sense to say
that explicitly in the changelog, and additionally bound the number of
VNIs created per request, add a reschedule point in the loop, or account
the memory, before claiming the OOM and rtnl stall are closed?

[Severity: Low]
This is a pre-existing inconsistency rather than something introduced
here, but the new range leaves .min at 0, so the policy now formally
advertises VNI 0 as a valid START/END value, while
vxlan_process_vni_filter() treats 0 as "attribute absent":

	if (!vni_start && !vni_end) {
		NL_SET_ERR_MSG_ATTR(extack, nlvnifilter,
				    "vni start nor end found in vni entry");
		return -EINVAL;
	}

A request carrying only START=0 passes the new validation and is then
rejected with an extack saying neither start nor end was found, while
START=0 with END=10 installs VNI 0 without complaint.  Is it worth
setting .min explicitly, or reworking the sentinel so the extack matches
the actual reason?

>  static const struct nla_policy vni_filter_entry_policy[VXLAN_VNIFILTER_ENTRY_MAX + 1] = {
> -	[VXLAN_VNIFILTER_ENTRY_START] = { .type = NLA_U32 },
> -	[VXLAN_VNIFILTER_ENTRY_END] = { .type = NLA_U32 },
> +	[VXLAN_VNIFILTER_ENTRY_START] = NLA_POLICY_FULL_RANGE(NLA_U32,
> +							      &vni_filter_vni_range),
> +	[VXLAN_VNIFILTER_ENTRY_END] = NLA_POLICY_FULL_RANGE(NLA_U32,
> +							    &vni_filter_vni_range),

[Severity: Low]
This isn't a bug introduced by this patch, but the changelog calls out the
missing memcg accounting as part of the problem, and vxlan_vni_alloc() is
unchanged:

	vninode = kzalloc_obj(*vninode);
	if (!vninode)
		return NULL;
	vninode->stats = netdev_alloc_pcpu_stats(struct vxlan_vni_stats_pcpu);

kzalloc_obj() uses plain GFP_KERNEL and netdev_alloc_pcpu_stats() ends up
at alloc_percpu_gfp(..., GFP_KERNEL), so neither carries __GFP_ACCOUNT and
neither is charged to the requesting task's cgroup.  A caller inside a
user plus net namespace can still ask for the full legitimate 24-bit range
and get one node plus one per-CPU block per VNI, scaled by nr_cpus,
outside its memcg limit.  Should the changelog stop listing the accounting
gap among the things being addressed, or should the allocations gain
__GFP_ACCOUNT as part of this change?

>  	[VXLAN_VNIFILTER_ENTRY_GROUP]	= NLA_POLICY_EXACT_LEN(sizeof_field(struct iphdr, daddr)),
>  	[VXLAN_VNIFILTER_ENTRY_GROUP6]	= NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)),
>  };

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