[PATCH net v2 0/3] amt: fix relay tunnel keying and unauthenticated-Request DoS

[PATCH net v2 0/3] amt: fix relay tunnel keying and unauthenticated-Request DoS

[Omar Ramadan]

This series fixes related problems in the AMT relay in
drivers/net/amt.c. The relay creates and mutates per-tunnel state in
response to AMT Request messages whose source is never validated, so a
spoofed-source flood exhausts the tunnel table and reflects Membership
Queries at arbitrary addresses. The same code keys tunnels on the source
address alone, so two gateways behind one NAT collide.

Reported privately to security@kernel.org first. The security team
determined there is no memory-safety exposure and no embargo is needed,
and asked that the fix be posted here in the open with Taehee Yoo in Cc.

Patches 1 and 3 carry Fixes: cbc21dc1cfe9. Patch 3 is the core fix: the
relay now answers a Request statelessly (it computes the response MAC
and emits the Query without allocating a tunnel) and only commits tunnel
state once the gateway echoes the nonce+MAC in an Update. A spoofed
source cannot complete that exchange, so it allocates nothing.

Testing: booted net at commit 6d25ffca055a ("cipso: adjust cached
option offsets when removing CIPSO") plus this series (arm64, QEMU via
virtme-ng) with CONFIG_KASAN=y, CONFIG_PROVE_LOCKING=y,
CONFIG_PROVE_RCU=y and CONFIG_DEBUG_LIST=y on top of
tools/testing/selftests/net/config. amt.sh passes all six tests,
including both forwarding-torture cases, with no KASAN, lockdep or RCU
reports, and debug_locks stays 1. Every case completes a real gateway
handshake, so this exercises the stateless Request path and the MAC
check on Update. drivers/net/amt.c and include/net/amt.h are unchanged
between that commit and the base-commit below.

A companion change bounds the number of verified tunnels admitted per
source address. It adds a new netlink attribute and so targets net-next
as a separate posting, not part of this series. One note on its default:
a per-source cap closes the non-spoofing exhaustion path (one host, many
real handshakes) that this series does not, but a low fixed default is
wrong behind carrier-grade NAT, where many independent subscribers share
one public address and would be refused past the cap. The net-next
posting sets the default accordingly and documents the CGNAT case; this
series does not depend on that cap and closes the spoofing primitive on
its own.

Changes in v2:
- Drop v1 patch 2/4 ("amt: send the relay General Query directly
  instead of via dev_queue_xmit"). The same fix is already in net as
  commit afae89de73dd ("amt: send the relay's General Query directly
  from the receive path"). v1 was generated against v7.1 and did not
  apply to net.
- Rebase onto net. In patch 1, the port check uses the header fields
  that amt_update_handler() now snapshots before the pull. In patch 3,
  the Query senders keep that commit's tx_dropped accounting and take
  the destination by value.
- Redo the testing on net. The forwarding-torture subtests now run to
  completion.
v1: https://lore.kernel.org/netdev/20261008003606.3666617-1-omar@blockcast.net/

Omar Ramadan (3):
  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

 drivers/net/amt.c | 266 +++++++++++++++++++++++++++++++---------------
 include/net/amt.h |  11 +-
 2 files changed, 185 insertions(+), 92 deletions(-)


base-commit: 37f12441f557468a56c1e27790413aa78c82afa2
-- 
2.47.3

[PATCH net v2 1/3] 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.

Also count the "Invalid MAC" drop in rx_dropped. It is currently a
netdev_dbg only, and an Update dropped for failing validation is the one
delivery failure a gateway cannot observe from its own side.

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")
Signed-off-by: Omar Ramadan <omar@blockcast.net>
---
 drivers/net/amt.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index b53f8ec5566..ed82f8fac3f 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -2471,6 +2471,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;
 
@@ -2484,6 +2485,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;
@@ -2491,7 +2494,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)) {
 				mod_delayed_work(amt_wq, &tunnel->gc_wq,
@@ -2499,7 +2503,13 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
 								  * 3);
 				goto report;
 			} else {
+				/* The endpoint match is unique, so no other
+				 * tunnel can validate this Update. Count the
+				 * drop: an unauthenticated Update is not
+				 * observable from the gateway's own side.
+				 */
 				netdev_dbg(amt->dev, "Invalid MAC\n");
+				amt->dev->stats.rx_dropped++;
 				return true;
 			}
 		}
@@ -2705,7 +2715,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);
@@ -2743,6 +2754,14 @@ 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 is already udph->source. A
+	 * gateway that re-Requests from a new ephemeral port no longer
+	 * aliases onto this tunnel -- it gets its own, and this one ages
+	 * out on gc_wq. Do not "refresh" the port here: that is what made
+	 * a colliding Request steal an established tunnel outright.
+	 */
 	tunnel->nonce = amtrh->nonce;
 	mac = siphash_3u32((__force u32)tunnel->ip4,
 			   (__force u32)tunnel->source_port,
-- 
2.47.3

Re: [PATCH net v2 1/3] 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: 0 (1) · High: 1 · Medium: 1 · Low: 1 (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).

- [High] Keying the tunnel lookup on (saddr, sport) instead of saddr
  alone removes the only limit on how many tunnels one source address…
- [Medium] The new `amt->dev->stats.rx_dropped++` on the "Invalid MAC"
  arm of amt_update_handler() counts each rejected Update twice.
- [Low] The new `amt->dev->stats.rx_dropped++` in amt_update_handler() is
  a plain read-modify-write of the shared net_device counter from…

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but it is a
  remotely reachable use-after-free that is still present at the end of…
- [Low] amt_request_handler() does its (ip4, source_port) lookup
  locklessly, then takes amt->lock and allocates without looking again.
- [Low] In amt_request_handler() a new tunnel is published with
  list_add_tail_rcu() before tunnel->key is set, and…

--

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

This patch 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 as RFC 7450 s4.2.2 describes. It also counts
"Invalid MAC" Update drops in rx_dropped.

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

[ ... ]

> @@ -2491,7 +2494,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)) {
>  				mod_delayed_work(amt_wq, &tunnel->gc_wq,

[Severity: Critical]
This problem is older than this patch, but can this mod_delayed_work()
re-arm gc_wq on a tunnel that amt_tunnel_expire() is about to free?

The tunnel comes from a lockless RCU walk, and nothing checks that it is
still on the list before the work is re-armed. The workqueue clears the
pending bit before it calls amt_tunnel_expire(), which then does:

	spin_lock_bh(&amt->lock);
	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);

Here is one possible sequence:

CPU1                                CPU2
                                    amt_rcv()
                                      rcu_read_lock_bh()
                                      amt_update_handler()
                                        finds tunnel in tunnel_list
gc_wq fires, pending cleared
amt_tunnel_expire()
  list_del_rcu(&tunnel->list)
                                        mod_delayed_work(&tunnel->gc_wq)
                                          re-arms gc_wq.timer
  kfree_rcu(tunnel, rcu)
                                      rcu_read_unlock_bh()
tunnel freed after grace period,
gc_wq.timer still queued

The gc_wq expiry is much longer than a grace period, so the timer would
still be in the timer wheel after the tunnel is freed. When it fires,
delayed_work_timer_fn() would work on freed memory. A second
amt_tunnel_expire() would then repeat list_del_rcu() and underflow
nr_tunnels.

This patch makes the race easier to hit, because one host can now hold
many tunnels that expire separately. The same window still exists at the
end of the series. The locked re-check in amt_tunnel_get_or_create() can
find the tunnel while amt_tunnel_expire() waits on amt->lock, and the
caller re-arms gc_wq after it unlocks.

> @@ -2499,7 +2503,13 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
>  								  * 3);
>  				goto report;
>  			} else {
> +				/* The endpoint match is unique, so no other
> +				 * tunnel can validate this Update. Count the
> +				 * drop: an unauthenticated Update is not
> +				 * observable from the gateway's own side.
> +				 */

[Severity: Low]
This race is older than this patch, but is the endpoint match really
unique? amt_request_handler() does its (ip4, source_port) lookup without
amt->lock, then takes amt->lock and allocates without looking again:

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

Two concurrent Requests from the same endpoint could create duplicate
tunnels. The early return on this Invalid MAC arm assumes there is only
one.

Later in the series, "amt: do not create tunnel state for unauthenticated
Requests" seems to fix this. Its amt_tunnel_get_or_create() re-checks
under amt->lock, and that commit also removes this comment.

>  				netdev_dbg(amt->dev, "Invalid MAC\n");
> +				amt->dev->stats.rx_dropped++;

[Severity: Medium]
Does this count each rejected Update twice? amt_update_handler() returns
true here, and amt_rcv() already counts that on its drop path:

drivers/net/amt.c:amt_rcv() {
    ...
		case AMT_MSG_MEMBERSHIP_UPDATE:
			err = amt_update_handler(amt, skb);
			if (err)
				goto drop;
    ...
drop:
	if (err) {
		amt->dev->stats.rx_dropped++;
		kfree_skb(skb);
    ...
}

If so, this part of the commit message is not accurate:

  "Also count the "Invalid MAC" drop in rx_dropped. It is currently a
  netdev_dbg only"

Before this patch, the drop was already counted once through amt_rcv().
The double count is still there at the end of the series, in the
!verified branch of amt_update_handler() after "amt: do not create tunnel
state for unauthenticated Requests".

Could the added increment be removed?

[Severity: Low]
Separately, this is a plain read-modify-write of dev->stats.rx_dropped in
softirq context. amt_rcv() can run on several CPUs at once, so concurrent
invalid Updates can lose increments. This increment also races with the
plain increment at the drop label in amt_rcv().

Removing the extra increment above would fix this too. If it stays, would
DEV_STATS_INC() be the right form?

>  				return true;
>  			}
>  		}

[ ... ]

> @@ -2705,7 +2715,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);

[Severity: High]
With the port added to the key, can one host using its own address fill
the whole tunnel table?

Every Request from a new UDP source port now misses this lookup and
allocates a new amt_tunnel_list. The only admission check left is the
device-wide one just below:

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

A host can open max_tunnels (default 128) sockets on different ports and
send a Request from each. It receives the Membership Query on each port,
since it owns them, and answers with valid Updates. Each refreshing Update
keeps its entry alive through mod_delayed_work() in amt_update_handler().
Once the table is full, every other gateway's Request gets
ICMP_HOST_UNREACH.

No spoofing is needed, so source address filtering does not help. Before
this patch, a non-spoofing host could hold at most one tunnel.

Each of these tunnels also has its own group and source state, and
amt_dev_xmit() calls amt_send_multicast_data() once per tunnel. Would the
relay then send up to max_tunnels copies of each joined stream to the
same address?

The commit message mentions this:

  "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. [ ... ] 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."

This patch has a Fixes: tag, so it may be backported to stable without
the net-next patch. Should the per-source bound go in with this fix?

The problem also remains at the end of the series. After "amt: do not
create tunnel state for unauthenticated Requests", the only limit in
amt_tunnel_get_or_create() is still nr_tunnels >= max_tunnels. A
non-spoofing host passes MAC verification on each of its own ports.

[ ... ]

> @@ -2743,6 +2754,14 @@ 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

[ ... ]

> +	 */
>  	tunnel->nonce = amtrh->nonce;
>  	mac = siphash_3u32((__force u32)tunnel->ip4,
>  			   (__force u32)tunnel->source_port,

[Severity: Low]
This problem is older than this patch, but the allocation path above
publishes the tunnel before its key is set:

	list_add_tail_rcu(&tunnel->list, &amt->tunnel_list);
	tunnel->key = amt->key;

Here, tunnel->nonce and tunnel->mac are also written without a lock, while
amt_update_handler() reads them locklessly.

Could a concurrent Request that finds the new entry compute tunnel->mac
with a zero key? Could amt_update_handler() see a nonce/mac pair that
comes from two different Requests?

Later in the series, "amt: do not create tunnel state for unauthenticated
Requests" appears to resolve this. It removes the per-tunnel nonce, key
and mac, and amt_tunnel_get_or_create() publishes only fully initialised
entries.

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

[PATCH net v2 2/3] 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 nonce and
interval echoed by the Membership Update, 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.

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 ed82f8fac3f..492d450fdb5 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.47.3

Re: [PATCH net v2 2/3] amt: make pre-query report drops visible

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message's protocol explanation is wrong.

--

amt: make pre-query report drops visible

In gateway mode, amt_dev_xmit() drops an IGMP or MLD report when no
Membership Query has arrived yet for that family. This commit adds a
family-specific netdev_dbg() message before that drop. The drop path
itself is unchanged and still increments tx_dropped.

> 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 nonce and
> interval echoed by the Membership Update, 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.

[Severity: Low]
Is "The query supplies the nonce and interval echoed by the Membership
Update" accurate? The code doesn't seem to match it.

The gateway generates the nonce itself in amt_event_send_request():

drivers/net/amt.c:amt_event_send_request() {
    ...
		get_random_bytes(&amt->nonce, sizeof(__be32));
    ...
}

amt_membership_query_handler() only checks the nonce against that value.
>From the Query it stores the response MAC and the query interval:

drivers/net/amt.c:amt_membership_query_handler() {
    ...
	if (amtmq->nonce != amt->nonce)
		return true;

	response_mac = amtmq->response_mac;
    ...
		WRITE_ONCE(amt->ready4, true);
		amt->mac = response_mac;
		amt->req_cnt = 0;
		amt->qi = ihv3->qqic;
    ...
}

amt_send_membership_update() echoes only the nonce and the response MAC.
amt->qi is not part of the Update:

drivers/net/amt.c:amt_send_membership_update() {
    ...
	amtmu->nonce		= amt->nonce;
	amtmu->response_mac	= amt->mac;
    ...
}

So the state the Update needs from the relay's Query is the response
MAC, not the nonce or the interval. Could the commit message say that
instead?

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

[PATCH net v2 3/3] 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 emits 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, so
   max_tunnels now bounds verified gateways.
 - amt->key becomes a two-element array. amt_secret_work() rotates every
   AMT_SECRET_TIMEOUT (60s) and an exchange may straddle a rotation, so
   Update accepts the current or previous secret. Previously each tunnel
   snapshotted the key at creation, which a stateless recompute cannot do.

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

Two smaller issues on the same path go away with it: the entry was
published by list_add_tail_rcu() before ->key, ->nonce and ->mac were
assigned, leaving a window in which a concurrent Update could match the
zeroed nonce/mac of a kzalloc()'d entry; and the send path read
tunnel->key into an unused local before that field was initialised.

Note for reviewers: the MAC covers the UDP source port, so a NAT
rebinding between Request and Update now changes the recomputed MAC where
the stored value would have survived it. Including the port is what
RFC 7450 5.1.4.6 describes, and the exposed window is a single round trip
rather than the tunnel lifetime, but it is a behaviour change.

Fixes: cbc21dc1cfe9 ("amt: add data plane of amt interface")
Signed-off-by: Omar Ramadan <omar@blockcast.net>
---
Note for whoever merges this with the pending net-next per-source cap
("amt: bound relay tunnels admitted per source address"): that patch
counts the tunnels a source already holds on the tunnel_list walk in
amt_request_handler(), which this patch removes. The count belongs in
the re-check walk in amt_tunnel_get_or_create(), which already visits
every tunnel and runs under amt->lock. Pasting the bound back without
moving the count leaves it at zero, so it enforces nothing, and no
merge conflict will point that out.

 drivers/net/amt.c | 267 +++++++++++++++++++++++++++++-----------------
 include/net/amt.h |  11 +-
 2 files changed, 172 insertions(+), 106 deletions(-)

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index 492d450fdb5..bc8e552a9ba 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -784,11 +784,16 @@ static void amt_send_request(struct amt_dev *amt, bool v6)
 
 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,8 @@ 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));
+	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 +1125,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 +1155,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 +1171,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 +1185,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;
 }
 
@@ -2469,14 +2485,75 @@ static bool amt_membership_query_handler(struct amt_dev *amt,
 	return false;
 }
 
+/* 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.
+ */
+static struct amt_tunnel_list *amt_tunnel_get_or_create(struct amt_dev *amt,
+							__be32 saddr,
+							__be16 sport)
+{
+	struct amt_tunnel_list *tunnel;
+	int i;
+
+	list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list)
+		if (tunnel->ip4 == saddr && tunnel->source_port == sport)
+			return tunnel;
+
+	spin_lock_bh(&amt->lock);
+
+	/* Re-check under the lock; a concurrent Update may have won the race. */
+	list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list) {
+		if (tunnel->ip4 == saddr && tunnel->source_port == sport) {
+			spin_unlock_bh(&amt->lock);
+			return tunnel;
+		}
+	}
+
+	if (amt->nr_tunnels >= amt->max_tunnels) {
+		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]);
+
+	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++;
+	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;
+	bool verified = false;
+	siphash_key_t key[2];
+	int len, hdr_size, i;
+	u64 response_mac;
 	struct ethhdr *eth;
 	struct iphdr *iph;
-	int len, hdr_size;
-	u64 response_mac;
 	__be32 saddr;
 	__be32 nonce;
 	__be16 sport;
@@ -2496,36 +2573,58 @@ 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.  Both the current and the previous
+	 * secret are accepted so that an exchange straddling a rotation by
+	 * amt_secret_work() is not spuriously rejected.
+	 *
+	 * This runs before the packet is decapsulated so that an unverified
+	 * source is rejected without any further work being done on it.
+	 */
+	spin_lock_bh(&amt->lock);
+	key[0] = amt->key[0];
+	key[1] = amt->key[1];
+	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)) {
-				mod_delayed_work(amt_wq, &tunnel->gc_wq,
-						 msecs_to_jiffies(amt_gmi(amt))
-								  * 3);
-				goto report;
-			} else {
-				/* The endpoint match is unique, so no other
-				 * tunnel can validate this Update. Count the
-				 * drop: an unauthenticated Update is not
-				 * observable from the gateway's own side.
-				 */
-				netdev_dbg(amt->dev, "Invalid MAC\n");
-				amt->dev->stats.rx_dropped++;
-				return true;
-			}
+		if (response_mac == mac) {
+			verified = true;
+			break;
 		}
 	}
 
-	return true;
+	if (!verified) {
+		/* Count the drop: an unauthenticated Update is not observable
+		 * from the gateway's own side.
+		 */
+		netdev_dbg(amt->dev, "Invalid MAC\n");
+		amt->dev->stats.rx_dropped++;
+		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 has proved it received our
+		 * Query, so it cannot be used to reflect at a third party.
+		 */
+		icmp_ndo_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_UNREACH, 0);
+		return true;
+	}
+
+	mod_delayed_work(amt_wq, &tunnel->gc_wq,
+			 msecs_to_jiffies(amt_gmi(amt)) * 3);
+
+	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;
 
@@ -2702,15 +2801,27 @@ static bool amt_discovery_handler(struct amt_dev *amt, struct sk_buff *skb)
 	return false;
 }
 
+/* Handle an AMT Relay Membership Request.
+ *
+ * The source address of a Request is unauthenticated: it is a bare UDP
+ * datagram and can be trivially spoofed.  Allocating tunnel state here let an
+ * attacker fill the tunnel table (amt->max_tunnels entries, each held for
+ * amt_gmi()) from spoofed addresses, and let a single spoofed packet overwrite
+ * the nonce/MAC of an already-established tunnel and cut that gateway off.
+ *
+ * Nothing needs to be remembered at this point.  Every input to the keyed MAC
+ * is either 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;
 	u64 mac;
-	int i;
 
 	if (!pskb_may_pull(skb, sizeof(*udph) + sizeof(*amtrh)))
 		return true;
@@ -2722,68 +2833,24 @@ 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]);
 
-	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_lock_bh(&amt->lock);
+	key = amt->key[0];
 	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 is already udph->source. A
-	 * gateway that re-Requests from a new ephemeral port no longer
-	 * aliases onto this tunnel -- it gets its own, and this one ages
-	 * out on gc_wq. Do not "refresh" the port here: that is what made
-	 * a colliding Request steal an established tunnel outright.
-	 */
-	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))
-		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;
 }
@@ -3061,7 +3128,7 @@ 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));
+	get_random_bytes(&amt->key, sizeof(amt->key));
 
 	amt->status = AMT_STATUS_INIT;
 	if (amt->mode == AMT_MODE_GATEWAY) {
diff --git a/include/net/amt.h b/include/net/amt.h
index 2846dde0cad..297cbc91c1d 100644
--- a/include/net/amt.h
+++ b/include/net/amt.h
@@ -242,10 +242,6 @@ struct amt_tunnel_list {
 	struct delayed_work	gc_wq;
 	__be16			source_port;
 	__be32			ip4;
-	__be32			nonce;
-	siphash_key_t		key;
-	u64			mac:48,
-				reserved:16;
 	struct rcu_head		rcu;
 	struct hlist_head	groups[];
 };
@@ -325,8 +321,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] is the previous
+	 * generation, kept so that a Request/Update exchange straddling a
+	 * secret rotation still verifies.
+	 */
+	siphash_key_t		key[2];
 	struct sock	  __rcu *sk;
 	u32			max_groups;
 	u32			max_sources;
-- 
2.47.3

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (2) · Medium: 4 · 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] Response MACs now expire after 60–120 s.
- [Medium] When the table is full, the relay now refuses only after
  sending a Query.
- [Medium] Every Membership Update rejected for a bad MAC is counted
  twice in rx_dropped.
- [Medium] At device open, packets can be checked against MAC keys that
  are still zero, which lets an attacker create tunnels for spoofed…
- [Low] Parts of the commit message are wrong for the tree this patch
  applies to, and one behaviour change is not mentioned.
- [Low] The selftest's explanation is now out of date.

Pre-existing issues:
- [High] Use-after-free: an Update can re-arm gc_wq on a tunnel that
  amt_tunnel_expire() is unlinking and freeing.
- [High] This problem wasn't introduced by this patch, but amt_dev_stop()
  can unlink and free a tunnel at the same time as that tunnel's…

--

amt: do not create tunnel state for unauthenticated Requests

With this commit, amt_request_handler() no longer allocates tunnel state
for unauthenticated Relay Membership Requests. On Request the relay
computes the response MAC without storing anything. On Update it
recomputes the MAC, accepting either the current or the previous secret.
A tunnel is created only after that check passes.

> Note for reviewers: the MAC covers the UDP source port, so a NAT
> rebinding between Request and Update now changes the recomputed MAC where
> the stored value would have survived it.

[Severity: Low]
Is this accurate for the tree this applies to? In the parent, the Update
lookup already required a matching port:

-		if (tunnel->ip4 == saddr &&
-		    tunnel->source_port == sport) {

The stored MAC was also already computed over tunnel->source_port. So an
Update from a rebound port already matched no tunnel and was dropped.
That behaviour came from "amt: key relay tunnel state on the (address,
port) endpoint, not the address", not from this patch.

> Once full, every further
> Request also emits an ICMP_DEST_UNREACH to the spoofed source, turning
> the relay into an ICMP reflector.

The ICMP reflector is listed as one of the problems being fixed. However,
amt_request_handler() still sends an unsolicited Membership Query to
every claimed source. Only the type of the reply has changed.

The commit message also doesn't say that "relay full" is now reported on
the Update path rather than the Request path. A refused gateway now
always gets a Query, becomes ready, and then gets ICMP host unreachable
for each Update.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 492d450fdb55c..bc8e552a9bad4 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c

[ ... ]

> @@ -925,7 +934,8 @@ 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));
> +	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));

[Severity: Medium]
Does this give every response MAC a fixed lifetime that is shorter than
the Query Interval the relay advertises?

amt_secret_work() runs every AMT_SECRET_TIMEOUT (60000 ms) and only one
previous key is kept. A MAC issued at time t therefore stops verifying in
amt_update_handler() at the second rotation after t, which is 60 to 120
seconds later.

Before this patch, each tunnel copied amt->key when it was created, and
that copy never rotated. A gateway's (nonce, MAC) stayed valid for the
whole life of the tunnel.

The relay advertises QQIC = amt->qi (AMT_INIT_QUERY_INTERVAL, 125) in
amt_build_igmp_gq() and amt_build_mld_gq(). Gateways reuse the cached
nonce and MAC from the last Query for every Update, including unsolicited
joins and leaves:

drivers/net/amt.c:amt_send_membership_update() {
    ...
	amtmu->nonce		= amt->nonce;
	amtmu->response_mac	= amt->mac;
    ...
}

Take a third-party gateway that paces its Requests by the advertised
125 second interval. Its joins and leaves would be dropped as "Invalid
MAC" during the last 5 to 65 seconds before each refresh. A lost join
means no multicast until the next cycle. A lost leave means the relay
keeps forwarding until gc.

The Linux gateway in this file sends a new Request about every 2 seconds
(amt_event_send_request() with req_cnt reset by each Query), so it is
not affected.

The commit message describes the two-key window only as covering "an
exchange straddling a rotation". Should it mention this lifetime limit?
Alternatively, should the rotation period or the number of kept keys be
tied to amt->qi?

> @@ -1115,9 +1125,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.
> + */

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

# 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 change amt_request_handler() neither looks up nor creates a
tunnel. Should the selftest comment be updated to match?

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

[ ... ]

> @@ -2496,36 +2573,58 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *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;

[ ... ]

> +		if (response_mac == mac) {
> +			verified = true;
> +			break;
>  		}
>  	}
>  
> -	return true;
> +	if (!verified) {
> +		/* Count the drop: an unauthenticated Update is not observable
> +		 * from the gateway's own side.
> +		 */
> +		netdev_dbg(amt->dev, "Invalid MAC\n");
> +		amt->dev->stats.rx_dropped++;
> +		return true;
> +	}

[Severity: Medium]
Is this packet counted twice in rx_dropped?

amt_update_handler() increments rx_dropped and returns true. amt_rcv()
then takes its drop path for the same skb:

drivers/net/amt.c:amt_rcv() {
    ...
		case AMT_MSG_MEMBERSHIP_UPDATE:
			err = amt_update_handler(amt, skb);
			if (err)
				goto drop;
    ...
drop:
	if (err) {
		amt->dev->stats.rx_dropped++;
		kfree_skb(skb);
    ...
}

The extra increment came in with "amt: key relay tunnel state on the
(address, port) endpoint, not the address". This patch moves it but
keeps it, so every spoofed or stale-MAC Update raises rx_dropped by 2.

> +
> +	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 has proved it received our
> +		 * Query, so it cannot be used to reflect at a third party.
> +		 */
> +		icmp_ndo_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_UNREACH, 0);
> +		return true;
> +	}

[Severity: Medium]
When the table is full, does this stop the gateway from ever falling back
to rediscovery?

amt_request_handler() now answers every Request with a Query, whatever
the capacity, and the refusal only happens here. With the gateway code in
this file, the sequence becomes:

gateway: amt_membership_query_handler()
    amt->req_cnt = 0;  (ready4/ready6 set to true)
gateway sends Update
relay: amt_update_handler() -> icmp_ndo_send(ICMP_HOST_UNREACH)
gateway: amt_err_lookup()
    mod_delayed_work(amt_wq, &amt->req_wq, 0);
gateway: amt_event_send_request() sees req_cnt == 0, sends a new Request
relay answers with another Query, and req_cnt is reset again

As a result, this give-up branch in amt_event_send_request() can no
longer be reached:

	if (amt->req_cnt > AMT_MAX_REQ_COUNT) {
		...
		WRITE_ONCE(amt->remote_ip, 0);
		amt_update_gw_status(amt, AMT_STATUS_INIT, false);

Before this patch, a full relay answered the Request with ICMP and sent
no Query. req_cnt kept climbing, and the gateway went back to discovery,
where an anycast discovery address could pick another relay.

Now the gateway keeps cycling through Request, Query, Update and ICMP
with a relay that will never admit it, and it reports ready4/ready6 as
true the whole time. Is this change in behaviour intended?

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

[Severity: High]
This is a pre-existing issue, but can this re-arm gc_wq on a tunnel that
amt_tunnel_expire() is in the middle of freeing?

amt_tunnel_get_or_create() returns an existing tunnel either from the
lockless walk or from the re-check after amt->lock is dropped. Here the
only protection is rcu_read_lock_bh() from amt_rcv(). Suppose the
tunnel's gc_wq has already started running:

CPU1 (amt_wq)
amt_tunnel_expire()
    spin_lock_bh(&amt->lock);
    list_del_rcu(&tunnel->list);
    amt->nr_tunnels--;
    amt_clear_groups(tunnel);
    spin_unlock_bh(&amt->lock);
    kfree_rcu(tunnel, rcu);

CPU2 (amt_rcv)
amt_update_handler()
    tunnel = amt_tunnel_get_or_create(...);  /* same tunnel */
    mod_delayed_work(amt_wq, &tunnel->gc_wq, ...);
    amt_igmp_report_handler(amt, skb, tunnel);

The work is running rather than pending, so mod_delayed_work() queues it
again and arms a timer inside an object that is waiting for kfree_rcu().
The report handler also attaches new group and source nodes after
amt_clear_groups() has already run.

When that timer fires, amt_tunnel_expire() runs on freed memory. It calls
list_del_rcu() on a poisoned entry and decrements nr_tunnels a second
time, which underflows it and bypasses max_tunnels. The orphaned group
nodes leak, with timers that point at the freed tunnel.

The lookup has now moved into a new helper. Would it be worth checking
under amt->lock that the tunnel is still on the list (or not marked dead)
before gc_wq is re-armed?

[ ... ]

> @@ -3061,7 +3128,7 @@ 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));
> +	get_random_bytes(&amt->key, sizeof(amt->key));

[Severity: Medium]
Can Updates be verified against all-zero keys while the device is being
opened?

amt_socket_create() -> setup_udp_tunnel_sock() publishes sk_user_data,
installs encap_rcv = amt_rcv and enables encap before it returns. The
keys are seeded only after that, and without amt->lock:

amt_dev_open()
    err = amt_socket_create(amt);
    ...
    get_random_bytes(&amt->key, sizeof(amt->key));

On the first bring-up the netdev private data is zero-filled, so key[0]
and key[1] are both zero during that window.

With this patch, the only check before admitting a tunnel is recomputing
siphash_3u32(saddr, sport, nonce, key) >> 16. A spoofed Update carrying a
MAC computed offline under the zero key would pass amt_update_handler()
and get a tunnel, plus group joins that forward multicast to the spoofed
address. That entry stays after the keys are randomized.

amt_request_handler(), amt_update_handler() and amt_secret_work() all
access amt->key[] under amt->lock, so readers here can also see a
half-written key. Should the keys be seeded under amt->lock before
amt_socket_create() is called?

[Severity: High]
This is a pre-existing issue and was not introduced by this patch.
While looking at tunnel lifetime, though, can amt_dev_stop() double free
a tunnel whose amt_tunnel_expire() is running at the same time?

drivers/net/amt.c:amt_dev_stop() {
    ...
	list_for_each_entry_safe(tunnel, tmp, &amt->tunnel_list, list) {
		list_del_rcu(&tunnel->list);
		amt->nr_tunnels--;
		cancel_delayed_work_sync(&tunnel->gc_wq);
		amt_clear_groups(tunnel);
		kfree_rcu(tunnel, rcu);
	}
    ...
}

amt->lock is not held here, and the tunnel is unlinked before gc_wq is
cancelled.

Suppose amt_tunnel_expire() is already running for that tunnel. It takes
amt->lock and calls list_del_rcu() on an entry that dev_stop has already
unlinked, which writes through LIST_POISON2.

cancel_delayed_work_sync() then waits for expire, which calls
kfree_rcu(tunnel), and dev_stop calls kfree_rcu(tunnel) a second time.
nr_tunnels is also decremented twice.

>  
>  	amt->status = AMT_STATUS_INIT;
>  	if (amt->mode == AMT_MODE_GATEWAY) {

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

Re: [PATCH net v2 0/3] amt: fix relay tunnel keying and unauthenticated-Request DoS

[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:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - 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 v2 0/3] amt: fix relay tunnel keying and unauthenticated-Request DoS

[Omar Ramadan]

Thanks -- both are fair asks. Answers below; they are also reflected in
the v2 cover letter. v1 was generated against v7.1 and did not apply to
net, so this is answered against v2, which is posted against net with the
General Query patch dropped (that fix is already in net as afae89de73dd).
v2 is three patches.

How it was found
  Manual code inspection of the AMT relay path in drivers/net/amt.c, read
  against RFC 7450, with LLM assistance -- hence the Assisted-by: LLM
  trailer on patch 3/3. It was not a syzbot report or a static-analysis
  tool scan. Patch 1/3 (endpoint keying) came out of the same reading of
  amt_request_handler() and amt_update_handler() against RFC 7450 s4.2.2.

Whether it was triggered
  Found by inspection; not observed in production. These are availability
  and correctness defects, not memory-safety bugs, so there is no oops or
  stack trace to attach. The symptoms follow deterministically from the
  code the diffs change:

   - 3/3, exhaustion: amt_request_handler() allocated a tunnel before any
     validation of the source, so Relay Membership Requests from distinct
     spoofable source endpoints fill the table to max_tunnels (default
     128). Once full, further Requests are answered with ICMP_DEST_UNREACH
     to the (spoofed) source and genuine gateways are refused; re-sending
     once per amt_gmi() interval holds it full.
   - 3/3, desync: the pre-validation lookup jumped to the send path and
     overwrote an established tunnel's ->nonce/->mac, so a single spoofed
     Request carrying a known gateway's source endpoint made that
     gateway's next Membership Update fail the "Invalid MAC" check -- a
     silent one-packet denial that consumes no table slot.
   - 1/3, aliasing: address-only keying collapses two endpoints that share
     a source address (NAT, or one host using separate IPv4/IPv6 ports per
     RFC 7450 s4.2.2) onto one tunnel; the later Request wins and the
     earlier gateway silently stops receiving.

  I have not staged a live end-to-end exploit run -- the above is read
  from the code paths, not a captured trace.

Fix testing (this part is observed, not inferred)
  The three patches were applied to net and the kernel booted (arm64,
  QEMU via virtme-ng) with CONFIG_KASAN=y, CONFIG_PROVE_LOCKING=y,
  CONFIG_PROVE_RCU=y and CONFIG_DEBUG_LIST=y on top of
  tools/testing/selftests/net/config. tools/testing/selftests/net/amt.sh
  passes all six tests -- amt discovery, IPv4 and IPv6 multicast
  forwarding, and both IPv4/IPv6 traffic-forwarding torture cases all
  report [ OK ] -- with no KASAN, lockdep or RCU reports.

Re: [PATCH net v2 0/3] amt: fix relay tunnel keying and unauthenticated-Request DoS

[Taehee Yoo]

On Sat, Oct 10, 2026 at 6:37 AM Omar Ramadan <omar@blockcast.net> wrote:
>

Hi Omar,

Thank you so much for your work!
I'm currently traveling for LPC, so my review will be delayed.
Sorry for the delay. I will take a look at your patches in the next 2-3 days.

Thanks a lot!
Taehee Yoo

> Thanks -- both are fair asks. Answers below; they are also reflected in
> the v2 cover letter. v1 was generated against v7.1 and did not apply to
> net, so this is answered against v2, which is posted against net with the
> General Query patch dropped (that fix is already in net as afae89de73dd).
> v2 is three patches.
>
> How it was found
>   Manual code inspection of the AMT relay path in drivers/net/amt.c, read
>   against RFC 7450, with LLM assistance -- hence the Assisted-by: LLM
>   trailer on patch 3/3. It was not a syzbot report or a static-analysis
>   tool scan. Patch 1/3 (endpoint keying) came out of the same reading of
>   amt_request_handler() and amt_update_handler() against RFC 7450 s4.2.2.
>
> Whether it was triggered
>   Found by inspection; not observed in production. These are availability
>   and correctness defects, not memory-safety bugs, so there is no oops or
>   stack trace to attach. The symptoms follow deterministically from the
>   code the diffs change:
>
>    - 3/3, exhaustion: amt_request_handler() allocated a tunnel before any
>      validation of the source, so Relay Membership Requests from distinct
>      spoofable source endpoints fill the table to max_tunnels (default
>      128). Once full, further Requests are answered with ICMP_DEST_UNREACH
>      to the (spoofed) source and genuine gateways are refused; re-sending
>      once per amt_gmi() interval holds it full.
>    - 3/3, desync: the pre-validation lookup jumped to the send path and
>      overwrote an established tunnel's ->nonce/->mac, so a single spoofed
>      Request carrying a known gateway's source endpoint made that
>      gateway's next Membership Update fail the "Invalid MAC" check -- a
>      silent one-packet denial that consumes no table slot.
>    - 1/3, aliasing: address-only keying collapses two endpoints that share
>      a source address (NAT, or one host using separate IPv4/IPv6 ports per
>      RFC 7450 s4.2.2) onto one tunnel; the later Request wins and the
>      earlier gateway silently stops receiving.
>
>   I have not staged a live end-to-end exploit run -- the above is read
>   from the code paths, not a captured trace.
>
> Fix testing (this part is observed, not inferred)
>   The three patches were applied to net and the kernel booted (arm64,
>   QEMU via virtme-ng) with CONFIG_KASAN=y, CONFIG_PROVE_LOCKING=y,
>   CONFIG_PROVE_RCU=y and CONFIG_DEBUG_LIST=y on top of
>   tools/testing/selftests/net/config. tools/testing/selftests/net/amt.sh
>   passes all six tests -- amt discovery, IPv4 and IPv6 multicast
>   forwarding, and both IPv4/IPv6 traffic-forwarding torture cases all
>   report [ OK ] -- with no KASAN, lockdep or RCU reports.