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

[Ali Firas <alishmery18@gmail.com>]

> I tend to agree, either this is a security fix and we need a stronger
> check. Or it's just making things slightly better and doesn't deserve
> Fixes/stable.

You're right, and my justification was worse than incomplete. Sorry for the
slow reply.

To be precise about what is and isn't real: the loop's own bound genuinely is
broken at END=U32_MAX -- v is int, end_vni is __u32, so the comparison is
unsigned and the loop has no exit condition of its own; it leaves only via
"if (err) goto out". But the wrap is unreachable. vxlan_vni_add() allocates
for every fresh VNI, so -ENOMEM ends the loop long before v could wrap, and
the delete path stops at the first absent VNI with -ENOENT. So it is not a
non-terminating loop, and the changelog should never have said so.

What I did observe in a 2G KASAN guest was a global OOM, and that is a
resource problem, not a control-flow one. The clamp does not address it:
START=0 END=0xFFFFFF passes the new policy and allocates just the same.

vxlan_vni_alloc() uses plain GFP_KERNEL for both the node and its per-CPU
stats, so none of it is charged to the caller. By size -- I haven't measured
the struct yet -- that is on the order of 192 bytes plus 64 per CPU per VNI,
so a full in-range request is a few GB on 2 CPUs and tens of GB on 64, from
one netlink message, by an unprivileged user holding CAP_NET_ADMIN in a
netns.

That looks like the same class as 1beb81947eb4 ("net/sched: account classifier
filter allocations to memcg"), and __netdev_alloc_pcpu_stats() already takes a
gfp, so no new API is needed. One difference worth flagging: that patch also
had to fix a real error-path hazard, because cls_basic did idr_alloc before
alloc_percpu. vxlan doesn't -- both allocations are inside vxlan_vni_alloc()
and return NULL before rhashtable_lookup_insert_fast(), so making them
failable exposes nothing new.

So I'd rather split this:

  - GFP_KERNEL_ACCOUNT on the node and its per-CPU stats. This is the actual
    fix. Given the difference above, I'm not sure whether it belongs in net
    with a Fixes tag or in net-next -- happy to go either way.
  - the range validation on its own, to net-next, no Fixes and no security
    claim. Its only real justification is that vxlan_vni_rht_params already
    declares .max_size = VXLAN_N_VID, so the netlink edge never enforced the
    bound the driver assumes. It does tighten uAPI: requests that used to
    succeed with an out-of-range VNI will now get -EINVAL.

I'm measuring the full in-range request against the accounting patch before
sending anything.

Thanks for the review,
Ali