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

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

[Omar Ramadan]

Five 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/5 closes the gc_wq re-arm use-after-free and makes device-stop teardown
safe for lockless RCU readers. 2/5 rejects gateway Query/Data from 0.0.0.0
before a relay is learned. 3/5 requires exactly four bytes for the IPv4
input attributes, avoiding silent truncation of larger payloads. 4/5 makes
pre-Query report drops visible and stops counting forwarded reports as
drops. 5/5 keys relay tunnels on the (address, port) endpoint, authenticates
Updates before allocating tunnel state, and closes the report-group and
post-stop creation races found in review of v3.

Each patch carries a Fixes: tag. The exact tip builds warning-free with
GCC and Clang, W=1, with IPv6 enabled and disabled. A KASAN + lockdep +
PROVE_RCU kernel passes amt.sh 6/6 with no splats. The full-table control
(max_tunnels=1) gives the same result as v3.

v4:
- 1/5: unlink tunnels in place with list_del_rcu() and defer their free
  with kfree_rcu(); do not splice onto a stack head visible to RCU readers.
- 3/5: replace the IPv6-length special case with NLA_POLICY_EXACT_LEN for
  LOCAL_IP and DISCOVERY_IP. REMOTE_IP is output-only and is unchanged.
- 4/5: return directly after a successful Membership Update, use
  DEV_STATS_INC() for tx counters, and clarify the debug message.
- 5/5: fold v3's endpoint-keying patch into the authenticated-allocation
  patch, so no intermediate commit allocates unauthenticated port-keyed
  tunnels; retain four secret generations (180s floor > 125s QQIC);
  withhold Queries at both tunnel limits and silently drop refused
  Requests; re-check dead under the tunnel lock before report handling;
  reject creation after device stop.
- Rewrap the comments reported by NIPA at 80 columns.

v3: https://lore.kernel.org/netdev/20261012125943.558629-1-omar@blockcast.net/
v2: https://lore.kernel.org/netdev/20261009201455.1904698-1-omar@blockcast.net/
v1: https://lore.kernel.org/netdev/20261008003606.3666617-1-omar@blockcast.net/

Omar Ramadan (5):
  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: require an exact IPv4-sized IFLA_AMT_{LOCAL,DISCOVERY}_IP
  amt: make pre-query report drops visible
  amt: harden relay tunnel lifetime and request admission

 drivers/net/amt.c | 389 +++++++++++++++++++++++++++++++++++-----------
 include/net/amt.h |  27 +++-
 2 files changed, 317 insertions(+), 99 deletions(-)

[PATCH net v4 1/5] 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, and it walked tunnel_list without
amt->lock while a running gc_wq could free the next entry under it.  Mark
every tunnel dead under amt->lock first, so a gc_wq that is already
running sees ->dead and bails instead of freeing in parallel; then unlink
the tunnels one at a time with list_del_rcu() under amt->lock, and cancel,
clear and free each one outside it.  The tunnel is freed exactly once, and
because readers walk tunnel_list locklessly under RCU, the entries are
unlinked in place rather than spliced onto a local list head.

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

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index b53f8ec..530650d 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -1328,8 +1328,30 @@ 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 +2516,13 @@ 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 +2758,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);
@@ -3050,7 +3077,7 @@ static int amt_dev_open(struct net_device *dev)
 static int amt_dev_stop(struct net_device *dev)
 {
 	struct amt_dev *amt = netdev_priv(dev);
-	struct amt_tunnel_list *tunnel, *tmp;
+	struct amt_tunnel_list *tunnel;
 	struct sk_buff *skb;
 	struct sock *sk;
 	int i;
@@ -3079,9 +3106,31 @@ 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 first, so a concurrent amt_tunnel_expire()
+	 * sees ->dead and bails instead of freeing a tunnel in parallel; this
+	 * path is then the sole freer.
+	 */
+	spin_lock_bh(&amt->lock);
+	list_for_each_entry(tunnel, &amt->tunnel_list, list)
+		tunnel->dead = true;
+	spin_unlock_bh(&amt->lock);
+
+	/* Unlink one tunnel at a time with list_del_rcu() rather than splicing
+	 * the list onto a local head: a lockless reader walking tunnel_list
+	 * must always terminate at &amt->tunnel_list, never at a stack head.
+	 */
+	for (;;) {
+		spin_lock_bh(&amt->lock);
+		tunnel = list_first_entry_or_null(&amt->tunnel_list,
+						  struct amt_tunnel_list, list);
+		if (tunnel) {
+			list_del_rcu(&tunnel->list);
+			amt->nr_tunnels--;
+		}
+		spin_unlock_bh(&amt->lock);
+		if (!tunnel)
+			break;
+
 		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;

Re: [PATCH net v4 1/5] 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 4 potential
issue(s) to consider.

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

- [Critical] At this commit the Update-vs-teardown race is only partly
  closed.
- [Medium] At this commit the initial gc_wq arm and the new expiry
  deadline disagree.
- [Medium] At this commit amt_dev_stop() sets ->dead on all tunnels in
  one amt->lock section and unlinks them later in separate amt->lock…
- [Low] The new comments disagree about which code frees a tunnel.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but tearing down a
  group does not wait for a group_timer callback that is already running.

--

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

This patch stops amt_update_handler() from re-arming a relay tunnel's
gc_wq. The Update path now only stamps tunnel->last_seen, and
amt_tunnel_expire() uses last_seen under amt->lock to decide whether to
re-arm itself or free the tunnel. It also adds a dead flag and changes
amt_dev_stop() to mark every tunnel dead under amt->lock, then unlink and
free them one at a time.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index b53f8ec556615..530650d5c19e9 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -1328,8 +1328,30 @@ static void amt_tunnel_expire(struct work_struct *work)

[ ... ]

>  	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.
> +	 */

[Severity: Low]
This isn't a bug, but the comments in this patch don't agree on which
code frees a tunnel. This one says amt_tunnel_expire() is "the only
context that frees the tunnel". The new comment in amt_dev_stop() says
"this path is then the sole freer", and amt_dev_stop() does call
kfree_rcu() on every tunnel left on the list.

The dead field comment in include/net/amt.h says either function may free
the tunnel, whichever gets there first. The commit message says "the only
context that frees a tunnel on the timeout path".

Could this comment keep the "on the timeout path" qualifier? Both comments
are still unchanged at the end of the series.

[ ... ]

> @@ -2494,9 +2516,13 @@ 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: Critical]
Can this cached tunnel pointer still be used after teardown? The tunnel
comes from a lockless list_for_each_entry_rcu() walk. After goto report,
amt_update_handler() does this without checking tunnel->dead:

    spin_lock_bh(&tunnel->lock);
    amt_igmp_report_handler(amt, skb, tunnel);
    spin_unlock_bh(&tunnel->lock);

If amt_tunnel_expire() or amt_dev_stop() runs at the same time:

CPU1 (amt_tunnel_expire)
  tunnel->dead = true;
  list_del_rcu(&tunnel->list);
  amt_clear_groups(tunnel);   /* takes and drops tunnel->lock */
  kfree_rcu(tunnel, rcu);

CPU2 (amt_update_handler)
  spin_lock_bh(&tunnel->lock);
  amt_igmp_report_handler()
    amt_igmpv2_report_handler()
      amt_add_group()
        gnode->tunnel_list = tunnel;
        hlist_add_head_rcu(&gnode->node, &tunnel->groups[hash]);

Nothing ever clears the new gnode, and its group_timer is armed with
dev_hold(). After the grace period the tunnel is freed, and
amt_group_work() later does this on freed memory:

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

The gnode and the netdev reference also leak. This is the same
cached-pointer pattern the commit message describes. The MLD path
(amt_mld_report_handler()) has the same gap.

The later patch "amt: harden relay tunnel lifetime and request admission"
adds a READ_ONCE(tunnel->dead) check under tunnel->lock before both report
handlers. That closes this window, but the check is not present at this
commit.

Even with that check, an Update that stamps last_seen just after
amt_tunnel_expire() reads it at the 3 * GMI boundary loses that one
refresh. This only drops the report and does not cause a memory-safety
problem.

[ ... ]

> @@ -2732,6 +2758,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: Medium]
amt_request_handler() still arms gc_wq for a single GMI a few lines below:

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

amt_tunnel_expire() now uses a 3 * GMI deadline:

    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);

When the work first fires at 1 * GMI, time_before() is true, so the work
re-arms for about 2 * GMI more instead of freeing the tunnel.

Does a tunnel created by a Request, and never confirmed by a MAC-valid
Membership Update, now live for 3 * GMI instead of 1 * GMI? That would
hold its max_tunnels slot three times longer than before.

Also, the new last_seen comment in include/net/amt.h says "Last time a
valid Membership Update refreshed this tunnel". At this commit the Request
path sets it without any Update.

The later patch "amt: harden relay tunnel lifetime and request admission"
makes amt_request_handler() stateless. It creates tunnels only in
amt_tunnel_get_or_create(), which arms gc_wq for 3 * GMI, so the arm time
and the deadline match by the end of the series. They don't match at this
commit.

[ ... ]

> @@ -3079,9 +3106,31 @@ 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 first, so a concurrent amt_tunnel_expire()
> +	 * sees ->dead and bails instead of freeing a tunnel in parallel; this
> +	 * path is then the sole freer.
> +	 */
> +	spin_lock_bh(&amt->lock);
> +	list_for_each_entry(tunnel, &amt->tunnel_list, list)
> +		tunnel->dead = true;
> +	spin_unlock_bh(&amt->lock);

[Severity: Medium]
Can a new tunnel be added between this marking pass and the unlink loop
below? The unlink loop takes amt->lock again for each entry and never sets
->dead itself.

The RX path doesn't seem to be quiesced at this point. Earlier in
amt_dev_stop():

    RCU_INIT_POINTER(amt->sk, NULL);
    synchronize_net();
    if (sk)
        udp_tunnel_sock_release(sk);

amt_rcv() finds the device through rcu_dereference_sk_user_data(sk).
udp_tunnel_sock_release() clears sk_user_data after synchronize_net() and
does not wait for another grace period. amt_request_handler() also has no
netif_running() check before list_add_tail_rcu() and arming gc_wq.

So a Request still in flight could publish a tunnel with dead == false and
gc_wq armed after this pass. That tunnel could stay on tunnel_list after
stop.

If its gc_wq instead runs between the list_del_rcu() below and
cancel_delayed_work_sync(), amt_tunnel_expire() sees dead == false. Both
contexts would then unlink and kfree_rcu() the same tunnel. That needs
gc_wq, armed for at least one GMI, to fire inside a very short window, so
it is unlikely.

The later patch "amt: harden relay tunnel lifetime and request admission"
moves tunnel creation into amt_tunnel_get_or_create(). That function checks
!netif_running(amt->dev) under amt->lock, which closes this by the end of
the series.

[ ... ]

> +	for (;;) {
> +		spin_lock_bh(&amt->lock);
> +		tunnel = list_first_entry_or_null(&amt->tunnel_list,
> +						  struct amt_tunnel_list, list);
> +		if (tunnel) {
> +			list_del_rcu(&tunnel->list);
> +			amt->nr_tunnels--;
> +		}
> +		spin_unlock_bh(&amt->lock);
> +		if (!tunnel)
> +			break;
> +
>  		cancel_delayed_work_sync(&tunnel->gc_wq);
>  		amt_clear_groups(tunnel);
>  		kfree_rcu(tunnel, rcu);

[Severity: High]
This isn't a bug introduced by this patch, but does group teardown wait
for a group_timer callback that is already running? amt_del_group() does:

    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);

cancel_delayed_work() returns false once amt_group_work() has started.
amt_group_work() reads gnode->tunnel_list and amt->hash_buckets before it
takes tunnel->lock. It never checks whether the gnode was already deleted.

Take an IGMPv2 group (MCAST_EXCLUDE, group_timer armed at GMI). If a
remote IGMPv2 Leave, or amt_clear_groups(), gets tunnel->lock while that
timer is running:

CPU1
amt_igmpv2_leave_handler()
  amt_del_group(amt, gnode)
    hlist_del_rcu(&gnode->node);
    kfree_rcu(gnode, rcu);

CPU2
amt_group_work()
  spin_lock_bh(&tunnel->lock);
  /* still MCAST_EXCLUDE, no sources, delete_group = true */
  amt_del_group(amt, gnode)
    hlist_del_rcu(&gnode->node);   /* pprev is LIST_POISON2 */

Would the second hlist_del_rcu() oops here? This path also decrements
nr_groups a second time and calls kfree_rcu() a second time.

On preemptible kernels, amt_group_work() can also be preempted before
spin_lock_bh(). The gnode's kfree_rcu() does not wait for it. Neither does
the tunnel's kfree_rcu() here, which follows amt_clear_groups() in
amt_dev_stop() or amt_tunnel_expire(). On resume, the work could touch
freed gnode and tunnel memory, including spin_lock_bh() on a freed
tunnel->lock. The dev_hold() taken when group_timer is armed only keeps
the netdev alive.

amt_dev_stop() only calls cancel_delayed_work_sync() on gc_wq, not on the
group or source timers. This code is unchanged through the end of the
series.

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

[PATCH net v4 2/5] 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 530650d..e9b1a45 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -2860,7 +2860,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;
@@ -2871,7 +2871,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;

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

[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 is incomplete.

--

amt: reject a gateway Query or Data from 0.0.0.0 before a relay is learned

This patch makes a gateway's amt_rcv() drop Membership Query and Multicast
Data messages while remote_ip is 0. Before, a packet from 0.0.0.0 matched
the unset remote_ip and was accepted. The commit message says that, with
the nonce reset to 0, such a spoofed Query would be taken as genuine.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 530650d5c19e9..e9b1a4585ce51 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c

[ ... ]

> @@ -2871,7 +2871,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;

[Severity: High]
Is this check early enough to give the result the commit message describes?
It runs in softirq, against a copy of remote_ip read at the top of
amt_rcv():

    remote_ip = READ_ONCE(amt->remote_ip);

The Query is then queued as AMT_EVENT_RECEIVE. It is handled later, in
amt_event_work()->amt_gw_rcv()->amt_membership_query_handler(). That
handler checks only the nonce. It does not look at remote_ip or
amt->status again:

    if (amtmq->nonce != amt->nonce)
        return true;

AMT_EVENT_SEND_REQUEST goes through the same FIFO. When discovery gives up,
amt_event_send_request() does this in the worker:

    if (amt->req_cnt > AMT_MAX_REQ_COUNT) {
        ...
        WRITE_ONCE(amt->remote_ip, 0);
        amt_update_gw_status(amt, AMT_STATUS_INIT, false);
        amt->req_cnt = 0;
        amt->nonce = 0;
        goto out;
    }

Consider a Query spoofed from the old relay address with nonce 0. It passes
the new check if it arrives while remote_ip still holds that address. This
can happen in two ways:

- it is queued behind a pending SEND_REQUEST event, or
- amt_rcv() read remote_ip just before the WRITE_ONCE().

If so, the worker would handle it after the reset:

amt_event_work()
  amt_event_send_request()       /* remote_ip = 0, nonce = 0, INIT */
  amt_gw_rcv()
    amt_membership_query_handler()
      amtmq->nonce == amt->nonce  /* 0 == 0 */
      WRITE_ONCE(amt->ready4, true);
      amt->mac = response_mac;
      amt->req_cnt = 0;
      amt->qi = ihv3->qqic;

The status then moves to AMT_STATUS_RECEIVED_QUERY. That makes
amt_event_send_discovery() skip discovery.

These are the effects the commit message says the patch prevents ("a
spoofed Query is then taken as genuine: it sets ready4/qi and the response
MAC and suppresses rediscovery"). Here they still happen with no relay
learned.

The give-up time is predictable. After an advertisement, unanswered
requests back off 2s/4s/8s/16s, so req_cnt passes AMT_MAX_REQ_COUNT about
30s later. Any host that can spoof the relay's source address could flood
Queries around that moment.

The later patches in this series do not appear to change
amt_membership_query_handler() or this event ordering.

Would it be better to check where the state is consumed? For example,
amt_membership_query_handler() could reject the Query unless amt->status is
AMT_STATUS_SENT_REQUEST or AMT_STATUS_RECEIVED_QUERY. Another option is to
require a non-zero remote_ip that matches ip_hdr(skb)->saddr, and possibly
to refuse the predictable nonce 0.

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

[PATCH net v4 3/5] amt: require an exact IPv4-sized IFLA_AMT_{LOCAL,DISCOVERY}_IP

[Omar Ramadan]

The netlink policy for IFLA_AMT_LOCAL_IP and IFLA_AMT_DISCOVERY_IP is an
untyped entry with .len set to the size of an IPv4 address, which netlink
validation treats as a minimum length only.  Any longer payload passes,
and nla_get_in_addr() then reads just its first four bytes: an iproute2
that puts a 16-byte IPv6 literal in one of these IPv4 attributes turns
"2001:db8::" into 32.1.13.184, and the device is created with an address
the user never asked for.

Use NLA_POLICY_EXACT_LEN(sizeof(__be32)) for both, so every payload that
is not exactly four bytes long is refused with a standard extack instead
of being truncated.  The range check this expands to fails the attribute
even under the liberal parsing rtnetlink applies to IFLA_INFO_DATA.
amt_get_size() and amt_fill_info() already emit exactly four bytes for
these attributes.  IFLA_AMT_REMOTE_IP is only ever reported, never read
from a request, so it is left unchanged.

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 e9b1a45..e466cf8 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -3201,9 +3201,9 @@ static const struct nla_policy amt_policy[IFLA_AMT_MAX + 1] = {
 	[IFLA_AMT_RELAY_PORT]	= { .type = NLA_U16 },
 	[IFLA_AMT_GATEWAY_PORT]	= { .type = NLA_U16 },
 	[IFLA_AMT_LINK]		= { .type = NLA_U32 },
-	[IFLA_AMT_LOCAL_IP]	= { .len = sizeof_field(struct iphdr, daddr) },
+	[IFLA_AMT_LOCAL_IP]	= NLA_POLICY_EXACT_LEN(sizeof(__be32)),
 	[IFLA_AMT_REMOTE_IP]	= { .len = sizeof_field(struct iphdr, daddr) },
-	[IFLA_AMT_DISCOVERY_IP]	= { .len = sizeof_field(struct iphdr, daddr) },
+	[IFLA_AMT_DISCOVERY_IP]	= NLA_POLICY_EXACT_LEN(sizeof(__be32)),
 	[IFLA_AMT_MAX_TUNNELS]	= { .type = NLA_U32 },
 };
 

[PATCH net v4 4/5] 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 drop path.

tx_dropped could not show these drops either: amt_dev_xmit() reaches the
same tx_dropped increment after amt_send_membership_update() has handed
the report to the tunnel, so in gateway mode every forwarded report was
also counted as a drop.  Return NETDEV_TX_OK directly once the report has
been sent, so tx_dropped counts only reports that were discarded.

The device is LLTX, so amt_dev_xmit() can run on several CPUs at once;
use DEV_STATS_INC() for its tx_dropped and tx_errors updates so
concurrent increments are not lost.

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

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index e466cf8..884f27f 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -1236,7 +1236,7 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 		group.ip6 = ipv6_hdr(skb)->daddr;
 #endif
 	} else {
-		dev->stats.tx_errors++;
+		DEV_STATS_INC(dev, tx_errors);
 		goto free;
 	}
 
@@ -1249,12 +1249,21 @@ 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: waiting for Membership Query\n",
+				   v6 ? "MLD" : "IGMP");
 			goto free;
+		}
 		if (amt_send_membership_update(amt, skb,  v6))
 			goto free;
-		goto unlock;
+		/* Sent: the tunnel consumed the skb.  Not a drop. */
+		return NETDEV_TX_OK;
 	} else if (amt->mode == AMT_MODE_RELAY) {
 		if (!data)
 			goto free;
@@ -1283,8 +1292,7 @@ found:
 	return NETDEV_TX_OK;
 free:
 	dev_kfree_skb(skb);
-unlock:
-	dev->stats.tx_dropped++;
+	DEV_STATS_INC(dev, tx_dropped);
 	return NETDEV_TX_OK;
 }
 

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

[netdev-bot+sashiko]

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

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

- [Medium] The commit message says amt_dev_xmit() now uses
  DEV_STATS_INC() for tx_dropped and tx_errors "so concurrent increments…

--

amt: make pre-query report drops visible

When a gateway drops an IGMP or MLD report because the relay's Membership
Query has not arrived yet, this patch now logs a netdev_dbg() message. It
also stops counting successfully forwarded gateway reports as tx_dropped.
The tx_dropped and tx_errors updates in amt_dev_xmit() now use
DEV_STATS_INC().

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index e466cf815a358..884f27fd84a4a 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -1236,7 +1236,7 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>  		group.ip6 = ipv6_hdr(skb)->daddr;
>  #endif
>  	} else {
> -		dev->stats.tx_errors++;
> +		DEV_STATS_INC(dev, tx_errors);
>  		goto free;
>  	}
>  

[ ... ]

> @@ -1283,8 +1292,7 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>  	return NETDEV_TX_OK;
>  free:
>  	dev_kfree_skb(skb);
> -unlock:
> -	dev->stats.tx_dropped++;
> +	DEV_STATS_INC(dev, tx_dropped);
>  	return NETDEV_TX_OK;
>  }

[Severity: Medium]
The commit message says DEV_STATS_INC() is used "so concurrent increments
are not lost". Is that still true while other code in amt.c updates the
same counters with a plain increment?

DEV_STATS_INC() expands to atomic_long_inc(&dev->stats.__tx_dropped).
NET_DEV_STAT() makes __tx_dropped a union alias of the same unsigned long
that stats.tx_dropped++ modifies. If one site uses the atomic increment and
another does a plain read-modify-write on that word, updates can still be
lost.

For tx_dropped, amt_send_igmp_gq() and amt_send_mld_gq() still use the
plain form:

drivers/net/amt.c:amt_send_igmp_gq() {
    ...
	if (amt_send_membership_query(amt, skb, tunnel, false)) {
		amt->dev->stats.tx_dropped++;
		kfree_skb(skb);
	}
}

In relay mode these functions run from RX softirq via
amt_rcv()->amt_request_handler(). A remote gateway's AMT Request is enough
to trigger them. Nothing serializes them with amt_dev_xmit(), which is
LLTX:

CPU1 amt_send_igmp_gq()          CPU0 amt_dev_xmit()
  load tx_dropped (N)
                                   atomic_long_inc() -> N+1
  store N+1

The counter ends up at N+1 instead of N+2.

For tx_errors, plain increments remain in amt_send_discovery() and
amt_send_request() (gateway event work). They also remain in
amt_build_mld_gq() and amt_send_advertisement() (relay RX softirq). For
example:

drivers/net/amt.c:amt_send_request() {
    ...
	if (IS_ERR(rt)) {
		amt->dev->stats.tx_errors++;
		goto out;
	}
    ...
}

Should every tx_dropped and tx_errors writer in amt.c be converted to
DEV_STATS_INC(), so that the statement in the commit message is accurate?
No later patch in the series seems to make this change.

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

[PATCH net v4 5/5] amt: harden relay tunnel lifetime and request admission

[Omar Ramadan]

Relay Membership Requests are unauthenticated UDP packets. Creating tunnel
state from a Request lets spoofed sources consume relay slots and lets a
spoofed Request overwrite the nonce and MAC of an established endpoint.

Keep Requests stateless: derive the Query MAC from the source address,
source port, nonce, and current key, then authenticate the Membership Update
before decapsulating it or creating tunnel state. Key tunnel state by the
(address, port) endpoint. Initialise every key before enabling receive.

Keep four generations of the relay key. The current key and three previous
keys provide a 180-240 second validation window, whose lower bound exceeds
the advertised QQIC request interval, so a gateway can renew its MAC across
key rotations.

When a new endpoint would exceed either the global tunnel limit or the
per-source limit, withhold the Membership Query and silently drop the
Request. This avoids reflecting ICMP to an unauthenticated source and lets
the gateway rediscover. Requests from an existing endpoint still receive a
Query and can renew their MAC.

A Membership Update may find a tunnel under RCU after expiry has marked it
dead. Recheck tunnel->dead while holding tunnel->lock before adding report
groups, so no group timer is armed after teardown has cleared the groups.
Check netif_running(amt->dev) under amt->lock before allocating a tunnel,
so an Update racing device stop cannot publish state that stop will miss.

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

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index 884f27f..0124f1b 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,14 @@ 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));
+	/* Keep three previous generations for a 180-240s MAC lifetime.
+	 * Its 180s floor exceeds the advertised QQIC (amt->qi, 125s),
+	 * allowing the gateway to renew its MAC with a periodic Request.
+	 */
+	amt->key[3] = amt->key[2];
+	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 +1131,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 +1161,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 +1177,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 +1191,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;
 }
 
@@ -2491,16 +2513,107 @@ 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;
+	}
+
+	if (!netif_running(amt->dev)) {
+		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[4];
+	int len, hdr_size, i;
 	u64 response_mac;
+	bool verified = false;
 	__be32 saddr;
 	__be32 nonce;
+	__be16 sport;
 
 	saddr = ip_hdr(skb)->saddr;
 
@@ -2514,34 +2627,58 @@ 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;
+	/* Recompute the MAC handed out in the Membership Query.  Four live
+	 * secrets cover 180-240s, exceeding the advertised QQIC (amt->qi,
+	 * 125s), so a periodic Request can renew the MAC before it expires.
+	 * Authenticate before decapsulation or allocating tunnel state.
+	 */
+	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) {
-			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;
 
@@ -2556,6 +2693,10 @@ report:
 		}
 
 		spin_lock_bh(&tunnel->lock);
+		if (READ_ONCE(tunnel->dead)) {
+			spin_unlock_bh(&tunnel->lock);
+			return true;
+		}
 		amt_igmp_report_handler(amt, skb, tunnel);
 		spin_unlock_bh(&tunnel->lock);
 
@@ -2576,6 +2717,10 @@ report:
 		}
 
 		spin_lock_bh(&tunnel->lock);
+		if (READ_ONCE(tunnel->dead)) {
+			spin_unlock_bh(&tunnel->lock);
+			return true;
+		}
 		amt_mld_report_handler(amt, skb, tunnel);
 		spin_unlock_bh(&tunnel->lock);
 
@@ -2718,15 +2863,29 @@ 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, have = false;
+	int per_src = 0;
 	u64 mac;
-	int i;
 
 	if (!pskb_may_pull(skb, sizeof(*udph) + sizeof(*amtrh)))
 		return true;
@@ -2738,60 +2897,40 @@ 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)
-			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);
+	if (!netif_running(amt->dev) || !netif_running(amt->stream_dev))
 		return true;
-	}
 
-	tunnel = kzalloc(sizeof(*tunnel) +
-			 (sizeof(struct hlist_head) * amt->hash_buckets),
-			 GFP_ATOMIC);
-	if (!tunnel) {
-		spin_unlock_bh(&amt->lock);
-		return true;
+	/* Snapshot the secret and tunnel limits on the same locked walk. */
+	spin_lock_bh(&amt->lock);
+	key = amt->key[0];
+	full = amt->nr_tunnels >= amt->max_tunnels;
+	list_for_each_entry(tunnel, &amt->tunnel_list, list) {
+		if (tunnel->ip4 != iph->saddr)
+			continue;
+		per_src++;
+		if (tunnel->source_port == udph->source)
+			have = 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)));
 	spin_unlock_bh(&amt->lock);
 
-send:
-	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))
+	/* Withhold the Query when a new endpoint cannot get a tunnel, so the
+	 * gateway rediscovers.  Existing endpoints must still renew their MAC.
+	 * Drop silently: an unauthenticated Request must not reflect ICMP.
+	 */
+	if (!have && (full || per_src >= AMT_MAX_TUNNELS_PER_SRC))
 		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;
 }
@@ -3059,6 +3198,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);
@@ -3069,7 +3216,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..63bb647 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,10 @@ struct amt_dev {
 	struct work_struct	event_wq;
 	/* AMT status */
 	enum amt_status		status;
-	/* Generated key */
-	siphash_key_t		key;
+	/* Current key and three previous generations keep MACs valid for
+	 * 180-240s, beyond the advertised QQIC (see amt_secret_work()).
+	 */
+	siphash_key_t		key[4];
 	struct sock	  __rcu *sk;
 	u32			max_groups;
 	u32			max_sources;
@@ -394,6 +392,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) + \

Re: [PATCH net v4 5/5] amt: harden relay tunnel lifetime and request admission

[netdev-bot+sashiko]

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

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

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] Commit "amt: make pre-query report drops visible" (in this
  baseline-to-target range) converted only amt_dev_xmit()'s counter…
- [Low] Two new comments about the per-source tunnel cap describe the
  code wrongly.
- [Low] The selftest comment above test_query_egress()
  (tools/testing/selftests/net/amt.sh:256-259) is now stale.
- [Low] amt_update_handler() now reads tunnel->dead with READ_ONCE()
  while holding tunnel->lock (drivers/net/amt.c:2696 and 2720).

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the
  group/source workers can still race with group deletion and cause a…

--

amt: harden relay tunnel lifetime and request admission

This patch makes AMT relay Membership Requests stateless. The Query MAC is
derived from the source address, port, nonce and a rotating key, and tunnel
state is only created once a Membership Update has been authenticated.
Tunnels are keyed by (address, port), four key generations are kept, new
endpoints over the global or per-source limits are dropped silently, and
tunnel->dead is rechecked under tunnel->lock in the Update path.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 884f27fd84a4a..0124f1be673b3 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c

[ ... ]

> @@ -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);
>  	}

[Severity: Medium]
The previous patch in this series, "amt: make pre-query report drops
visible", changed amt_dev_xmit() to use DEV_STATS_INC() for tx_dropped and
tx_errors "so concurrent increments are not lost". Can concurrent
increments still be lost on this counter?

amt_send_igmp_gq() and amt_send_mld_gq() still do a plain read-modify-write
on the same field:

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

In relay mode, amt_request_handler() runs from UDP softirq on one CPU, and
amt_dev_xmit() can take its free path on another CPU at the same time:

    free:
        dev_kfree_skb(skb);
        DEV_STATS_INC(dev, tx_dropped);

Nothing serialises these two paths. The plain store can therefore
overwrite the atomic increment.

The tx_errors++ sites in amt_send_discovery(), amt_send_request(),
amt_send_advertisement() and similar helpers mix the two kinds of update
in the same way.

Should these sites use DEV_STATS_INC() as well?

[ ... ]

> @@ -1115,9 +1131,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)

[Severity: Low]
This isn't a bug, but the comment above test_query_egress() in
tools/testing/selftests/net/amt.sh still describes the old design:

    # The relay sends its General Queries straight from the receive path, in
    # the same context that found the tunnel. A query queued on the amt device
    # instead could outlive the tunnel it was built for.

After this patch, amt_request_handler() replies to a Request without
looking up or creating a tunnel. The Query helpers now take daddr, dport,
nonce and mac by value.

Should the selftest comment be updated to match? The egress check itself
still looks useful.

[ ... ]

> @@ -2556,6 +2693,10 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
>  		}
>  
>  		spin_lock_bh(&tunnel->lock);
> +		if (READ_ONCE(tunnel->dead)) {
> +			spin_unlock_bh(&tunnel->lock);
> +			return true;
> +		}
>  		amt_igmp_report_handler(amt, skb, tunnel);
>  		spin_unlock_bh(&tunnel->lock);

[Severity: Low]
This check reads tunnel->dead with READ_ONCE() while holding tunnel->lock.
Both writers use a plain store and hold only amt->lock:

amt_tunnel_expire():
    tunnel->dead = true;
    rcu_read_lock();
    list_del_rcu(&tunnel->list);

amt_dev_stop():
    list_for_each_entry(tunnel, &amt->tunnel_list, list)
        tunnel->dead = true;

In amt_tunnel_expire(), the store happens before amt_clear_groups() takes
tunnel->lock, so it can run concurrently with this load.

The logic itself still holds, because of the tunnel->lock release/acquire
in amt_clear_groups(). Would KCSAN still report this as a data race?
Should the two writers use WRITE_ONCE()?

> @@ -2576,6 +2717,10 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
>  		}
>  
>  		spin_lock_bh(&tunnel->lock);
> +		if (READ_ONCE(tunnel->dead)) {
> +			spin_unlock_bh(&tunnel->lock);
> +			return true;
> +		}
>  		amt_mld_report_handler(amt, skb, tunnel);
>  		spin_unlock_bh(&tunnel->lock);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. The new
dead checks only cover the Update path. Can the group and source timer
workers still race with group deletion?

amt_group_work() reads its owners and takes the tunnel lock before it has
any lifetime protection:

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

amt_source_work() follows the same pattern.

amt_del_group() does not wait for a worker that is already running:

    if (cancel_delayed_work(&gnode->group_timer))
        dev_put(amt->dev);
    hlist_del_rcu(&gnode->node);
    ...
    kfree_rcu(gnode, rcu);

amt_del_group() is called from several places:

  - amt_clear_groups(), from amt_tunnel_expire() and from amt_dev_stop().
    amt_dev_stop() only sync-cancels tunnel->gc_wq.
  - Leave handlers such as amt_igmpv2_leave_handler().

amt_wq is WQ_UNBOUND.

Two outcomes look possible.

On preemptible kernels, the worker can be preempted before spin_lock_bh().
It is not an RCU reader at that point. After the grace period it can
resume and access a freed gnode or tunnel.

On any kernel, the worker can spin on tunnel->lock while the deleter holds
it. Once the lock is released, the worker sees MCAST_EXCLUDE with empty
source buckets and calls amt_del_group(amt, gnode) a second time.
hlist_del_rcu() then writes through LIST_POISON2, and kfree_rcu() runs
twice on the same gnode.

[ ... ]

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

[ ... ]

> @@ -394,6 +392,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]
Is this comment accurate? What does "the configurable net-next per-source
bound" refer to?

The IFLA_AMT_* attributes in include/uapi/linux/amt.h include no
per-source limit. amt_newlink() applies IFLA_AMT_MAX_TUNNELS only to
amt->max_tunnels. Both amt_tunnel_get_or_create() and amt_request_handler()
compare against the fixed AMT_MAX_TUNNELS_PER_SRC.

The "one tunnel per family" rationale also appears in the
amt_tunnel_get_or_create() comment. It doesn't seem to match the data
model:

  - struct amt_tunnel_list has no family field.
  - Tunnels are keyed only by (ip4, source_port).
  - amt_update_handler() feeds both IGMP and MLD reports into the same
    tunnel.

So a single gateway endpoint uses one tunnel, and the comment doesn't
explain why the value is 16.

Could both comments describe the actual limit and the reason for 16?

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

Re: [PATCH net v4 0/5] 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.