[PATCH net v3 0/6] amt: relay tunnel lifetime, validation and request-path hardening

[PATCH net v3 0/6] amt: relay tunnel lifetime, validation and request-path hardening

[Omar Ramadan]

Six fixes to the AMT relay data and control paths, found while adding an
IPv6 outer transport (that feature follows for net-next, rebased on this).
All are independent of the IPv6 work and belong in net with stable in mind.

1/6 is the most serious: a Membership Update can re-arm the gc timer of a
tunnel amt_tunnel_expire() is already freeing, firing a delayed_work in
freed memory.  2/6 and 3/6 close two input-validation holes (a gateway
Query/Data from 0.0.0.0 before a relay is learned; a 16-byte IPv6 literal
accepted into an IPv4 netlink attribute).  4/6-6/6 harden the relay's
tunnel table: key it on the (address, port) endpoint RFC 7450 defines,
surface silently-dropped pre-Query reports, and stop allocating tunnel
state for unauthenticated (spoofable) Requests, which closes a table-
exhaustion / ICMP-reflector / session-desync primitive and moves the
max_tunnels bound onto verified gateways.

Each patch carries a Fixes: tag.  Verified W=1 clean and with a
KASAN + lockdep + PROVE_RCU boot of the amt selftests, no splat, and
of amt.sh with max_tunnels 1, which keeps the tunnel table full while
the gateway keeps sending Requests.

v3:
- Add 1/6-3/6, found in review of v2 and of the net-next IPv6
  series [1]: the gc_wq re-arm use-after-free, and the IPv4 halves of
  two input-validation bugs.  The IPv6 series will be rebased on this.
- 4/6: do not count the invalid-MAC drop in rx_dropped; amt_rcv()
  already counts it.
- 5/6: commit message: the Query supplies the response MAC that the
  Update echoes, not the nonce.
- 6/6: keep three secrets so a response MAC outlives the advertised
  QQIC; cap the tunnels one source can hold in the locked re-check (a
  fixed limit, no new UAPI); when the table is full, answer a gateway
  that holds no tunnel with ICMP and no Query, so it rediscovers; seed
  the secret before the socket is created; refresh last_seen rather
  than re-arming gc_wq (see 1/6).
v2: https://lore.kernel.org/netdev/20261009201455.1904698-1-omar@blockcast.net/
v1: https://lore.kernel.org/netdev/20261008003606.3666617-1-omar@blockcast.net/

[1] https://lore.kernel.org/netdev/20261009122426.551178-1-omar@blockcast.net/

Omar Ramadan (6):
  amt: fix use-after-free when a Membership Update re-arms a freed tunnel
  amt: reject a gateway Query or Data from 0.0.0.0 before a relay is learned
  amt: reject an IPv6 address passed in an IPv4 address attribute
  amt: key relay tunnel state on the (address, port) endpoint, not the address
  amt: make pre-query report drops visible
  amt: do not create tunnel state for unauthenticated Requests

[PATCH net v3 1/6] amt: fix use-after-free when a Membership Update re-arms a freed tunnel

[Omar Ramadan]

amt_update_handler() refreshes a relay tunnel's garbage-collection timer
with mod_delayed_work(&tunnel->gc_wq, ...) on a tunnel found by a lockless
list_for_each_entry_rcu() walk, holding only rcu_read_lock().  Nothing
serialises this against amt_tunnel_expire(), which runs from that same
gc_wq: once expire has taken amt->lock, done list_del_rcu() and queued
kfree_rcu(), a concurrent Update that already cached the tunnel pointer
re-arms gc_wq on an object that is about to be freed.  After the grace
period the delayed_work timer is left armed in freed memory, so it fires
into a use-after-free (and double-list_del_rcu/nr_tunnels underflow on the
way).

Make amt_tunnel_expire() the only context that frees a tunnel on the
timeout path and let it re-arm itself: the Update path only stamps
tunnel->last_seen, and expire re-reads last_seen under amt->lock to decide
whether to re-arm or free.  The Update path therefore never touches gc_wq,
so it can no longer re-queue the work of a tunnel being torn down.

amt_dev_stop() is the other freer; mark each tunnel dead and splice the
list out under amt->lock before cancelling and freeing, so a gc_wq that is
already running sees ->dead and bails instead of freeing in parallel.  The
tunnel is then freed exactly once.

Fixes: cbc21dc1cfe9 ("amt: add data plane of amt interface")
Assisted-by: LLM
Signed-off-by: Omar Ramadan <omar@blockcast.net>
---
 drivers/net/amt.c | 48 +++++++++++++++++++++++++++++++++++++++++------
 include/net/amt.h | 11 +++++++++++
 2 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index b53f8ec..b284fa7 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -1328,8 +1328,29 @@ static void amt_tunnel_expire(struct work_struct *work)
 						      struct amt_tunnel_list,
 						      gc_wq);
 	struct amt_dev *amt = tunnel->amt;
+	unsigned long deadline;
 
 	spin_lock_bh(&amt->lock);
+	/* amt_dev_stop() has taken ownership of the teardown. */
+	if (tunnel->dead) {
+		spin_unlock_bh(&amt->lock);
+		return;
+	}
+	/* A Membership Update may have refreshed the tunnel after this work was
+	 * queued.  Re-arm from last_seen rather than letting amt_update_handler()
+	 * re-arm gc_wq: this handler is then the only context that frees the
+	 * tunnel (and the only one amt_dev_stop() has to cancel), which closes
+	 * the use-after-free where a concurrent Update re-armed gc_wq on a tunnel
+	 * this function had already list_del_rcu()'d and kfree_rcu()'d.
+	 */
+	deadline = READ_ONCE(tunnel->last_seen) +
+		   msecs_to_jiffies(amt_gmi(amt)) * 3;
+	if (time_before(jiffies, deadline)) {
+		mod_delayed_work(amt_wq, &tunnel->gc_wq, deadline - jiffies);
+		spin_unlock_bh(&amt->lock);
+		return;
+	}
+	tunnel->dead = true;
 	rcu_read_lock();
 	list_del_rcu(&tunnel->list);
 	amt->nr_tunnels--;
@@ -2494,9 +2515,12 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
 		if (tunnel->ip4 == saddr) {
 			if ((nonce == tunnel->nonce &&
 			     response_mac == tunnel->mac)) {
-				mod_delayed_work(amt_wq, &tunnel->gc_wq,
-						 msecs_to_jiffies(amt_gmi(amt))
-								  * 3);
+				/* Refresh liveness only.  gc_wq is re-armed by
+				 * amt_tunnel_expire() itself from last_seen, never
+				 * from here, so this cannot re-queue the work of a
+				 * tunnel that is being freed.
+				 */
+				WRITE_ONCE(tunnel->last_seen, jiffies);
 				goto report;
 			} else {
 				netdev_dbg(amt->dev, "Invalid MAC\n");
@@ -2732,6 +2756,7 @@ static bool amt_request_handler(struct amt_dev *amt, struct sk_buff *skb)
 	for (i = 0; i < amt->hash_buckets; i++)
 		INIT_HLIST_HEAD(&tunnel->groups[i]);
 
+	tunnel->last_seen = jiffies;
 	INIT_DELAYED_WORK(&tunnel->gc_wq, amt_tunnel_expire);
 
 	list_add_tail_rcu(&tunnel->list, &amt->tunnel_list);
@@ -3053,6 +3078,7 @@ static int amt_dev_stop(struct net_device *dev)
 	struct amt_tunnel_list *tunnel, *tmp;
 	struct sk_buff *skb;
 	struct sock *sk;
+	LIST_HEAD(doomed);
 	int i;
 
 	disable_delayed_work_sync(&amt->req_wq);
@@ -3079,9 +3105,19 @@ static int amt_dev_stop(struct net_device *dev)
 	amt->req_cnt = 0;
 	WRITE_ONCE(amt->remote_ip, 0);
 
-	list_for_each_entry_safe(tunnel, tmp, &amt->tunnel_list, list) {
-		list_del_rcu(&tunnel->list);
-		amt->nr_tunnels--;
+	/* Mark every tunnel dead and detach the list under the lock, so a
+	 * concurrent amt_tunnel_expire() sees ->dead and bails instead of
+	 * freeing a tunnel in parallel.  The detached list is then private to
+	 * this path, which is the sole freer.
+	 */
+	spin_lock_bh(&amt->lock);
+	list_for_each_entry(tunnel, &amt->tunnel_list, list)
+		tunnel->dead = true;
+	list_splice_init(&amt->tunnel_list, &doomed);
+	amt->nr_tunnels = 0;
+	spin_unlock_bh(&amt->lock);
+
+	list_for_each_entry_safe(tunnel, tmp, &doomed, list) {
 		cancel_delayed_work_sync(&tunnel->gc_wq);
 		amt_clear_groups(tunnel);
 		kfree_rcu(tunnel, rcu);
diff --git a/include/net/amt.h b/include/net/amt.h
index 2846dde..0520121 100644
--- a/include/net/amt.h
+++ b/include/net/amt.h
@@ -240,6 +240,17 @@ struct amt_tunnel_list {
 	u32			nr_sources;
 	enum amt_status		status;
 	struct delayed_work	gc_wq;
+	/* Last time a valid Membership Update refreshed this tunnel.  gc_wq
+	 * re-arms itself from this rather than being re-armed by the Update
+	 * path, so the Update path cannot re-queue the work of a tunnel that
+	 * amt_tunnel_expire() is freeing.
+	 */
+	unsigned long		last_seen;
+	/* Set under amt->lock by whichever of amt_tunnel_expire() or
+	 * amt_dev_stop() reaches the tunnel first; the other then leaves it
+	 * alone, so the tunnel is freed exactly once.
+	 */
+	bool			dead;
 	__be16			source_port;
 	__be32			ip4;
 	__be32			nonce;
-- 
2.43.0

[PATCH net v3 2/6] amt: reject a gateway Query or Data from 0.0.0.0 before a relay is learned

[Omar Ramadan]

A gateway's amt_rcv() admits a Membership Query or Multicast Data message
when "saddr != remote_ip" is false.  remote_ip is 0 before discovery has
learned a relay (and is reset to 0 by amt_dev_open() and when discovery
gives up), so a packet whose source is 0.0.0.0 -- for example a
0.0.0.0 -> 255.255.255.255 broadcast delivered to the socket -- satisfies
0 != 0 and is accepted.  With the nonce freshly reset to 0, a spoofed
Query is then taken as genuine: it sets ready4/qi and the response MAC and
suppresses rediscovery until the discovery timeout, and a spoofed Data
message is injected the same way.

Reject both when no relay is learned: a 0 remote_ip must match nothing, so
guard the check with !remote_ip.  Discovery is unaffected -- Relay
Advertisements are validated against discovery_ip, not remote_ip.

Fixes: b9022b53adad ("amt: add control plane of amt interface")
Assisted-by: LLM
Signed-off-by: Omar Ramadan <omar@blockcast.net>
---
 drivers/net/amt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index b284fa7..c3716ba 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -2858,7 +2858,7 @@ static int amt_rcv(struct sock *sk, struct sk_buff *skb)
 			}
 			goto out;
 		case AMT_MSG_MULTICAST_DATA:
-			if (saddr != remote_ip) {
+			if (!remote_ip || saddr != remote_ip) {
 				netdev_dbg(amt->dev, "Invalid Relay IP\n");
 				err = true;
 				goto drop;
@@ -2869,7 +2869,7 @@ static int amt_rcv(struct sock *sk, struct sk_buff *skb)
 			else
 				goto out;
 		case AMT_MSG_MEMBERSHIP_QUERY:
-			if (saddr != remote_ip) {
+			if (!remote_ip || saddr != remote_ip) {
 				netdev_dbg(amt->dev, "Invalid Relay IP\n");
 				err = true;
 				goto drop;
-- 
2.43.0

[PATCH net v3 3/6] amt: reject an IPv6 address passed in an IPv4 address attribute

[Omar Ramadan]

The netlink policy for IFLA_AMT_LOCAL_IP and IFLA_AMT_DISCOVERY_IP sets
only a minimum length (the size of an IPv4 address), so an iproute2 that
puts a 16-byte IPv6 literal in one of these IPv4 attributes passes
validation.  nla_get_in_addr() then reads just the first four bytes, so
"2001:db8::" silently becomes 32.1.13.184 and the device is created with an
address the user never asked for.

Reject an attribute whose length is exactly an IPv6 address for the two
IPv4 attributes, so the mistake is refused with an extack instead of being
truncated.

Fixes: b9022b53adad ("amt: add control plane of amt interface")
Assisted-by: LLM
Signed-off-by: Omar Ramadan <omar@blockcast.net>
---
 drivers/net/amt.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index c3716ba..550b0a5 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -3194,6 +3194,15 @@ static const struct nla_policy amt_policy[IFLA_AMT_MAX + 1] = {
 	[IFLA_AMT_MAX_TUNNELS]	= { .type = NLA_U32 },
 };
 
+static bool amt_ip6_in_ip4_attr(const struct nlattr *attr,
+				struct netlink_ext_ack *extack)
+{
+	if (!attr || nla_len(attr) != sizeof(struct in6_addr))
+		return false;
+	NL_SET_ERR_MSG_ATTR(extack, attr, "IPv6 address in an IPv4 attribute");
+	return true;
+}
+
 static int amt_validate(struct nlattr *tb[], struct nlattr *data[],
 			struct netlink_ext_ack *extack)
 {
@@ -3224,6 +3233,9 @@ static int amt_validate(struct nlattr *tb[], struct nlattr *data[],
 		return -EINVAL;
 	}
 
+	if (amt_ip6_in_ip4_attr(data[IFLA_AMT_LOCAL_IP], extack))
+		return -EINVAL;
+
 	if (!data[IFLA_AMT_DISCOVERY_IP] &&
 	    nla_get_u32(data[IFLA_AMT_MODE]) == AMT_MODE_GATEWAY) {
 		NL_SET_ERR_MSG_ATTR(extack, data[IFLA_AMT_LOCAL_IP],
@@ -3231,6 +3243,9 @@ static int amt_validate(struct nlattr *tb[], struct nlattr *data[],
 		return -EINVAL;
 	}
 
+	if (amt_ip6_in_ip4_attr(data[IFLA_AMT_DISCOVERY_IP], extack))
+		return -EINVAL;
+
 	return 0;
 }
 
-- 
2.43.0

[PATCH net v3 4/6] amt: key relay tunnel state on the (address, port) endpoint, not the address

[Omar Ramadan]

amt_request_handler and amt_update_handler both look a tunnel up by the
outer source address alone:

	list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list)
		if (tunnel->ip4 == iph->saddr)
			goto send;

RFC 7450 s4.2.2 defines the unit of relay tunnel state differently: "an
AMT 'tunnel' is identified by the IP address and UDP port pair used as
the destination address for sending encapsulated multicast IP datagrams
to a gateway", and "each unique combination represents a unique tunnel
endpoint".

Because the port term is missing, two distinct endpoints that share a
source address alias onto one tunnel. The second Request to arrive
reaches `send:`, overwrites tunnel->nonce and re-derives tunnel->mac, so
the first gateway's subsequent Membership Updates no longer match and
are dropped at the "Invalid MAC" arm. That arm returns rather than
continuing the walk, so there is no recovery path: the first gateway has
had its Request answered and its membership accepted, and simply never
receives data again. The failure is silent on both sides.

Two deployments reach this, and the same RFC section names both:

 - NAT, which s4.2.2 calls out explicitly ("this address may differ from
   that carried by the message when it exited the gateway as a result of
   network address translation"). CGNAT, a single-WAN site with a
   redundant gateway pair, or two subscriber devices behind one
   residential NAT all present as one source address.
 - A single gateway host, with no NAT anywhere, which s4.2.2 says "may
   use separate ports for the IPv4/IGMP and IPv6/MLD protocols".

Add the port term to both lookups. amt_update_handler snapshots the
source port before iptunnel_pull_header() strips the encap, alongside
the existing pre-pull reads.

With the endpoint keyed correctly, a gateway that re-Requests from a new
ephemeral port no longer aliases onto its own previous tunnel: it gets a
new one addressed to the port it is listening on, and the old one ages
out on gc_wq. That is the same stale-Membership-Query symptom addressed
by refreshing tunnel->source_port at `send:`, fixed at the cause instead
-- so this change supersedes that approach rather than stacking on it.

Note this removes an accidental bound: while tunnels were keyed on the
address alone, one source address could never hold more than one tunnel,
whatever it did. RFC 7450 s5.3.3 asks for that bound explicitly, and it
is restored in a companion net-next patch ("amt: bound relay tunnels
admitted per source address") rather than here, since it adds UAPI and
this is a fix.

Fixes: cbc21dc1cfe9 ("amt: add data plane of amt interface")
Assisted-by: LLM
Signed-off-by: Omar Ramadan <omar@blockcast.net>
---
 drivers/net/amt.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index 550b0a5..2feb418 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -2492,6 +2492,7 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
 	u64 response_mac;
 	__be32 saddr;
 	__be32 nonce;
+	__be16 sport;
 
 	saddr = ip_hdr(skb)->saddr;
 
@@ -2505,6 +2506,8 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
 
 	nonce = amtmu->nonce;
 	response_mac = amtmu->response_mac;
+	/* Snapshot the tunnel endpoint port before the encap is stripped. */
+	sport = udp_hdr(skb)->source;
 
 	if (iptunnel_pull_header(skb, hdr_size, skb->protocol, false))
 		return true;
@@ -2512,7 +2515,8 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
 	skb_reset_network_header(skb);
 
 	list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list) {
-		if (tunnel->ip4 == saddr) {
+		if (tunnel->ip4 == saddr &&
+		    tunnel->source_port == sport) {
 			if ((nonce == tunnel->nonce &&
 			     response_mac == tunnel->mac)) {
 				/* Refresh liveness only.  gc_wq is re-armed by
@@ -2729,7 +2733,8 @@ static bool amt_request_handler(struct amt_dev *amt, struct sk_buff *skb)
 		return true;
 
 	list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list)
-		if (tunnel->ip4 == iph->saddr)
+		if (tunnel->ip4 == iph->saddr &&
+		    tunnel->source_port == udph->source)
 			goto send;
 
 	spin_lock_bh(&amt->lock);
@@ -2768,6 +2773,12 @@ static bool amt_request_handler(struct amt_dev *amt, struct sk_buff *skb)
 	spin_unlock_bh(&amt->lock);
 
 send:
+	/* source_port is part of the tunnel's identity and is set once, in
+	 * the allocation path above; the lookup only reaches here on an exact
+	 * (address, port) match, so it already equals udph->source.  Do not
+	 * "refresh" it here: doing so let a colliding Request from a new port
+	 * steal an established tunnel outright.
+	 */
 	tunnel->nonce = amtrh->nonce;
 	mac = siphash_3u32((__force u32)tunnel->ip4,
 			   (__force u32)tunnel->source_port,
-- 
2.43.0

[PATCH net v3 5/6] amt: make pre-query report drops visible

[Omar Ramadan]

A gateway cannot forward an IGMP or MLD report until it has received the
relay's Membership Query for that family.  The Query supplies the
response MAC the Update echoes, so dropping an early report is required,
but doing so silently leaves operators with a dark multicast path and no
indication why the join never happened.

Emit a family-specific debug message before taking the existing drop
path.  That path already increments tx_dropped, so each discarded report
is accounted exactly once.

Assisted-by: LLM
Signed-off-by: Omar Ramadan <omar@blockcast.net>
---
 drivers/net/amt.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index 2feb418..fec3265 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -1249,9 +1249,17 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 		/* Gateway only passes IGMP/MLD packets */
 		if (!report)
 			goto free;
+		/* A validated report can only be forwarded after the relay's
+		 * family-specific Membership Query supplies the state echoed
+		 * by the Membership Update. Log this readiness failure before
+		 * the shared drop path accounts it.
+		 */
 		if ((!v6 && !READ_ONCE(amt->ready4)) ||
-		    (v6 && !READ_ONCE(amt->ready6)))
+		    (v6 && !READ_ONCE(amt->ready6))) {
+			netdev_dbg(dev, "drop %s report: no Membership Query for this family yet\n",
+				   v6 ? "MLD" : "IGMP");
 			goto free;
+		}
 		if (amt_send_membership_update(amt, skb,  v6))
 			goto free;
 		goto unlock;
-- 
2.43.0

[PATCH net v3 6/6] amt: do not create tunnel state for unauthenticated Requests

[Omar Ramadan]

amt_request_handler() allocated a struct amt_tunnel_list for any incoming
Relay Membership Request, keyed on the claimed source endpoint, before
anything about that source had been verified. A Request is a bare UDP
datagram with a trivially spoofable source address, so this gave a remote
attacker two primitives:

 - Resource exhaustion. Each entry is held for amt_gmi() (260s with the
   default qrv=2/qi=125/qri=10) and the table is bounded by
   amt->max_tunnels (default AMT_MAX_TUNNELS, 128). Requests from 128
   spoofed addresses fill the table, and re-sending once per interval
   keeps it full, so real gateways are refused. Once full, every further
   Request also emitted an ICMP_DEST_UNREACH to the spoofed source,
   turning the relay into an ICMP reflector.

 - Session desynchronisation. The lookup hit at the top of the function
   jumped to the send path, which took no lock and overwrote ->nonce and
   ->mac of an already-established tunnel. One spoofed packet carrying a
   known gateway's source endpoint invalidates that gateway's outstanding
   (nonce, response_mac), so its next Membership Update is dropped as
   "Invalid MAC". This costs one packet, consumes no table slot, and is
   therefore unaffected by max_tunnels tuning.

No state actually has to be created at Request time. Every input to the
keyed MAC is carried in the packet or is device state, so the MAC can be
generated on Request and recomputed on Update rather than stored:

 - amt_request_handler() now allocates nothing. It computes the MAC and
   replies, matching amt_send_advertisement(), which already answers
   Discovery statelessly from the same context.
 - amt_update_handler() recomputes the expected MAC from the packet's
   source address, source port and nonce and drops the packet on
   mismatch. Tunnel state is created only after that check passes, in
   amt_tunnel_get_or_create(), so max_tunnels now bounds verified
   gateways rather than unverified Requests.
 - amt->key becomes a three-element array. amt_secret_work() rotates
   every AMT_SECRET_TIMEOUT (60s) and an exchange may straddle a
   rotation, so Update accepts the current or either of the two previous
   secrets. Three generations keep a MAC valid for 120-180s, at least the
   amt_gmi() the Query advertised; two generations (120s floor) could
   expire a MAC before the interval it promised. A stateless recompute
   cannot snapshot the key per tunnel the way the old code did.

Fold in four further issues on the same path:

 - Per-source exhaustion. With the table now bounded by verified
   gateways, one verified source could still take every slot. The locked
   re-check in amt_tunnel_get_or_create() counts the tunnels a source
   already holds and refuses past a fixed internal cap
   (AMT_MAX_TUNNELS_PER_SRC). This needs no UAPI, so it is a backportable
   fix; a configurable per-source bound is left to net-next.
 - Table-full killed rediscovery. A Request that arrives when the table
   is full, from a source that holds no tunnel, is now refused with ICMP
   and no Query, rather than answered with a Query whose later Update
   cannot be admitted. Withholding the Query lets the gateway's req_cnt
   climb and drive rediscovery. The ICMP is rate-limited and allocates
   nothing, unlike the old reflector. A source that holds a tunnel is
   still answered, as before: its periodic Request renews the response
   MAC its Updates echo.
 - Zero-key window at open. amt_dev_open() seeded amt->key only after
   amt_socket_create() had already enabled the encap receive path, so a
   Request arriving in that window was answered with a MAC keyed on the
   all-zero (predictable) secret. Seed the keys under amt->lock before
   the socket is created.
 - The entry was published by list_add_tail_rcu() before ->nonce and
   ->mac were assigned, leaving a window in which a concurrent Update
   could match the zeroed fields of a kzalloc()'d entry; and the send
   path read tunnel->key into an unused local before it was initialised.
   Both go away now that nothing is published until it is fully built.

The General Query is passed its destination by value rather than a tunnel
pointer, because at Request time no tunnel exists to point at.

Fixes: cbc21dc1cfe9 ("amt: add data plane of amt interface")
Assisted-by: LLM
Signed-off-by: Omar Ramadan <omar@blockcast.net>
---
 drivers/net/amt.c | 310 ++++++++++++++++++++++++++++++++--------------
 include/net/amt.h |  17 ++-
 2 files changed, 228 insertions(+), 99 deletions(-)

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index fec3265..2d9f2d9 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -784,11 +784,16 @@ out:
 
 static bool amt_send_membership_query(struct amt_dev *amt,
 				      struct sk_buff *skb,
-				      struct amt_tunnel_list *tunnel,
+				      __be32 daddr, __be16 dport,
+				      __be32 nonce, u64 mac,
 				      bool v6);
 
-static void amt_send_igmp_gq(struct amt_dev *amt,
-			     struct amt_tunnel_list *tunnel)
+/* The destination is passed by value rather than as a tunnel: the requesting
+ * source is not authenticated yet and no tunnel state exists for it - see
+ * amt_request_handler().
+ */
+static void amt_send_igmp_gq(struct amt_dev *amt, __be32 daddr, __be16 dport,
+			     __be32 nonce, u64 mac)
 {
 	struct sk_buff *skb;
 
@@ -797,7 +802,8 @@ static void amt_send_igmp_gq(struct amt_dev *amt,
 		return;
 
 	skb_pull(skb, sizeof(struct ethhdr));
-	if (amt_send_membership_query(amt, skb, tunnel, false)) {
+	if (amt_send_membership_query(amt, skb, daddr, dport, nonce, mac,
+				      false)) {
 		amt->dev->stats.tx_dropped++;
 		kfree_skb(skb);
 	}
@@ -876,7 +882,8 @@ static struct sk_buff *amt_build_mld_gq(struct amt_dev *amt)
 	return skb;
 }
 
-static void amt_send_mld_gq(struct amt_dev *amt, struct amt_tunnel_list *tunnel)
+static void amt_send_mld_gq(struct amt_dev *amt, __be32 daddr, __be16 dport,
+			    __be32 nonce, u64 mac)
 {
 	struct sk_buff *skb;
 
@@ -885,13 +892,15 @@ static void amt_send_mld_gq(struct amt_dev *amt, struct amt_tunnel_list *tunnel)
 		return;
 
 	skb_pull(skb, sizeof(struct ethhdr));
-	if (amt_send_membership_query(amt, skb, tunnel, true)) {
+	if (amt_send_membership_query(amt, skb, daddr, dport, nonce, mac,
+				      true)) {
 		amt->dev->stats.tx_dropped++;
 		kfree_skb(skb);
 	}
 }
 #else
-static void amt_send_mld_gq(struct amt_dev *amt, struct amt_tunnel_list *tunnel)
+static void amt_send_mld_gq(struct amt_dev *amt, __be32 daddr, __be16 dport,
+			    __be32 nonce, u64 mac)
 {
 }
 #endif
@@ -925,7 +934,13 @@ static void amt_secret_work(struct work_struct *work)
 					   secret_wq);
 
 	spin_lock_bh(&amt->lock);
-	get_random_bytes(&amt->key, sizeof(siphash_key_t));
+	/* Rotate: keep the two previous generations so a MAC handed out just
+	 * before a rotation still verifies for up to three AMT_SECRET_TIMEOUT
+	 * windows (120-180s), covering the amt_gmi() the Query advertised.
+	 */
+	amt->key[2] = amt->key[1];
+	amt->key[1] = amt->key[0];
+	get_random_bytes(&amt->key[0], sizeof(siphash_key_t));
 	spin_unlock_bh(&amt->lock);
 	mod_delayed_work(amt_wq, &amt->secret_wq,
 			 msecs_to_jiffies(AMT_SECRET_TIMEOUT));
@@ -1115,9 +1130,16 @@ static void amt_send_multicast_data(struct amt_dev *amt,
 			    0);
 }
 
+/* Send a Membership Query to a source that has not been authenticated yet.
+ *
+ * No per-source state exists at this point and none is created: the nonce and
+ * the keyed MAC are carried in the Query and recomputed from the Membership
+ * Update when it comes back, so everything this needs is passed by value.
+ */
 static bool amt_send_membership_query(struct amt_dev *amt,
 				      struct sk_buff *skb,
-				      struct amt_tunnel_list *tunnel,
+				      __be32 daddr, __be16 dport,
+				      __be32 nonce, u64 mac,
 				      bool v6)
 {
 	struct amt_header_membership_query *amtmq;
@@ -1138,13 +1160,13 @@ static bool amt_send_membership_query(struct amt_dev *amt,
 	skb_reset_inner_headers(skb);
 	memset(&fl4, 0, sizeof(struct flowi4));
 	fl4.flowi4_oif         = amt->stream_dev->ifindex;
-	fl4.daddr              = tunnel->ip4;
+	fl4.daddr              = daddr;
 	fl4.saddr              = amt->local_ip;
 	fl4.flowi4_dscp        = inet_dsfield_to_dscp(AMT_TOS);
 	fl4.flowi4_proto       = IPPROTO_UDP;
 	rt = ip_route_output_key(amt->net, &fl4);
 	if (IS_ERR(rt)) {
-		netdev_dbg(amt->dev, "no route to %pI4\n", &tunnel->ip4);
+		netdev_dbg(amt->dev, "no route to %pI4\n", &daddr);
 		return true;
 	}
 
@@ -1154,8 +1176,8 @@ static bool amt_send_membership_query(struct amt_dev *amt,
 	amtmq->reserved = 0;
 	amtmq->l	= 0;
 	amtmq->g	= 0;
-	amtmq->nonce	= tunnel->nonce;
-	amtmq->response_mac = tunnel->mac;
+	amtmq->nonce	= nonce;
+	amtmq->response_mac = mac;
 
 	if (!v6)
 		skb_set_inner_protocol(skb, htons(ETH_P_IP));
@@ -1168,11 +1190,10 @@ static bool amt_send_membership_query(struct amt_dev *amt,
 			    ip4_dst_hoplimit(&rt->dst),
 			    0,
 			    amt->relay_port,
-			    tunnel->source_port,
+			    dport,
 			    false,
 			    false,
 			    0);
-	amt_update_relay_status(tunnel, AMT_STATUS_SENT_QUERY, true);
 	return false;
 }
 
@@ -2490,14 +2511,99 @@ static bool amt_membership_query_handler(struct amt_dev *amt,
 	return false;
 }
 
+/* The tunnel of the gateway endpoint @saddr:@sport, or NULL. */
+static struct amt_tunnel_list *amt_tunnel_lookup(struct amt_dev *amt,
+						 __be32 saddr, __be16 sport)
+{
+	struct amt_tunnel_list *tunnel;
+
+	list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list)
+		if (tunnel->ip4 == saddr && tunnel->source_port == sport)
+			return tunnel;
+	return NULL;
+}
+
+/* Look up the tunnel for a source whose Membership Update has already been
+ * authenticated, creating it on first contact.
+ *
+ * This is now the only place tunnel state is allocated, so amt->max_tunnels
+ * bounds the number of *verified* gateways rather than the number of
+ * unverified Requests anyone can send.  A fixed per-source cap additionally
+ * stops one verified endpoint from monopolising the table; a single gateway
+ * needs at most one tunnel per family (RFC 7450 s4.2.2).
+ */
+static struct amt_tunnel_list *amt_tunnel_get_or_create(struct amt_dev *amt,
+							__be32 saddr,
+							__be16 sport)
+{
+	struct amt_tunnel_list *tunnel;
+	int per_src = 0;
+	int i;
+
+	tunnel = amt_tunnel_lookup(amt, saddr, sport);
+	if (tunnel)
+		return tunnel;
+
+	spin_lock_bh(&amt->lock);
+
+	/* Re-check under the lock in case a concurrent Update won the race, and
+	 * count how many tunnels this source already holds on the same walk.
+	 */
+	list_for_each_entry(tunnel, &amt->tunnel_list, list) {
+		if (tunnel->ip4 != saddr)
+			continue;
+		if (tunnel->source_port == sport) {
+			spin_unlock_bh(&amt->lock);
+			return tunnel;
+		}
+		per_src++;
+	}
+
+	if (amt->nr_tunnels >= amt->max_tunnels ||
+	    per_src >= AMT_MAX_TUNNELS_PER_SRC) {
+		spin_unlock_bh(&amt->lock);
+		return NULL;
+	}
+
+	tunnel = kzalloc(sizeof(*tunnel) +
+			 (sizeof(struct hlist_head) * amt->hash_buckets),
+			 GFP_ATOMIC);
+	if (!tunnel) {
+		spin_unlock_bh(&amt->lock);
+		return NULL;
+	}
+
+	tunnel->source_port = sport;
+	tunnel->ip4 = saddr;
+	tunnel->amt = amt;
+	spin_lock_init(&tunnel->lock);
+	for (i = 0; i < amt->hash_buckets; i++)
+		INIT_HLIST_HEAD(&tunnel->groups[i]);
+
+	tunnel->last_seen = jiffies;
+	INIT_DELAYED_WORK(&tunnel->gc_wq, amt_tunnel_expire);
+	__amt_update_relay_status(tunnel, AMT_STATUS_RECEIVED_UPDATE, false);
+
+	/* Publish only once the entry is fully initialised. */
+	list_add_tail_rcu(&tunnel->list, &amt->tunnel_list);
+	amt->nr_tunnels++;
+	mod_delayed_work(amt_wq, &tunnel->gc_wq,
+			 msecs_to_jiffies(amt_gmi(amt)) * 3);
+	spin_unlock_bh(&amt->lock);
+
+	return tunnel;
+}
+
 static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
 {
 	struct amt_header_membership_update *amtmu;
 	struct amt_tunnel_list *tunnel;
 	struct ethhdr *eth;
 	struct iphdr *iph;
-	int len, hdr_size;
+	siphash_key_t key[3];
+	int len, hdr_size, i;
 	u64 response_mac;
+	bool verified = false;
 	__be32 saddr;
 	__be32 nonce;
 	__be16 sport;
@@ -2517,33 +2623,57 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
 	/* Snapshot the tunnel endpoint port before the encap is stripped. */
 	sport = udp_hdr(skb)->source;
 
-	if (iptunnel_pull_header(skb, hdr_size, skb->protocol, false))
-		return true;
+	/* Recompute the MAC handed out in the Membership Query rather than
+	 * comparing against a stored copy.  All three live secrets are accepted
+	 * so an exchange straddling one or two rotations by amt_secret_work() is
+	 * not spuriously rejected (three generations cover 120-180s, at least
+	 * the amt_gmi() the Query advertised).  This runs before the packet is
+	 * decapsulated and before any tunnel state is looked up or created, so
+	 * an unverified source is rejected with no further work.
+	 */
+	spin_lock_bh(&amt->lock);
+	for (i = 0; i < ARRAY_SIZE(key); i++)
+		key[i] = amt->key[i];
+	spin_unlock_bh(&amt->lock);
 
-	skb_reset_network_header(skb);
+	for (i = 0; i < ARRAY_SIZE(key); i++) {
+		u64 mac = siphash_3u32((__force u32)saddr,
+				       (__force u32)sport,
+				       (__force u32)nonce,
+				       &key[i]) >> 16;
 
-	list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list) {
-		if (tunnel->ip4 == saddr &&
-		    tunnel->source_port == sport) {
-			if ((nonce == tunnel->nonce &&
-			     response_mac == tunnel->mac)) {
-				/* Refresh liveness only.  gc_wq is re-armed by
-				 * amt_tunnel_expire() itself from last_seen, never
-				 * from here, so this cannot re-queue the work of a
-				 * tunnel that is being freed.
-				 */
-				WRITE_ONCE(tunnel->last_seen, jiffies);
-				goto report;
-			} else {
-				netdev_dbg(amt->dev, "Invalid MAC\n");
-				return true;
-			}
+		if (response_mac == mac) {
+			verified = true;
+			break;
 		}
 	}
 
-	return true;
+	if (!verified) {
+		netdev_dbg(amt->dev, "Invalid MAC\n");
+		return true;
+	}
+
+	tunnel = amt_tunnel_get_or_create(amt, saddr, sport);
+	if (!tunnel) {
+		/* Out of tunnel slots.  Unlike the Request path this reply only
+		 * ever goes to a source that proved it received our Query, so it
+		 * cannot be used to reflect at a spoofed third party.
+		 */
+		icmp_ndo_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_UNREACH, 0);
+		return true;
+	}
+
+	/* Refresh liveness only.  gc_wq is re-armed by amt_tunnel_expire()
+	 * itself from last_seen, never from here, so this cannot re-queue the
+	 * work of a tunnel that is being freed.
+	 */
+	WRITE_ONCE(tunnel->last_seen, jiffies);
+
+	if (iptunnel_pull_header(skb, hdr_size, skb->protocol, false))
+		return true;
+
+	skb_reset_network_header(skb);
 
-report:
 	if (!pskb_may_pull(skb, sizeof(*iph)))
 		return true;
 
@@ -2720,15 +2850,27 @@ static bool amt_discovery_handler(struct amt_dev *amt, struct sk_buff *skb)
 	return false;
 }
 
+/* Handle an AMT Relay Membership Request.
+ *
+ * The source of a Request is unauthenticated: it is a bare UDP datagram with a
+ * trivially spoofable source address.  Allocating tunnel state here let an
+ * attacker fill the table from spoofed addresses, and let one spoofed packet
+ * overwrite the nonce/MAC of an established tunnel and cut that gateway off.
+ *
+ * Nothing needs to be remembered at this point.  Every input to the keyed MAC
+ * is carried in the packet or is device state, so the MAC is generated here,
+ * echoed back by the gateway in its Membership Update, and recomputed and
+ * verified there - see amt_update_handler().  Tunnel state is created only once
+ * that verification succeeds.
+ */
 static bool amt_request_handler(struct amt_dev *amt, struct sk_buff *skb)
 {
 	struct amt_header_request *amtrh;
-	struct amt_tunnel_list *tunnel;
-	unsigned long long key;
 	struct udphdr *udph;
 	struct iphdr *iph;
+	siphash_key_t key;
+	bool full;
 	u64 mac;
-	int i;
 
 	if (!pskb_may_pull(skb, sizeof(*udph) + sizeof(*amtrh)))
 		return true;
@@ -2740,67 +2882,42 @@ static bool amt_request_handler(struct amt_dev *amt, struct sk_buff *skb)
 	if (amtrh->reserved1 || amtrh->reserved2 || amtrh->version)
 		return true;
 
-	list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list)
-		if (tunnel->ip4 == iph->saddr &&
-		    tunnel->source_port == udph->source)
-			goto send;
-
-	spin_lock_bh(&amt->lock);
-	if (amt->nr_tunnels >= amt->max_tunnels) {
-		spin_unlock_bh(&amt->lock);
-		icmp_ndo_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_UNREACH, 0);
-		return true;
-	}
-
-	tunnel = kzalloc(sizeof(*tunnel) +
-			 (sizeof(struct hlist_head) * amt->hash_buckets),
-			 GFP_ATOMIC);
-	if (!tunnel) {
-		spin_unlock_bh(&amt->lock);
+	if (!netif_running(amt->dev) || !netif_running(amt->stream_dev))
 		return true;
-	}
 
-	tunnel->source_port = udph->source;
-	tunnel->ip4 = iph->saddr;
-
-	memcpy(&key, &tunnel->key, sizeof(unsigned long long));
-	tunnel->amt = amt;
-	spin_lock_init(&tunnel->lock);
-	for (i = 0; i < amt->hash_buckets; i++)
-		INIT_HLIST_HEAD(&tunnel->groups[i]);
-
-	tunnel->last_seen = jiffies;
-	INIT_DELAYED_WORK(&tunnel->gc_wq, amt_tunnel_expire);
-
-	list_add_tail_rcu(&tunnel->list, &amt->tunnel_list);
-	tunnel->key = amt->key;
-	__amt_update_relay_status(tunnel, AMT_STATUS_RECEIVED_REQUEST, true);
-	amt->nr_tunnels++;
-	mod_delayed_work(amt_wq, &tunnel->gc_wq,
-			 msecs_to_jiffies(amt_gmi(amt)));
+	/* No state is created for a Request (see amt_update_handler()); read the
+	 * current secret and the table occupancy together under the lock.
+	 */
+	spin_lock_bh(&amt->lock);
+	key = amt->key[0];
+	full = amt->nr_tunnels >= amt->max_tunnels;
 	spin_unlock_bh(&amt->lock);
 
-send:
-	/* source_port is part of the tunnel's identity and is set once, in
-	 * the allocation path above; the lookup only reaches here on an exact
-	 * (address, port) match, so it already equals udph->source.  Do not
-	 * "refresh" it here: doing so let a colliding Request from a new port
-	 * steal an established tunnel outright.
+	/* When the table is full, refuse a source that holds no tunnel without
+	 * sending a Query.  A silent Query would keep the gateway believing the
+	 * exchange is progressing while its later, verified Update is refused;
+	 * withholding it lets the gateway's req_cnt climb and drive rediscovery
+	 * instead.  The ICMP is rate-limited and no state is allocated, so this
+	 * is not the reflector the pre-verification allocation path used to be.
+	 * A source that holds a tunnel is answered: its periodic Request is how
+	 * it renews the response MAC its Updates echo.
 	 */
-	tunnel->nonce = amtrh->nonce;
-	mac = siphash_3u32((__force u32)tunnel->ip4,
-			   (__force u32)tunnel->source_port,
-			   (__force u32)tunnel->nonce,
-			   &tunnel->key);
-	tunnel->mac = mac >> 16;
-
-	if (!netif_running(amt->dev) || !netif_running(amt->stream_dev))
+	if (full && !amt_tunnel_lookup(amt, iph->saddr, udph->source)) {
+		icmp_ndo_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_UNREACH, 0);
 		return true;
+	}
+
+	mac = siphash_3u32((__force u32)iph->saddr,
+			   (__force u32)udph->source,
+			   (__force u32)amtrh->nonce,
+			   &key) >> 16;
 
 	if (!amtrh->p)
-		amt_send_igmp_gq(amt, tunnel);
+		amt_send_igmp_gq(amt, iph->saddr, udph->source, amtrh->nonce,
+				 mac);
 	else
-		amt_send_mld_gq(amt, tunnel);
+		amt_send_mld_gq(amt, iph->saddr, udph->source, amtrh->nonce,
+				mac);
 
 	return false;
 }
@@ -3068,6 +3185,14 @@ static int amt_dev_open(struct net_device *dev)
 	enable_delayed_work(&amt->discovery_wq);
 	enable_delayed_work(&amt->req_wq);
 
+	/* Seed the keys before amt_socket_create() enables the encap receive
+	 * path, so a Request or Update that arrives immediately is never
+	 * verified against an all-zero (predictable) secret.
+	 */
+	spin_lock_bh(&amt->lock);
+	get_random_bytes(&amt->key, sizeof(amt->key));
+	spin_unlock_bh(&amt->lock);
+
 	err = amt_socket_create(amt);
 	if (err) {
 		disable_delayed_work(&amt->req_wq);
@@ -3078,7 +3203,6 @@ static int amt_dev_open(struct net_device *dev)
 	amt->req_cnt = 0;
 	WRITE_ONCE(amt->remote_ip, 0);
 	amt->nonce = 0;
-	get_random_bytes(&amt->key, sizeof(siphash_key_t));
 
 	amt->status = AMT_STATUS_INIT;
 	if (amt->mode == AMT_MODE_GATEWAY) {
diff --git a/include/net/amt.h b/include/net/amt.h
index 0520121..1c7c144 100644
--- a/include/net/amt.h
+++ b/include/net/amt.h
@@ -253,10 +253,6 @@ struct amt_tunnel_list {
 	bool			dead;
 	__be16			source_port;
 	__be32			ip4;
-	__be32			nonce;
-	siphash_key_t		key;
-	u64			mac:48,
-				reserved:16;
 	struct rcu_head		rcu;
 	struct hlist_head	groups[];
 };
@@ -336,8 +332,11 @@ struct amt_dev {
 	struct work_struct	event_wq;
 	/* AMT status */
 	enum amt_status		status;
-	/* Generated key */
-	siphash_key_t		key;
+	/* Generated keys.  key[0] is current; key[1] and key[2] are the two
+	 * previous generations, kept so a Request/Update exchange straddling
+	 * one or two secret rotations still verifies (see amt_secret_work()).
+	 */
+	siphash_key_t		key[3];
 	struct sock	  __rcu *sk;
 	u32			max_groups;
 	u32			max_sources;
@@ -394,6 +393,12 @@ struct amt_dev {
 #define AMT_SECRET_TIMEOUT	60000
 #define IANA_AMT_UDP_PORT	2268
 #define AMT_MAX_TUNNELS         128
+/* Per-source tunnel cap.  A single gateway needs at most one tunnel per
+ * family; this stops one verified source endpoint from monopolising the
+ * AMT_MAX_TUNNELS-slot table.  Deployments that legitimately need more behind
+ * one address (e.g. CGNAT) use the configurable net-next per-source bound.
+ */
+#define AMT_MAX_TUNNELS_PER_SRC 16
 #define AMT_MAX_REQS		128
 #define AMT_GW_HLEN (sizeof(struct iphdr) + \
 		     sizeof(struct udphdr) + \
-- 
2.43.0

Re: [PATCH net v3 0/6] amt: relay tunnel lifetime, validation and request-path hardening

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net v3 0/6] amt: relay tunnel lifetime, validation and request-path hardening

[Omar Ramadan]

Thanks. 4/6-6/6 are carried over from v2, where the same question was
answered [1]. 1/6-3/6 are new in v3, so per patch:

1/6 (gc_wq re-arm use-after-free): theoretical. The Sashiko review of v2
pointed it out, and reading amt_update_handler() against
amt_tunnel_expire() and amt_dev_stop() confirms it. It needs a Membership
Update that has already found the tunnel under RCU to arrive while
amt_tunnel_expire() is freeing that tunnel. I have not triggered it, so
there is no trace to attach. After the patch the Update path no longer
touches gc_wq, so it cannot re-arm a tunnel that is being freed.

2/6 (Query or Data from 0.0.0.0): theoretical, found by inspection while
writing the IPv6 receive path in the net-next series [2]. Sashiko's review
of that series asked for the IPv4 part to be split out for net. While
remote_ip is still 0, "saddr != remote_ip" is false for a 0.0.0.0 source,
so a 0.0.0.0 -> 255.255.255.255 broadcast reaching the socket is accepted.
Not observed.

3/6 (IPv6 literal in an IPv4 attribute): triggered, while writing the IPv6
selftest. An iproute2 without the IPv6 attributes puts all 16 bytes of
"discovery 2001:db8:a::2" into IFLA_AMT_DISCOVERY_IP. On a stock kernel
the command exited 0 with no error or warning, and the device came up as
an IPv4 gateway with discovery address 32.1.13.184, the first four bytes.
There is no oops or trace: the symptom is the silently wrong address,
which the selftest's capability probe was fooled by and now checks for.
With the patch the attribute is refused with an extack.

Fix testing is in the cover letter: W=1 clean, a KASAN + lockdep +
PROVE_RCU boot running the amt selftests with no splat, and amt.sh with
max_tunnels 1.

[1] https://lore.kernel.org/netdev/20261009213749.2226283-1-omar@blockcast.net/
[2] https://lore.kernel.org/netdev/20261009122426.551178-1-omar@blockcast.net/

Re: [PATCH net v3 1/6] amt: fix use-after-free when a Membership Update re-arms a freed tunnel

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 (1) · Medium: 0 · 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).

- [High] The fix stops a concurrent Membership Update from re-arming
  gc_wq, but that Update can still arm other delayed work that points at…
- [High] amt_dev_stop() replaced the RCU-safe list_del_rcu() loop with
  list_splice_init(&amt->tunnel_list, &doomed), which moves the RCU-…
- [Low] At this commit, a tunnel that only ever received a Request lives
  three times as long (3*GMI instead of GMI), and the commit message…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but tunnel
  teardown does not wait for a group_timer callback that is already…

--

amt: fix use-after-free when a Membership Update re-arms a freed tunnel

amt_update_handler() no longer re-arms tunnel->gc_wq. It now stamps a new
tunnel->last_seen field, and amt_tunnel_expire() either re-arms itself or
frees the tunnel under amt->lock. The patch also adds a tunnel->dead flag,
and amt_dev_stop() now marks tunnels dead and splices the tunnel list out
under amt->lock before freeing them.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index b53f8ec556615..b284fa7b7307e 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c

[ ... ]

> @@ -2494,9 +2515,12 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
>  		if (tunnel->ip4 == saddr) {
>  			if ((nonce == tunnel->nonce &&
>  			     response_mac == tunnel->mac)) {
> -				mod_delayed_work(amt_wq, &tunnel->gc_wq,
> -						 msecs_to_jiffies(amt_gmi(amt))
> -								  * 3);
> +				/* Refresh liveness only.  gc_wq is re-armed by
> +				 * amt_tunnel_expire() itself from last_seen, never
> +				 * from here, so this cannot re-queue the work of a
> +				 * tunnel that is being freed.
> +				 */
> +				WRITE_ONCE(tunnel->last_seen, jiffies);
>  				goto report;

[Severity: High]
The Update path no longer re-arms gc_wq. Can it still arm other delayed
work that points at a tunnel being freed?

amt_update_handler() still holds only rcu_read_lock(), and it never checks
the new tunnel->dead flag. After the goto report it does this:

drivers/net/amt.c:amt_update_handler() {
    ...
		spin_lock_bh(&tunnel->lock);
		amt_igmp_report_handler(amt, skb, tunnel);
		spin_unlock_bh(&tunnel->lock);
    ...
}

The MLD branch does the same.

Suppose amt_tunnel_expire() reads last_seen before this WRITE_ONCE() is
visible. It then goes ahead with the teardown:

CPU1 (amt_tunnel_expire)            CPU2 (amt_update_handler)
                                    finds tunnel in RCU walk
deadline = READ_ONCE(last_seen)...
tunnel->dead = true;
list_del_rcu(&tunnel->list);
amt_clear_groups(tunnel);
kfree_rcu(tunnel, rcu);
                                    WRITE_ONCE(tunnel->last_seen, jiffies);
                                    spin_lock_bh(&tunnel->lock);
                                    amt_igmp_report_handler()
                                      amt_igmpv2_report_handler()
                                        amt_add_group()
                                          gnode->tunnel_list = tunnel;
                                          hlist_add_head_rcu(...)
                                        mod_delayed_work(group_timer)
                                        dev_hold(amt->dev)

Nothing deletes that gnode afterwards. Would the gnode, its sources and
the netdev reference all leak?

The tunnel is freed once the grace period ends. When group_timer fires,
amt_group_work() loads gnode->tunnel_list and calls
spin_lock_bh(&tunnel->lock) on the freed tunnel. amt_del_group() also
writes gnode->tunnel_list->nr_groups.

The commit message describes this same class of bug: an Update holding a
cached tunnel pointer arms delayed work that outlives the tunnel.

amt_dev_stop() seems to have the same window. An Update that found the
tunnel before the list_splice_init() can add groups after
amt_clear_groups() has run. This path looks unchanged through the end of
the series.

Could amt_update_handler() check tunnel->dead under tunnel->lock and bail
out before calling the report handlers? Both freers set dead before
amt_clear_groups() takes tunnel->lock, so that check should close the
window.

[ ... ]

> @@ -2732,6 +2756,7 @@ static bool amt_request_handler(struct amt_dev *amt, struct sk_buff *skb)
>  	for (i = 0; i < amt->hash_buckets; i++)
>  		INIT_HLIST_HEAD(&tunnel->groups[i]);
>  
> +	tunnel->last_seen = jiffies;
>  	INIT_DELAYED_WORK(&tunnel->gc_wq, amt_tunnel_expire);
>  
>  	list_add_tail_rcu(&tunnel->list, &amt->tunnel_list);

[Severity: Low]
Does this extend the lifetime of a Request-only tunnel from GMI to 3*GMI?

amt_request_handler() still arms the first expiry at one GMI:

	mod_delayed_work(amt_wq, &tunnel->gc_wq,
			 msecs_to_jiffies(amt_gmi(amt)));

When that work fires, amt_tunnel_expire() computes:

	deadline = READ_ONCE(tunnel->last_seen) +
		   msecs_to_jiffies(amt_gmi(amt)) * 3;

The deadline has not passed yet, so it re-arms for roughly another 2*GMI.
Before this patch, a tunnel that only received a Request was freed after
GMI. Unauthenticated Requests can now hold max_tunnels slots three times
longer, and the commit message does not mention the change.

The new comment in struct amt_tunnel_list also describes last_seen as the
"Last time a valid Membership Update refreshed this tunnel". At this
commit the Request path writes it as well.

A later commit in the series, "amt: do not create tunnel state for
unauthenticated Requests", seems to fix this. That commit stops the
Request path from allocating tunnels, which also makes the struct comment
accurate.

[ ... ]

> @@ -3053,6 +3078,7 @@ static int amt_dev_stop(struct net_device *dev)
>  	struct amt_tunnel_list *tunnel, *tmp;
>  	struct sk_buff *skb;
>  	struct sock *sk;
> +	LIST_HEAD(doomed);
>  	int i;
>  
>  	disable_delayed_work_sync(&amt->req_wq);
> @@ -3079,9 +3105,19 @@ static int amt_dev_stop(struct net_device *dev)
>  	amt->req_cnt = 0;
>  	WRITE_ONCE(amt->remote_ip, 0);
>  
> -	list_for_each_entry_safe(tunnel, tmp, &amt->tunnel_list, list) {
> -		list_del_rcu(&tunnel->list);
> -		amt->nr_tunnels--;

[ ... ]

> +	spin_lock_bh(&amt->lock);
> +	list_for_each_entry(tunnel, &amt->tunnel_list, list)
> +		tunnel->dead = true;
> +	list_splice_init(&amt->tunnel_list, &doomed);
> +	amt->nr_tunnels = 0;
> +	spin_unlock_bh(&amt->lock);

[Severity: High]
Is list_splice_init() safe here while RX readers may still be walking
amt->tunnel_list with list_for_each_entry_rcu()?

The old loop used list_del_rcu(). After this splice, the last tunnel's
->next points at the on-stack doomed head. amt->tunnel_list is
reinitialised to empty, with no grace period in between.

The only grace period in amt_dev_stop() is the synchronize_net() before
udp_tunnel_sock_release(). udp_tunnel_sock_release() clears sk_user_data
with rcu_assign_sk_user_data(sk, NULL), then shuts down and releases the
socket without waiting for readers.

So an amt_rcv() call that already loaded amt from sk_user_data could
still be inside amt_request_handler() or amt_update_handler():

	list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list) {
		if (tunnel->ip4 == saddr) {

A reader that has already passed the last tunnel would never reach the
terminator, &amt->tunnel_list. Would it then treat &doomed as a struct
amt_tunnel_list and read ip4 from stack memory past the list_head? It
would then follow doomed.next back to the first tunnel and loop in softirq
under rcu_read_lock().

That would stall RCU, which also holds up the pending kfree_rcu() calls.
Once amt_dev_stop() returns and its stack frame is reused, the reader
follows garbage pointers. If the stack bytes happen to match saddr, the
send: path in amt_request_handler() writes tunnel->nonce through the bogus
pointer.

There is a second problem. A reader that reaches the allocation in
amt_request_handler() after the splice (amt_tunnel_get_or_create() later
in the series) can list_add_tail_rcu() a new tunnel onto the emptied
amt->tunnel_list. Nothing in this stop path frees that tunnel.

Would list_splice_init_rcu(), which waits for a grace period, or keeping
list_del_rcu() work better here? This code also looks unchanged at the end
of the series.

> +
> +	list_for_each_entry_safe(tunnel, tmp, &doomed, list) {
>  		cancel_delayed_work_sync(&tunnel->gc_wq);
>  		amt_clear_groups(tunnel);
>  		kfree_rcu(tunnel, rcu);

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Does this
teardown wait for a gnode->group_timer callback that is already running?

amt_clear_groups() calls amt_del_group() for each group under
tunnel->lock. amt_del_group() only does a non-synchronous cancel:

drivers/net/amt.c:amt_del_group() {
	if (cancel_delayed_work(&gnode->group_timer))
		dev_put(amt->dev);
	hlist_del_rcu(&gnode->node);
	gnode->tunnel_list->nr_groups--;
	...
	kfree_rcu(gnode, rcu);
}

If amt_group_work() is already running, cancel_delayed_work() returns
false. amt_group_work() loads the tunnel pointer outside any RCU read
section:

	struct amt_tunnel_list *tunnel = gnode->tunnel_list;
	...
	spin_lock_bh(&tunnel->lock);

If the callback is preempted before spin_lock_bh(), could the grace period
finish so that it takes tunnel->lock inside a freed tunnel?

If the memory has not been freed yet, the callback takes tunnel->lock once
amt_clear_groups() drops it. For an EXCLUDE mode group the source lists
are empty by then, so delete_group stays true and amt_del_group() runs a
second time.

Would that call hlist_del_rcu() again on a node whose pprev is already
LIST_POISON2? It would also decrement nr_groups twice and call
kfree_rcu(gnode) twice.

amt_tunnel_expire() has the same issue, since it also calls
amt_clear_groups() before kfree_rcu(tunnel). Only gc_wq is cancelled
synchronously here; the per-group work is never waited for.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012125943.558629-1-omar%40blockcast.net

Re: [PATCH net v3 3/6] amt: reject an IPv6 address passed in an IPv4 address attribute

[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

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 fix does not close the bug class its commit message
  describes.

--

amt: reject an IPv6 address passed in an IPv4 address attribute

This patch adds amt_ip6_in_ip4_attr() and calls it from amt_validate().
amt_validate() then rejects an IFLA_AMT_LOCAL_IP or IFLA_AMT_DISCOVERY_IP
attribute whose payload is exactly 16 bytes. The aim is to stop
nla_get_in_addr() from silently truncating an IPv6 literal to an IPv4
address.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index c3716bad251f6..550b0a5e73766 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -3194,6 +3194,15 @@ static const struct nla_policy amt_policy[IFLA_AMT_MAX + 1] = {
>  	[IFLA_AMT_MAX_TUNNELS]	= { .type = NLA_U32 },
>  };
>  
> +static bool amt_ip6_in_ip4_attr(const struct nlattr *attr,
> +				struct netlink_ext_ack *extack)
> +{
> +	if (!attr || nla_len(attr) != sizeof(struct in6_addr))
> +		return false;

[Severity: High]
Can other oversized lengths still get through and be truncated?

The commit message names these policy entries as the root cause, but the
patch does not change them:

drivers/net/amt.c:amt_policy[] {
    ...
	[IFLA_AMT_LOCAL_IP]	= { .len = sizeof_field(struct iphdr, daddr) },
	...
	[IFLA_AMT_DISCOVERY_IP]	= { .len = sizeof_field(struct iphdr, daddr) },
    ...
}

These entries are NLA_UNSPEC, so validate_nla() in lib/nlattr.c treats .len
only as a minimum length:

	if (attrlen < minlen)
		goto out_err;

Some lengths still pass the policy: 5 to 15 bytes, and 17 bytes or more.
amt_ip6_in_ip4_attr() returns false for these because the length is not
exactly 16.

amt_newlink() then truncates the value in the same way:

	amt->local_ip = nla_get_in_addr(data[IFLA_AMT_LOCAL_IP]);
	...
	amt->discovery_ip = nla_get_in_addr(data[IFLA_AMT_DISCOVERY_IP]);

The device is then created with an address that was never requested.

Would it be better to change these two policy entries to
NLA_POLICY_EXACT_LEN(sizeof(__be32)) or NLA_BE32? That would reject every
wrong length with a standard bad-attribute extack, and the open-coded
helper would no longer be needed. amt_get_size() and amt_fill_info()
already emit exactly sizeof(__be32) for these attributes.

The problem is still there at the end of the series. The last patch, "amt:
do not create tunnel state for unauthenticated Requests", leaves both
amt_policy and amt_ip6_in_ip4_attr() unchanged.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012125943.558629-1-omar%40blockcast.net

Re: [PATCH net v3 4/6] amt: key relay tunnel state on the (address, port) endpoint, not the address

[netdev-bot+sashiko]

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

Critical: 1 · High: 1 · Medium: 1 (1) · Low: 0 (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).

- [Critical] The Update-versus-expiry use-after-free that the commit
  "amt: fix use-after-free when a Membership Update re-arms a freed…
- [High] In amt_dev_stop() (drivers/net/amt.c), the tunnel list is moved
  onto a local stack list with list_splice_init() while RCU receive…
- [Medium] At the reviewed commit, keying tunnels on (address, port) in
  amt_request_handler() removes the implicit limit of one tunnel per…

Pre-existing issues:
- [Medium] At the reviewed commit, amt_request_handler() does a lockless
  RCU lookup and then allocates and inserts a tunnel under amt->lock…
- [Low] At the reviewed commit, the send: path in amt_request_handler()
  writes tunnel->nonce and tunnel->mac (a u64:48 bitfield) with no lock,…

--

amt: key relay tunnel state on the (address, port) endpoint, not the address

This commit adds the UDP source port to the relay tunnel lookups in
amt_request_handler() and amt_update_handler(), so tunnels are keyed on
the (address, port) endpoint described in RFC 7450 s4.2.2.
amt_update_handler() saves the source port before iptunnel_pull_header()
strips the encapsulation. The commit message notes that this removes the
implicit one-tunnel-per-address bound, which is to be restored separately.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 550b0a5e73766..2feb418e87edb 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c

[ ... ]

> @@ -2512,7 +2515,8 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
>  	skb_reset_network_header(skb);
>  
>  	list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list) {
> -		if (tunnel->ip4 == saddr) {
> +		if (tunnel->ip4 == saddr &&
> +		    tunnel->source_port == sport) {

[Severity: High]
This patch doesn't change it directly, but this lockless walk of
amt->tunnel_list can now race with the list_splice_init() that
"amt: fix use-after-free when a Membership Update re-arms a freed tunnel"
added to amt_dev_stop() earlier in this series. Is that safe?

amt_dev_stop() has one grace period, and it runs before
udp_tunnel_sock_release() clears sk_user_data:

drivers/net/amt.c:amt_dev_stop() {
	...
	RCU_INIT_POINTER(amt->sk, NULL);
	synchronize_net();
	if (sk)
		udp_tunnel_sock_release(sk);
	...
	spin_lock_bh(&amt->lock);
	list_for_each_entry(tunnel, &amt->tunnel_list, list)
		tunnel->dead = true;
	list_splice_init(&amt->tunnel_list, &doomed);
	amt->nr_tunnels = 0;
	spin_unlock_bh(&amt->lock);
	...
}

udp_tunnel_sock_release() calls rcu_assign_sk_user_data(sk, NULL) but does
not wait for receivers that are already running. A Membership Update that
arrives after the synchronize_net() can therefore still get amt in
amt_rcv(). amt_update_handler() has no netif_running() check before this
walk.

Suppose the reader is mid-walk when the splice happens. The last entry's
next pointer then points at &doomed on amt_dev_stop()'s stack, and
doomed.next points back to the first tunnel. The loop only stops at
&amt->tunnel_list. If no key matches, wouldn't the reader treat &doomed as
a struct amt_tunnel_list, read ip4/source_port from the stack, and keep
looping?

Once amt_dev_stop() returns and that stack slot is reused, the reader
follows garbage pointers. If the garbage happens to match the key, the
WRITE_ONCE(tunnel->last_seen, jiffies) below writes through it. The
baseline removed each entry with list_del_rcu(), which did not have this
problem.

There is a second issue in the final state of the series. A lockless miss
in amt_tunnel_get_or_create() falls through to the locked path. That path
sees the now-empty list with nr_tunnels == 0, so it can add a new tunnel
with gc_wq armed after amt_dev_stop() has finished its cleanup pass.

>  			if ((nonce == tunnel->nonce &&
>  			     response_mac == tunnel->mac)) {
>  				/* Refresh liveness only.  gc_wq is re-armed by

[Severity: Critical]
Can a Membership Update still add a group to a tunnel that
amt_tunnel_expire() has already torn down?

The tunnel is found here without a lock. The Update then only does
WRITE_ONCE(tunnel->last_seen, jiffies) before report processing. Nothing
checks tunnel->dead under amt->lock or tunnel->lock.

Meanwhile, amt_tunnel_expire() can read the old last_seen under amt->lock
and retire the tunnel:

drivers/net/amt.c:amt_tunnel_expire() {
	...
	tunnel->dead = true;
	rcu_read_lock();
	list_del_rcu(&tunnel->list);
	amt->nr_tunnels--;
	amt_clear_groups(tunnel);
	rcu_read_unlock();
	spin_unlock_bh(&amt->lock);
	kfree_rcu(tunnel, rcu);
}

The Update may take tunnel->lock after amt_clear_groups() has finished. In
that case amt_igmp_report_handler()->amt_igmpv2_report_handler()->
amt_add_group() links a new gnode into the dead tunnel's groups[] and arms
gnode->group_timer for amt_gmi() ms.

RCU keeps the tunnel alive only until the Update's amt_rcv() read section
ends. When group_timer fires later:

amt_group_work()
    struct amt_tunnel_list *tunnel = gnode->tunnel_list;
    ...
    spin_lock_bh(&tunnel->lock);
    ...
    amt_del_group()
        gnode->tunnel_list->nr_groups--;

Don't both of these access the freed tunnel, one of them as a write?

A gateway that received the relay's Query can produce a valid MAC. It can
let the tunnel approach last_seen + 3 * amt_gmi() and then send IGMPv2
joins in Membership Updates around the expiry time.

The baseline already had this group_timer race. However, "amt: fix
use-after-free when a Membership Update re-arms a freed tunnel" earlier in
this series says it closes the Update-versus-expiry use-after-free, and it
only replaced the gc_wq re-arm with a timestamp. The same structure
remains at the end of the series.

[ ... ]

> @@ -2729,7 +2733,8 @@ static bool amt_request_handler(struct amt_dev *amt, struct sk_buff *skb)
>  		return true;
>  
>  	list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list)
> -		if (tunnel->ip4 == iph->saddr)
> +		if (tunnel->ip4 == iph->saddr &&
> +		    tunnel->source_port == udph->source)
>  			goto send;

[Severity: Medium]
With the port in this lookup, a Request from a new source port always
misses and goes on to allocate a tunnel. The Request is not authenticated
at this point, and only the global nr_tunnels >= max_tunnels check
applies. Can one host take every tunnel slot just by changing its source
port?

By joining the same group on each of those tunnels, the host could also
make amt_dev_xmit() send many copies of the same data toward one address.

The commit message says the per-source bound will come back in a separate
net-next patch. Within this series, "amt: do not create tunnel state for
unauthenticated Requests" later removes allocation from the Request path.
Tunnels are then created only after MAC verification, in
amt_tunnel_get_or_create(), and that commit adds a per-source cap checked
under amt->lock (AMT_MAX_TUNNELS_PER_SRC, 16) with no new UAPI.

Should that bound be part of this patch, or should this patch be ordered
after that change?

>  
>  	spin_lock_bh(&amt->lock);

[Severity: Medium]
This is a pre-existing issue that this patch did not introduce, but the
insert under amt->lock does not repeat the (ip4, source_port) lookup. If
two Requests from the same endpoint are handled at once, could both miss
the lockless walk and each insert a tunnel with the same key?

amt_update_handler() stops at the first key match. If that tunnel's
nonce/mac belong to the other Request, the gateway's Updates are dropped
at "Invalid MAC" until it sends its next Request.

Later in this series, "amt: do not create tunnel state for unauthenticated
Requests" removes allocation from this path. Its
amt_tunnel_get_or_create() repeats the walk under amt->lock before
inserting.

[ ... ]

> @@ -2768,6 +2773,12 @@ static bool amt_request_handler(struct amt_dev *amt, struct sk_buff *skb)
>  	spin_unlock_bh(&amt->lock);
>  
>  send:
> +	/* source_port is part of the tunnel's identity and is set once, in
> +	 * the allocation path above; the lookup only reaches here on an exact
> +	 * (address, port) match, so it already equals udph->source.  Do not
> +	 * "refresh" it here: doing so let a colliding Request from a new port
> +	 * steal an established tunnel outright.
> +	 */
>  	tunnel->nonce = amtrh->nonce;
>  	mac = siphash_3u32((__force u32)tunnel->ip4,
>  			   (__force u32)tunnel->source_port,

[Severity: Low]
This isn't a bug introduced by this patch, but tunnel->nonce and
tunnel->mac (a 48-bit bitfield) are written here without a lock.
amt_update_handler() reads the same pair without a lock as its
authenticity check:

	if ((nonce == tunnel->nonce &&
	     response_mac == tunnel->mac)) {

Could two concurrent Requests, or a Request racing an Update, leave or
observe a mixed nonce/mac pair, so that a valid Update gets rejected?

Later in this series, "amt: do not create tunnel state for unauthenticated
Requests" removes the stored per-tunnel nonce/mac/key. After that change,
the Update path recomputes the MAC using a copy of amt->key[] taken under
amt->lock.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012125943.558629-1-omar%40blockcast.net

Re: [PATCH net v3 5/6] amt: make pre-query report drops visible

[netdev-bot+sashiko]

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

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

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 gateway mode, amt_dev_xmit() (drivers/net/amt.c) counts
  every successfully forwarded IGMP/MLD report as a TX drop.
- [Low] The new netdev_dbg in amt_dev_xmit() (drivers/net/amt.c:1259)
  says "drop %s report: no Membership Query for this family yet"…
- [Low] net: amt: lockless non-atomic increments of dev->stats in tx path

--

amt: make pre-query report drops visible

This adds a family-specific netdev_dbg() to amt_dev_xmit(). It fires when
a gateway drops an IGMP or MLD report because the relay's Membership Query
for that family has not arrived yet. The commit message says the existing
drop path already increments tx_dropped, so each discarded report is
counted once.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 2feb418e87edb..fec32650cb91f 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -1249,9 +1249,17 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>  		/* Gateway only passes IGMP/MLD packets */
>  		if (!report)
>  			goto free;
> +		/* A validated report can only be forwarded after the relay's
> +		 * family-specific Membership Query supplies the state echoed
> +		 * by the Membership Update. Log this readiness failure before
> +		 * the shared drop path accounts it.
> +		 */
>  		if ((!v6 && !READ_ONCE(amt->ready4)) ||
> -		    (v6 && !READ_ONCE(amt->ready6)))
> +		    (v6 && !READ_ONCE(amt->ready6))) {
> +			netdev_dbg(dev, "drop %s report: no Membership Query for this family yet\n",
> +				   v6 ? "MLD" : "IGMP");

[Severity: Low]
This isn't introduced by this patch, but the commit message relies on
tx_dropped to count each discarded report exactly once. Is that counter
reliable here?

amt_link_setup() sets dev->lltx = true along with IFF_NO_QUEUE. So the
core does not take HARD_TX_LOCK around ndo_start_xmit, and
amt_dev_xmit() can run on several CPUs at once.

Both drop paths in amt_dev_xmit() update the counters with plain
increments:

	dev->stats.tx_errors++;
	...
	dev->stats.tx_dropped++;

These are non-atomic read-modify-write operations on unsigned long.
Two CPUs dropping packets at the same time can lose increments. The
only effect is inaccurate statistics, but it means "accounted exactly
once" doesn't hold under concurrent transmit.

Would it make sense to switch these to DEV_STATS_INC(dev, tx_dropped)
and DEV_STATS_INC(dev, tx_errors)? Those use atomic_long_inc() on the
same fields. Another option is to move the driver to per-CPU stats.
That is probably a separate fix rather than something for this patch.

[Severity: Low]
This isn't a bug, but could the wording of this message be a bit more
precise?

ready4 and ready6 are false before the first Query, and they are also
cleared on every refresh cycle in amt_event_send_request():

	if (!amt->req_cnt) {
		WRITE_ONCE(amt->ready4, false);
		WRITE_ONCE(amt->ready6, false);
		get_random_bytes(&amt->nonce, sizeof(__be32));
	}

They are cleared again on the relay-lost path, when req_cnt exceeds
AMT_MAX_REQ_COUNT.

The nonce is regenerated at the same time, and
amt_membership_query_handler() rejects Queries with a stale nonce. So the
message is correct for the current Request.

Still, an operator who sees it in steady state might read "no Membership
Query for this family yet" as meaning the relay never answered. Would
something like "waiting for Membership Query" be clearer?

> +			goto free;
> +		}
>  		if (amt_send_membership_update(amt, skb,  v6))
>  			goto free;
>  		goto unlock;

[Severity: Medium]
The commit message says:

    That path already increments tx_dropped, so each discarded report
    is accounted exactly once.

The new comment also refers to "the shared drop path". Can tx_dropped
actually be used to count these drops?

When amt_send_membership_update() returns false, it has already handed
the skb to the tunnel:

	udp_tunnel_xmit_skb(rt, sk, skb, ...);
	amt_update_gw_status(amt, AMT_STATUS_SENT_UPDATE, true);
	return false;

The caller then does goto unlock. That label reaches the same increment
that the real drops use:

free:
	dev_kfree_skb(skb);
unlock:
	dev->stats.tx_dropped++;
	return NETDEV_TX_OK;

So in gateway mode, every IGMP/MLD report that is forwarded successfully
is also counted in tx_dropped. That doesn't match the rtnl_link_stats64
definition of tx_dropped ("Number of packets dropped on their way to
transmission").

The success-path increment has been there since commit cbc21dc1cfe9
("amt: add data plane of amt interface"). This patch, though, depends on
tx_dropped to count the pre-Query drops. How would the counter tell those
drops apart from reports that were sent?

Should the goto unlock success path skip the tx_dropped increment?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012125943.558629-1-omar%40blockcast.net

Re: [PATCH net v3 6/6] amt: do not create tunnel state for unauthenticated Requests

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (3) · Medium: 3 · 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] The patch says three secret generations keep a response MAC
  valid "for 120-180s, at least the amt_gmi() the Query advertised".
- [Medium] amt_request_handler() still sends
  icmp_ndo_send(ICMP_DEST_UNREACH, ICMP_HOST_UNREACH) to the claimed,…
- [Medium] The patch withholds the Query when a later verified Update
  could not be admitted.
- [Low] The comment on AMT_MAX_TUNNELS_PER_SRC in include/net/amt.h says
  deployments needing more than 16 tunnels behind one address "use the…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it remains on
  the path the patch rewrites.
- [High] This problem wasn't introduced by this patch, but tunnel
  teardown can free a group and its tunnel while amt_source_work() is…
- [High] This problem wasn't introduced by this patch, but the patch
  moves tunnel allocation to a site where it stays open.

--

amt: do not create tunnel state for unauthenticated Requests

This patch stops amt_request_handler() from allocating tunnel state for
unauthenticated Membership Requests. amt_update_handler() now recomputes
the keyed MAC from packet fields against three rotating secrets. Tunnel
state is created only after an Update verifies, in the new
amt_tunnel_get_or_create(), which also enforces a fixed per-source cap.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index fec32650cb91f..2d9f2d95bd959 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c

[ ... ]

> @@ -925,7 +934,13 @@ static void amt_secret_work(struct work_struct *work)
>  					   secret_wq);
>  
>  	spin_lock_bh(&amt->lock);
> -	get_random_bytes(&amt->key, sizeof(siphash_key_t));
> +	/* Rotate: keep the two previous generations so a MAC handed out just
> +	 * before a rotation still verifies for up to three AMT_SECRET_TIMEOUT
> +	 * windows (120-180s), covering the amt_gmi() the Query advertised.
> +	 */
> +	amt->key[2] = amt->key[1];
> +	amt->key[1] = amt->key[0];
> +	get_random_bytes(&amt->key[0], sizeof(siphash_key_t));

[Severity: Medium]
Do three generations actually cover amt_gmi()?

AMT_SECRET_TIMEOUT is 60000 ms and only key[0..2] are kept. A MAC minted
just before a rotation therefore verifies for as little as 120s, and at
most 180s.

With the relay defaults from amt_newlink() (qrv = 2, qi = 125, qri = 10),
amt_gmi() is ((2 * 125) + 10) * 1000 = 260s. The commit message quotes
that figure itself.

Even the 120s floor is shorter than the 125s QQIC that amt_build_igmp_gq()
advertises:

    ihv3->qqic	= amt->qi;

The commit message says:

    Three generations keep a MAC valid for 120-180s, at least the
    amt_gmi() the Query advertised; two generations (120s floor) could
    expire a MAC before the interval it promised.

The same claim is repeated in the new amt_update_handler() comment. Also,
two generations would give a 60s floor, not 120s.

Before this patch, tunnel->mac was stored and stayed valid until the
gateway's next Request replaced it. Take a gateway that refreshes its
Request at the advertised 125s QQIC and got its MAC just before a
rotation. Won't its Updates in the last few seconds of each interval be
dropped in amt_update_handler() as "Invalid MAC", even on an established
tunnel?

The Linux gateway re-requests often enough to avoid this. Third-party
gateways that pace Requests by QQIC would not.

>  	spin_unlock_bh(&amt->lock);

[ ... ]

> @@ -2490,14 +2511,99 @@ static bool amt_membership_query_handler(struct amt_dev *amt,

[ ... ]

> +static struct amt_tunnel_list *amt_tunnel_get_or_create(struct amt_dev *amt,
> +							__be32 saddr,
> +							__be16 sport)
> +{
> +	struct amt_tunnel_list *tunnel;
> +	int per_src = 0;
> +	int i;
> +
> +	tunnel = amt_tunnel_lookup(amt, saddr, sport);
> +	if (tunnel)
> +		return tunnel;

[Severity: High]
This isn't a bug introduced by this patch, but this lockless lookup can
return a tunnel that amt_tunnel_expire() has already marked dead. Does
anything stop the Update path from adding groups to it?

amt_update_handler() takes tunnel->lock and calls
amt_igmp_report_handler() or amt_mld_report_handler(). These can call
amt_add_group(), which stores the tunnel pointer in gnode->tunnel_list and
arms gnode->group_timer with dev_hold(). Nothing on that path checks
tunnel->dead.

A possible interleaving:

CPU1 (amt_update_handler)            CPU2 (amt_tunnel_expire)
amt_tunnel_lookup() finds tunnel
                                     reads stale last_seen
                                     tunnel->dead = true;
                                     list_del_rcu(&tunnel->list);
                                     amt_clear_groups(tunnel);
WRITE_ONCE(tunnel->last_seen, ...);
spin_lock_bh(&tunnel->lock);
amt_igmp_report_handler()
    amt_add_group()   /* new gnode on dying tunnel */
                                     kfree_rcu(tunnel, rcu);

When the orphaned group_timer fires later:

amt_group_work()
    struct amt_tunnel_list *tunnel = gnode->tunnel_list;
    ...
    spin_lock_bh(&tunnel->lock);    /* tunnel already freed */

The same window exists for Updates still in flight while amt_dev_stop()
runs amt_clear_groups().

The comment above WRITE_ONCE(tunnel->last_seen, jiffies) says this "cannot
re-queue the work of a tunnel that is being freed". That holds for gc_wq,
but not for group_timer.

Would checking READ_ONCE(tunnel->dead) under tunnel->lock in the Update
path, and bailing out, close this? ->dead is set before amt_clear_groups()
takes tunnel->lock.

> +
> +	spin_lock_bh(&amt->lock);

[ ... ]

> +	if (amt->nr_tunnels >= amt->max_tunnels ||
> +	    per_src >= AMT_MAX_TUNNELS_PER_SRC) {
> +		spin_unlock_bh(&amt->lock);
> +		return NULL;
> +	}

[Severity: High]
This is a pre-existing issue, but the patch moves tunnel allocation here.
Neither amt_update_handler() nor amt_tunnel_get_or_create() checks whether
the device is running or stopping. Can a tunnel be created after
amt_dev_stop() has drained the list?

amt_dev_stop() does:

    RCU_INIT_POINTER(amt->sk, NULL);
    synchronize_net();
    if (sk)
        udp_tunnel_sock_release(sk);
    ...
    spin_lock_bh(&amt->lock);
    list_for_each_entry(tunnel, &amt->tunnel_list, list)
        tunnel->dead = true;
    list_splice_init(&amt->tunnel_list, &doomed);
    amt->nr_tunnels = 0;
    spin_unlock_bh(&amt->lock);

udp_tunnel_sock_release() clears sk_user_data but doesn't wait for
amt_rcv() callers that already fetched amt. A verified Update received
between synchronize_net() and that clear can get here after the splice.
It would then list_add_tail_rcu() a new tunnel and arm its gc_wq for
3 * amt_gmi().

This stop never tears that tunnel down. amt_dev_uninit() and amt_dellink()
do no tunnel cleanup, and needs_free_netdev is set. After the device is
deleted, amt_tunnel_expire() would do spin_lock_bh(&tunnel->amt->lock) on
a freed amt_dev.

Separately, list_splice_init() is not RCU-safe on the RCU-traversed
tunnel_list. A concurrent amt_tunnel_lookup() or amt_dev_xmit() reader at
the last entry can follow ->next to the on-stack doomed head. The splice
came from an earlier commit in this series.

__LINK_STATE_START is cleared before ndo_stop. Would a netif_running()
check at the start of amt_update_handler(), or under amt->lock here, close
the creation race?

[ ... ]

> +	/* Publish only once the entry is fully initialised. */
> +	list_add_tail_rcu(&tunnel->list, &amt->tunnel_list);
> +	amt->nr_tunnels++;
> +	mod_delayed_work(amt_wq, &tunnel->gc_wq,
> +			 msecs_to_jiffies(amt_gmi(amt)) * 3);
> +	spin_unlock_bh(&amt->lock);

[Severity: High]
This isn't a bug introduced by this patch. Tunnels created here are torn
down later by amt_tunnel_expire() or amt_dev_stop(). Can that teardown
free a group and its tunnel while amt_source_work() is still running for
one of the group's sources?

amt_source_work() reads the parent pointer before it takes any lock or
RCU read lock:

    tunnel = gnode->tunnel_list;
    spin_lock_bh(&tunnel->lock);
    rcu_read_lock();

The teardown path is amt_clear_groups()->amt_del_group()->
amt_destroy_source(), and amt_destroy_source() only cancels
asynchronously:

    cancel_delayed_work(&snode->source_timer);
    hlist_del_init_rcu(&snode->node);

kfree_rcu(gnode, rcu) and kfree_rcu(tunnel, rcu) are then queued while
the source work may still be running.

With CONFIG_PREEMPT, the worker can be preempted between reading
gnode->tunnel_list and spin_lock_bh(). It is not in an RCU reader at that
point, so the grace period can complete and spin_lock_bh(&tunnel->lock)
runs on freed memory.

Without preemption, the worker can be spinning on tunnel->lock while
amt_clear_groups() holds it. It then calls amt_destroy_source() a second
time on an snode that is already on source_gc_list. hlist_del_init_rcu()
unlinks it from source_gc_list without source_gc_lock, racing
__amt_source_gc_work(), and nr_sources is decremented twice.

amt_dev_stop() only syncs gc_wq and never drains these source works.

[ ... ]

> @@ -2740,67 +2882,42 @@ static bool amt_request_handler(struct amt_dev *amt, struct sk_buff *skb)

[ ... ]

> +	spin_lock_bh(&amt->lock);
> +	key = amt->key[0];
> +	full = amt->nr_tunnels >= amt->max_tunnels;
>  	spin_unlock_bh(&amt->lock);

[ ... ]

> +	/* When the table is full, refuse a source that holds no tunnel without
> +	 * sending a Query.  A silent Query would keep the gateway believing the
> +	 * exchange is progressing while its later, verified Update is refused;
> +	 * withholding it lets the gateway's req_cnt climb and drive rediscovery
> +	 * instead.  The ICMP is rate-limited and no state is allocated, so this
> +	 * is not the reflector the pre-verification allocation path used to be.
> +	 * A source that holds a tunnel is answered: its periodic Request is how
> +	 * it renews the response MAC its Updates echo.
>  	 */

[Severity: Medium]
Should the full check above also account for AMT_MAX_TUNNELS_PER_SRC?

It only looks at the global limit. amt_tunnel_get_or_create() also refuses
when per_src >= AMT_MAX_TUNNELS_PER_SRC.

Suppose a source address already holds 16 tunnels and sends a Request
from a new port, for example another gateway behind the same NAT address:

  amt_request_handler() answers with a Query
  gateway: amt_membership_query_handler() sets ready4/ready6, req_cnt = 0
  amt_update_handler()->amt_tunnel_get_or_create() returns NULL
  amt_update_handler() sends ICMP host unreachable
  gateway: amt_err_lookup() does mod_delayed_work(amt_wq, &amt->req_wq, 0)
  next Request is answered with another Query, req_cnt reset again

Doesn't this loop indefinitely? It is the "silent Query" case this comment
says it avoids, and the gateway never reaches rediscovery.

[ ... ]

> -	if (!netif_running(amt->dev) || !netif_running(amt->stream_dev))
> +	if (full && !amt_tunnel_lookup(amt, iph->saddr, udph->source)) {
> +		icmp_ndo_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_UNREACH, 0);
>  		return true;
> +	}

[Severity: Medium]
Is this still an ICMP reflector toward spoofed sources?

A spoofed victim address never holds a tunnel. So every spoofed Request
that arrives while the table is full still sends ICMP_DEST_UNREACH to its
claimed source, using the same icmp_ndo_send() call as the old full-table
path.

Filling the table now takes verified tunnels. With AMT_MAX_TUNNELS_PER_SRC
at 16, that is 128 / 16 = 8 real IP addresses, or just a table that is
legitimately full of real gateways.

The comment says "is not the reflector the pre-verification allocation
path used to be". The commit message says "The ICMP is rate-limited and
allocates nothing, unlike the old reflector".

The old full-table path also returned before kzalloc() and had the same
icmp_global/inet_peer rate limiting. Also, the ICMP path can allocate: for
each new spoofed destination,
__icmp_send()->icmpv4_xrlim_allow()->inet_getpeer_v4() can create an
inet_peer.

Each spoofed Request also uses up part of the host-wide icmp_msgs_per_sec
budget.

Are these claims in the comment and commit message accurate?

[ ... ]

> diff --git a/include/net/amt.h b/include/net/amt.h
> index 0520121ded649..1c7c144e4db31 100644
> --- a/include/net/amt.h
> +++ b/include/net/amt.h

[ ... ]

> @@ -394,6 +393,12 @@ struct amt_dev {
>  #define AMT_SECRET_TIMEOUT	60000
>  #define IANA_AMT_UDP_PORT	2268
>  #define AMT_MAX_TUNNELS         128
> +/* Per-source tunnel cap.  A single gateway needs at most one tunnel per
> + * family; this stops one verified source endpoint from monopolising the
> + * AMT_MAX_TUNNELS-slot table.  Deployments that legitimately need more behind
> + * one address (e.g. CGNAT) use the configurable net-next per-source bound.
> + */
> +#define AMT_MAX_TUNNELS_PER_SRC 16

[Severity: Low]
Which configurable per-source bound does this comment refer to?

This tree exposes only IFLA_AMT_MAX_TUNNELS in include/uapi/linux/amt.h,
and there is no module parameter. The check in amt_tunnel_get_or_create()
is unconditional, so a relay with more than 16 gateways behind one public
address has no way to raise it.

This patch carries a Fixes: tag and is described as backportable. Should
the comment avoid pointing stable users at a mechanism that does not exist
here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012125943.558629-1-omar%40blockcast.net

Re: [PATCH net v3 1/6] amt: fix use-after-free when a Membership Update re-arms a freed tunnel

[Taehee Yoo]

On Tue, Oct 13, 2026 at 11:01 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
>
> Critical: 0 · High: 2 (1) · Medium: 0 · 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).
>
> - [High] The fix stops a concurrent Membership Update from re-arming
>   gc_wq, but that Update can still arm other delayed work that points at…
> - [High] amt_dev_stop() replaced the RCU-safe list_del_rcu() loop with
>   list_splice_init(&amt->tunnel_list, &doomed), which moves the RCU-…
> - [Low] At this commit, a tunnel that only ever received a Request lives
>   three times as long (3*GMI instead of GMI), and the commit message…
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but tunnel
>   teardown does not wait for a group_timer callback that is already…
>
> --
>
> amt: fix use-after-free when a Membership Update re-arms a freed tunnel
>
> amt_update_handler() no longer re-arms tunnel->gc_wq. It now stamps a new
> tunnel->last_seen field, and amt_tunnel_expire() either re-arms itself or
> frees the tunnel under amt->lock. The patch also adds a tunnel->dead flag,
> and amt_dev_stop() now marks tunnels dead and splices the tunnel list out
> under amt->lock before freeing them.
>
> > diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> > index b53f8ec556615..b284fa7b7307e 100644
> > --- a/drivers/net/amt.c
> > +++ b/drivers/net/amt.c
>
> [ ... ]
>
> > @@ -2494,9 +2515,12 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
> >               if (tunnel->ip4 == saddr) {
> >                       if ((nonce == tunnel->nonce &&
> >                            response_mac == tunnel->mac)) {
> > -                             mod_delayed_work(amt_wq, &tunnel->gc_wq,
> > -                                              msecs_to_jiffies(amt_gmi(amt))
> > -                                                               * 3);
> > +                             /* Refresh liveness only.  gc_wq is re-armed by
> > +                              * amt_tunnel_expire() itself from last_seen, never
> > +                              * from here, so this cannot re-queue the work of a
> > +                              * tunnel that is being freed.
> > +                              */
> > +                             WRITE_ONCE(tunnel->last_seen, jiffies);
> >                               goto report;
>
> [Severity: High]
> The Update path no longer re-arms gc_wq. Can it still arm other delayed
> work that points at a tunnel being freed?
>
> amt_update_handler() still holds only rcu_read_lock(), and it never checks
> the new tunnel->dead flag. After the goto report it does this:
>
> drivers/net/amt.c:amt_update_handler() {
>     ...
>                 spin_lock_bh(&tunnel->lock);
>                 amt_igmp_report_handler(amt, skb, tunnel);
>                 spin_unlock_bh(&tunnel->lock);
>     ...
> }
>
> The MLD branch does the same.
>
> Suppose amt_tunnel_expire() reads last_seen before this WRITE_ONCE() is
> visible. It then goes ahead with the teardown:
>
> CPU1 (amt_tunnel_expire)            CPU2 (amt_update_handler)
>                                     finds tunnel in RCU walk
> deadline = READ_ONCE(last_seen)...
> tunnel->dead = true;
> list_del_rcu(&tunnel->list);
> amt_clear_groups(tunnel);
> kfree_rcu(tunnel, rcu);
>                                     WRITE_ONCE(tunnel->last_seen, jiffies);
>                                     spin_lock_bh(&tunnel->lock);
>                                     amt_igmp_report_handler()
>                                       amt_igmpv2_report_handler()
>                                         amt_add_group()
>                                           gnode->tunnel_list = tunnel;
>                                           hlist_add_head_rcu(...)
>                                         mod_delayed_work(group_timer)
>                                         dev_hold(amt->dev)
>
> Nothing deletes that gnode afterwards. Would the gnode, its sources and
> the netdev reference all leak?
>
> The tunnel is freed once the grace period ends. When group_timer fires,
> amt_group_work() loads gnode->tunnel_list and calls
> spin_lock_bh(&tunnel->lock) on the freed tunnel. amt_del_group() also
> writes gnode->tunnel_list->nr_groups.
>
> The commit message describes this same class of bug: an Update holding a
> cached tunnel pointer arms delayed work that outlives the tunnel.
>
> amt_dev_stop() seems to have the same window. An Update that found the
> tunnel before the list_splice_init() can add groups after
> amt_clear_groups() has run. This path looks unchanged through the end of
> the series.
>
> Could amt_update_handler() check tunnel->dead under tunnel->lock and bail
> out before calling the report handlers? Both freers set dead before
> amt_clear_groups() takes tunnel->lock, so that check should close the
> window.
>

I agree with Sashiko's suggestion.

Please make tunnel->dead protected by tunnel->lock as well, and
have amt_update_handler() check it under tunnel->lock before calling
the report handlers.

> [ ... ]
>
> > @@ -2732,6 +2756,7 @@ static bool amt_request_handler(struct amt_dev *amt, struct sk_buff *skb)
> >       for (i = 0; i < amt->hash_buckets; i++)
> >               INIT_HLIST_HEAD(&tunnel->groups[i]);
> >
> > +     tunnel->last_seen = jiffies;
> >       INIT_DELAYED_WORK(&tunnel->gc_wq, amt_tunnel_expire);
> >
> >       list_add_tail_rcu(&tunnel->list, &amt->tunnel_list);
>
> [Severity: Low]
> Does this extend the lifetime of a Request-only tunnel from GMI to 3*GMI?
>
> amt_request_handler() still arms the first expiry at one GMI:
>
>         mod_delayed_work(amt_wq, &tunnel->gc_wq,
>                          msecs_to_jiffies(amt_gmi(amt)));
>
> When that work fires, amt_tunnel_expire() computes:
>
>         deadline = READ_ONCE(tunnel->last_seen) +
>                    msecs_to_jiffies(amt_gmi(amt)) * 3;
>
> The deadline has not passed yet, so it re-arms for roughly another 2*GMI.
> Before this patch, a tunnel that only received a Request was freed after
> GMI. Unauthenticated Requests can now hold max_tunnels slots three times
> longer, and the commit message does not mention the change.
>
> The new comment in struct amt_tunnel_list also describes last_seen as the
> "Last time a valid Membership Update refreshed this tunnel". At this
> commit the Request path writes it as well.
>
> A later commit in the series, "amt: do not create tunnel state for
> unauthenticated Requests", seems to fix this. That commit stops the
> Request path from allocating tunnels, which also makes the struct comment
> accurate.
>
> [ ... ]
>
> > @@ -3053,6 +3078,7 @@ static int amt_dev_stop(struct net_device *dev)
> >       struct amt_tunnel_list *tunnel, *tmp;
> >       struct sk_buff *skb;
> >       struct sock *sk;
> > +     LIST_HEAD(doomed);
> >       int i;
> >
> >       disable_delayed_work_sync(&amt->req_wq);
> > @@ -3079,9 +3105,19 @@ static int amt_dev_stop(struct net_device *dev)
> >       amt->req_cnt = 0;
> >       WRITE_ONCE(amt->remote_ip, 0);
> >
> > -     list_for_each_entry_safe(tunnel, tmp, &amt->tunnel_list, list) {
> > -             list_del_rcu(&tunnel->list);
> > -             amt->nr_tunnels--;
>
> [ ... ]
>
> > +     spin_lock_bh(&amt->lock);
> > +     list_for_each_entry(tunnel, &amt->tunnel_list, list)
> > +             tunnel->dead = true;
> > +     list_splice_init(&amt->tunnel_list, &doomed);
> > +     amt->nr_tunnels = 0;
> > +     spin_unlock_bh(&amt->lock);
>
> [Severity: High]
> Is list_splice_init() safe here while RX readers may still be walking
> amt->tunnel_list with list_for_each_entry_rcu()?
>
> The old loop used list_del_rcu(). After this splice, the last tunnel's
> ->next points at the on-stack doomed head. amt->tunnel_list is
> reinitialised to empty, with no grace period in between.
>
> The only grace period in amt_dev_stop() is the synchronize_net() before
> udp_tunnel_sock_release(). udp_tunnel_sock_release() clears sk_user_data
> with rcu_assign_sk_user_data(sk, NULL), then shuts down and releases the
> socket without waiting for readers.
>
> So an amt_rcv() call that already loaded amt from sk_user_data could
> still be inside amt_request_handler() or amt_update_handler():

I think this is correct.
amt_dev_stop() needs to wait for in-flight amt_rcv() calls to finish
before tearing down the tunnel list.

>
>         list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list) {
>                 if (tunnel->ip4 == saddr) {
>
> A reader that has already passed the last tunnel would never reach the
> terminator, &amt->tunnel_list. Would it then treat &doomed as a struct
> amt_tunnel_list and read ip4 from stack memory past the list_head? It
> would then follow doomed.next back to the first tunnel and loop in softirq
> under rcu_read_lock().
>
> That would stall RCU, which also holds up the pending kfree_rcu() calls.
> Once amt_dev_stop() returns and its stack frame is reused, the reader
> follows garbage pointers. If the stack bytes happen to match saddr, the
> send: path in amt_request_handler() writes tunnel->nonce through the bogus
> pointer.
>
> There is a second problem. A reader that reaches the allocation in
> amt_request_handler() after the splice (amt_tunnel_get_or_create() later
> in the series) can list_add_tail_rcu() a new tunnel onto the emptied
> amt->tunnel_list. Nothing in this stop path frees that tunnel.
>
> Would list_splice_init_rcu(), which waits for a grace period, or keeping
> list_del_rcu() work better here? This code also looks unchanged at the end
> of the series.
>
> > +
> > +     list_for_each_entry_safe(tunnel, tmp, &doomed, list) {
> >               cancel_delayed_work_sync(&tunnel->gc_wq);
> >               amt_clear_groups(tunnel);
> >               kfree_rcu(tunnel, rcu);
>
> [Severity: High]
> This is a pre-existing issue, not one introduced by this patch. Does this
> teardown wait for a gnode->group_timer callback that is already running?
>
> amt_clear_groups() calls amt_del_group() for each group under
> tunnel->lock. amt_del_group() only does a non-synchronous cancel:
>
> drivers/net/amt.c:amt_del_group() {
>         if (cancel_delayed_work(&gnode->group_timer))
>                 dev_put(amt->dev);
>         hlist_del_rcu(&gnode->node);
>         gnode->tunnel_list->nr_groups--;
>         ...
>         kfree_rcu(gnode, rcu);
> }
>
> If amt_group_work() is already running, cancel_delayed_work() returns
> false. amt_group_work() loads the tunnel pointer outside any RCU read
> section:
>
>         struct amt_tunnel_list *tunnel = gnode->tunnel_list;
>         ...
>         spin_lock_bh(&tunnel->lock);
>
> If the callback is preempted before spin_lock_bh(), could the grace period
> finish so that it takes tunnel->lock inside a freed tunnel?
>
> If the memory has not been freed yet, the callback takes tunnel->lock once
> amt_clear_groups() drops it. For an EXCLUDE mode group the source lists
> are empty by then, so delete_group stays true and amt_del_group() runs a
> second time.
>
> Would that call hlist_del_rcu() again on a node whose pprev is already
> LIST_POISON2? It would also decrement nr_groups twice and call
> kfree_rcu(gnode) twice.
>
> amt_tunnel_expire() has the same issue, since it also calls
> amt_clear_groups() before kfree_rcu(tunnel). Only gc_wq is cancelled
> synchronously here; the per-group work is never waited for.
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012125943.558629-1-omar%40blockcast.net

Re: [PATCH net v3 0/6] amt: relay tunnel lifetime, validation and request-path hardening

[Omar Ramadan]

Thanks for the review. These comments are against the v3 series; v4 has
addressed the three issues introduced or exposed at those bisect points.

The Update path now takes tunnel->lock and checks READ_ONCE(tunnel->dead)
before either the IGMP or MLD report handler. Both tunnel freers set dead
before clearing groups, so an Update that acquires the lock after teardown
starts cannot add a group or arm its group_timer. An Update that acquires the
lock first is covered by the same lock ordering: teardown waits for the
handler before clearing the group list.

amt_dev_stop() no longer uses list_splice_init(). It marks all tunnels dead,
then removes one tunnel at a time with list_del_rcu() while holding amt->lock,
and performs cancel_delayed_work_sync(), group clearing, and kfree_rcu()
outside the lock. Every lockless reader therefore keeps the real list head as
its terminator, and the post-stop get_or_create() path also rejects creation
when !netif_running(amt->dev).

The Request-only lifetime issue is removed by v4's final patch. Requests are
stateless and do not allocate tunnel entries; tunnel state is created only
after a Membership Update authenticates. last_seen therefore describes a
valid Update again, and the global and per-source limits cannot be consumed
by unauthenticated Requests.

The pre-existing race involving a group_timer callback that is already
running while amt_clear_groups() calls cancel_delayed_work() remains outside
this series. It needs a separate lifetime fix for the callback and its gnode;
we will follow up on that independently rather than widen this stable fix
series.

pw-bot: cr