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

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

[Ali Firas]

VXLAN_VNIFILTER_ENTRY_START and VXLAN_VNIFILTER_ENTRY_END are declared as
bare NLA_U32, so neither is range-checked before vxlan_process_vni_filter()
passes them to vxlan_vni_add_del():

	int v, err = 0;

	for (v = start_vni; v <= end_vni; v++) {

v is int and end_vni is __u32, so the comparison is unsigned. With
end_vni == U32_MAX the loop cannot terminate through its own condition:
v reaches U32_MAX, wraps to 0, and 0 <= U32_MAX is true again. Every
iteration allocates a vxlan_vni_node plus a per-CPU vxlan_vni_stats_pcpu
block until the machine is out of memory. Neither allocation carries
__GFP_ACCOUNT, so the memory is not charged to the caller's memory
cgroup, and the per-CPU block multiplies the cost by the number of CPUs.

The rtnl_msg_handler entry for RTM_NEWTUNNEL carries no
RTNL_FLAG_DOIT_UNLOCKED, so the loop runs holding rtnl_lock. rtnl_lock is
global rather than per-netns, so for as long as the loop runs every
network configuration operation on the host blocks, in every namespace.
There is no cond_resched() in the loop either.

The interface is reachable without privilege: creating the device and
adding VNIs only requires CAP_NET_ADMIN in the network namespace's user
namespace, so an unprivileged user inside unshare(CLONE_NEWUSER |
CLONE_NEWNET) can trigger this with a single netlink message.

A VNI at or above VXLAN_N_VID is also accepted and stored, although the
VXLAN header carries only 24 bits.

The MDB interface in the same driver already range-validates its VNI
attributes with an identical constraint (vxlan_mdb.c, vni_range with
.max = VXLAN_N_VID - 1). Apply the same validation here.

Tested under KASAN in a 2G guest: before the change, adding the range
0-U32_MAX from an unprivileged user namespace drives a global OOM with
the allocating task in vxlan_vnifilter_process(); after it, out-of-range
values are rejected and the in-range case is unaffected.

Reproducer available on request.

Fixes: f9c4bb0b245c ("vxlan: vni filtering support on collect metadata device")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-6 [claude-code]
Signed-off-by: Ali Firas <alishmery18@gmail.com>
---
 drivers/net/vxlan/vxlan_vnifilter.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

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,
+};
+
 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),
 	[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)),
 };
-- 
2.53.0

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