[PATCH net] vxlan: reject appending a remote to a nexthop fdb entry

[PATCH net] vxlan: reject appending a remote to a nexthop fdb entry

[Henry Martin]

vxlan_fdb_update_existing() guards nexthop entries against nhid
replacement (nhid && !f->nh) and against nhid-carrying NLM_F_APPEND,
but the plain NLM_F_APPEND branch for multicast/zero MACs never
checks f->nh: vxlan_fdb_append() happily links a remote onto an
entry that is backed by a nexthop group.

This breaks the "nexthop entries have no remote list" invariant: the
xmit path consults f->nh while dump/notify/delete walk the remotes
list.  Concretely, the appended remote is invisible in fdb dumps,
yet "bridge fdb del ... dst <ip>" removes it, and once the last
remote is gone the whole nexthop entry is destroyed, silently
dropping a forwarding binding the user never asked to remove.

Commit 98fc57d167446 ("vxlan: reject dynamic fdb entries that
reference a nexthop id") closed the sibling nh/state mixing vector;
this is the same invariant violation through the opposite door.

Reject NLM_F_APPEND on nexthop-backed entries.

This issue was discovered by Tencent CodeBuddy Security.

Cc: stable@vger.kernel.org
Fixes: 1274e1cc4226 ("vxlan: ecmp support for mac fdb entries")
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
---
 drivers/net/vxlan/vxlan_core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index 3390e341d1e58..aff450ff6aea7 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -1038,6 +1038,11 @@ static int vxlan_fdb_update_existing(struct vxlan_dev *vxlan,
 	if ((flags & NLM_F_APPEND) &&
 	    (is_multicast_ether_addr(f->key.eth_addr) ||
 	     is_zero_ether_addr(f->key.eth_addr))) {
+		if (rcu_access_pointer(f->nh)) {
+			NL_SET_ERR_MSG(extack,
+				       "Cannot append a remote to a nexthop fdb");
+			return -EOPNOTSUPP;
+		}
 		rc = vxlan_fdb_append(f, ip, port, vni, ifindex, &rd);
 
 		if (rc < 0)
-- 
2.43.7

Re: [PATCH net] vxlan: reject appending a remote to a nexthop fdb entry

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 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] In vxlan_fdb_update_existing()
  (drivers/net/vxlan/vxlan_core.c), the new `if…
- [Low] The commit message presents the change only as rejecting a
  userspace `bridge fdb append` onto a nexthop entry.

--

vxlan: reject appending a remote to a nexthop fdb entry

This patch adds a check to vxlan_fdb_update_existing(). An NLM_F_APPEND
request on a multicast or zero MAC fdb entry now fails with -EOPNOTSUPP
when the entry is backed by a nexthop group. Before this,
vxlan_fdb_append() would link a remote onto such an entry, which breaks
the rule that nexthop entries have no remote list.

> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index 278d41f58d895..e9c5ba7a74172 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c
> @@ -1047,6 +1047,11 @@ static int vxlan_fdb_update_existing(struct vxlan_dev *vxlan,
>  	if ((flags & NLM_F_APPEND) &&
>  	    (is_multicast_ether_addr(f->key.eth_addr) ||
>  	     is_zero_ether_addr(f->key.eth_addr))) {
> +		if (rcu_access_pointer(f->nh)) {
> +			NL_SET_ERR_MSG(extack,
> +				       "Cannot append a remote to a nexthop fdb");
> +			return -EOPNOTSUPP;
> +		}

[Severity: Medium]
Can this early return leave the entry partly updated? When this check
runs, vxlan_fdb_update_existing() has already written the new state and
flags:

	if (!(fdb_flags & NTF_EXT_LEARNED) ||
	    !(f->flags & NTF_VXLAN_ADDED_BY_USER)) {
		if (f->state != state) {
			f->state = state;
			notify = 1;
		}
		if (f->flags != fdb_flags) {
			f->flags = fdb_flags;
			notify = 1;
		}
	}

Returning -EOPNOTSUPP here skips the if (notify) block, so no
RTM_NEWNEIGH or switchdev notification goes out. It also skips the
err_notify rollback.

For example, create a zero MAC nexthop entry with:

  bridge fdb add 00:00:00:00:00:00 dev vx0 nhid N self permanent

Then run:

  bridge fdb append 00:00:00:00:00:00 dev vx0 dst X self permanent router

The append passes the earlier nexthop checks, because nhid is 0 and the
state includes NUD_PERMANENT. It changes f->flags and then fails.
vxlan_fdb_add() returns the error and restores nothing.

The in-kernel callers vxlan_changelink() and
vxlan_update_default_fdb_entry() also reach this point. They pass
ndm_flags = NTF_SELF and state = NUD_REACHABLE | NUD_PERMANENT.

On a nexthop default entry created by the user, f->flags is overwritten
with NTF_SELF, which clears NTF_VXLAN_ADDED_BY_USER, and then the call
fails. vxlan_changelink() treats the failure as if nothing changed and
frees new_cfg.

The other three nexthop checks near the top of this function all return
before anything is changed. Could this check move up next to them, so
that the failed request has no side effects?

[Severity: Low]
The commit message presents this as rejecting a user's bridge fdb append
onto a nexthop entry. Two in-kernel paths also reach this check with
NLM_F_APPEND | NLM_F_CREATE on all_zeros_mac and nhid = 0:

  vxlan_changelink()
    vxlan_fdb_update()
      vxlan_fdb_update_existing()

  vxlan_update_default_fdb_entry()
    vxlan_fdb_update()
      vxlan_fdb_update_existing()

The first runs on "ip link set dev vx0 type vxlan remote <ip>". The
second runs on vnifilter group add or change, for example
"bridge vni add ... group ...".

On a non-metadata device, the zero MAC lookup is keyed on cfg->vni. A
nexthop entry created with "bridge fdb add 00:00:00:00:00:00 dev vx0
nhid N self permanent" is therefore the same entry these paths update.

Before this patch, these link and VNI changes succeeded, though they left
an unused remote on the entry. With this patch they fail with
-EOPNOTSUPP and the extack "Cannot append a remote to a nexthop fdb".
That message describes an fdb operation the user never issued.

Rejecting these requests is probably the right result. Since the patch
is tagged for stable, should the commit message mention this change in
rtnetlink link and vnifilter behavior? Could the error message also be
made accurate for these callers?

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