[PATCH net v2] amt: do not store tunnel pointer in skb control block

[PATCH net v2] amt: do not store tunnel pointer in skb control block

[Cen Zhang (Microsoft Security FORGE Labs)]

An skb queued in a qdisc can outlive the tunnel it references
through a raw pointer in skb->cb. For example, a netem delay of
180s exceeds the default tunnel lifetime of 135s (igmp_qrv=1);
when the tunnel expires and is freed, the subsequent dequeue
triggers a use-after-free in amt_dev_xmit().

  BUG: KASAN: slab-use-after-free in amt_dev_xmit+0x2763/0x2e20
  Call Trace:
   amt_dev_xmit+0x2763/0x2e20 [drivers/net/amt.c:1262]
   dev_hard_start_xmit+0x22f/0x620
   sch_direct_xmit+0x12e/0xac0
   netem_dequeue+0x333/0xc50
   net_tx_action+0x35c/0xa60

Store the tunnel identity (ip4 + source_port) in skb->cb instead
of a pointer, and re-lookup the tunnel under RCU in amt_dev_xmit().
If the tunnel is gone, the query is simply dropped.

A refcount fix would be hard to keep balanced here, as the skb may
be dropped or cloned by the qdisc layer before reaching
amt_dev_xmit().

Fixes: cbc21dc1cfe9 ("amt: add data plane of amt interface")
Reported-by: AutonomousCodeSecurity@microsoft.com
Reported-by: Xiang Mei (Microsoft) <xmei5@asu.edu>
Reported-by: Cen Zhang (Microsoft Security FORGE Labs) <cenzhang@linux.microsoft.com>
Signed-off-by: Cen Zhang (Microsoft Security FORGE Labs) <cenzhang@linux.microsoft.com>
---
v2:
 - Drop the comment above struct amt_skb_cb (Taehee Yoo)
 - Mention in the commit message why a refcount is not used
   (Taehee Yoo)
 - Rebase on net/main
v1: https://lore.kernel.org/netdev/20260818164825.63967-1-blbllhy@gmail.com/

 drivers/net/amt.c | 34 +++++++++++++++++++++++++---------
 include/net/amt.h |  3 ++-
 2 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index bddc24e1856..b660cebf248 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -791,6 +791,18 @@ static void amt_send_request(struct amt_dev *amt, bool v6)
 	rcu_read_unlock();
 }
 
+static struct amt_tunnel_list *amt_lookup_tunnel(struct amt_dev *amt,
+						 __be32 ip4, __be16 source_port)
+{
+	struct amt_tunnel_list *tunnel;
+
+	list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list)
+		if (tunnel->ip4 == ip4 && tunnel->source_port == source_port)
+			return tunnel;
+
+	return NULL;
+}
+
 static void amt_send_igmp_gq(struct amt_dev *amt,
 			     struct amt_tunnel_list *tunnel)
 {
@@ -800,7 +812,8 @@ static void amt_send_igmp_gq(struct amt_dev *amt,
 	if (!skb)
 		return;
 
-	amt_skb_cb(skb)->tunnel = tunnel;
+	amt_skb_cb(skb)->tunnel_ip4 = tunnel->ip4;
+	amt_skb_cb(skb)->tunnel_port = tunnel->source_port;
 	dev_queue_xmit(skb);
 }
 
@@ -885,7 +898,8 @@ static void amt_send_mld_gq(struct amt_dev *amt, struct amt_tunnel_list *tunnel)
 	if (!skb)
 		return;
 
-	amt_skb_cb(skb)->tunnel = tunnel;
+	amt_skb_cb(skb)->tunnel_ip4 = tunnel->ip4;
+	amt_skb_cb(skb)->tunnel_port = tunnel->source_port;
 	dev_queue_xmit(skb);
 }
 #else
@@ -1262,15 +1276,17 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 		goto unlock;
 	} else if (amt->mode == AMT_MODE_RELAY) {
 		if (query) {
-			tunnel = amt_skb_cb(skb)->tunnel;
-			if (!tunnel) {
-				WARN_ON(1);
-				goto free;
-			}
-
+			rcu_read_lock();
+			tunnel = amt_lookup_tunnel(amt,
+						   amt_skb_cb(skb)->tunnel_ip4,
+						   amt_skb_cb(skb)->tunnel_port);
 			/* Do not forward unexpected query */
-			if (amt_send_membership_query(amt, skb, tunnel, v6))
+			if (!tunnel ||
+			    amt_send_membership_query(amt, skb, tunnel, v6)) {
+				rcu_read_unlock();
 				goto free;
+			}
+			rcu_read_unlock();
 			goto unlock;
 		}
 
diff --git a/include/net/amt.h b/include/net/amt.h
index a0255491f5b..8727cf007bc 100644
--- a/include/net/amt.h
+++ b/include/net/amt.h
@@ -232,7 +232,8 @@ struct amt_relay_headers {
 } __packed;
 
 struct amt_skb_cb {
-	struct amt_tunnel_list *tunnel;
+	__be32			tunnel_ip4;
+	__be16			tunnel_port;
 };
 
 struct amt_tunnel_list {
-- 
2.53.0

Re: [PATCH net v2] amt: do not store tunnel pointer in skb control block

[Omar Ramadan]

[Resending with the list and maintainers on Cc; my first copy went to
Cen only. Sorry for the duplicate, Cen.]

Hi Cen,

On Tue, Sep 22, 2026 at 05:41:50PM -0400, Cen Zhang (Microsoft Security FORGE Labs) wrote:
> An skb queued in a qdisc can outlive the tunnel it references
> through a raw pointer in skb->cb. For example, a netem delay of
> 180s exceeds the default tunnel lifetime of 135s (igmp_qrv=1);
> when the tunnel expires and is freed, the subsequent dequeue
> triggers a use-after-free in amt_dev_xmit().

Thanks for finding this and for sticking with it. I agree with the
analysis and the trace. Nothing pins the tunnel while the General
Query waits in the qdisc, and amt_tunnel_expire() frees it with
kfree_rcu() well before a 180s netem delay runs out.

> Store the tunnel identity (ip4 + source_port) in skb->cb instead
> of a pointer, and re-lookup the tunnel under RCU in amt_dev_xmit().
> If the tunnel is gone, the query is simply dropped.
>
> A refcount fix would be hard to keep balanced here, as the skb may
> be dropped or cloned by the qdisc layer before reaching
> amt_dev_xmit().

Agreed on the refcount. Could we avoid both the refcount and the
lookup by not sending the relay's GQ through amt_dev_xmit() at all?

amt_send_igmp_gq() and amt_send_mld_gq() have one caller,
amt_request_handler(). It runs inside the rcu_read_lock_bh() section
of amt_rcv() and already holds the right tunnel, whether it found the
tunnel or just created it. For this skb, dev_queue_xmit() only leads
back into amt_dev_xmit(), which strips the Ethernet header and calls
amt_send_membership_query(amt, skb, tunnel, v6). The two senders can
make that call themselves, in the same way that
amt_send_advertisement() already transmits from this receive path.

With that change:
 - the skb never waits in a qdisc, so there is no lifetime window;
 - the tunnel is only dereferenced inside the RCU section that found
   or created it, and rcu_dereference_bh(amt->sk) in the sender stays
   covered;
 - it is O(1): nothing is stored in skb->cb, and there is no lookup
   and no refcount.

Once the round trip is gone, the query branch in amt_dev_xmit(),
amt_skb_cb() and struct amt_skb_cb have no users left and can be
removed.

> +	list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list)
> +		if (tunnel->ip4 == ip4 && tunnel->source_port == source_port)
> +			return tunnel;

This keeps the per-Query walk of tunnel_list that Taehee raised on
v1. Space isn't the issue: your (ip4, port) key fits in the free cb
bytes, and so would a u32 tunnel id. The cost is that any key needs a
lookup at dequeue time, plus a rule for a tunnel that expired or was
recreated in the meantime, while the sender already has the tunnel in
hand.

These are the behaviour changes I'm aware of. The first two only
affect the relay's own GQs; the third affects foreign queries:
 - They no longer go through a qdisc or taps on the amt device. They
   are still visible as UDP on the underlay.
 - They are no longer counted as tx_dropped when sent successfully.
   Today the query branch in amt_dev_xmit() exits through the unlock
   label, which counts every sent query as dropped.
 - A query that reaches amt_dev_xmit() on a relay from somewhere
   else, such as a userspace querier, is now dropped at the IGMP/MLD
   type switch. Before, it trusted whatever skb->cb held, and a NULL
   hit the WARN_ON(1). I found this by reading the code and have not
   exercised it.

What I have and haven't tested:
 - The diff below applies to net at 9c572a83037a. It builds with W=1
   and no warnings (gcc 14, arm64, CONFIG_IPV6=y and =n), and
   checkpatch --strict is clean. I haven't booted it.
 - An earlier form of this direct send (without the dead-code removal
   or the failure accounting) passed amt.sh 5/5 under vng in August,
   on a v7.1 tree carrying our other pending AMT patches. That was not
   a KASAN build, and it predates this diff. Our out-of-tree module's
   source carries the same earlier form.
 - I haven't reproduced the KASAN report. Could you share the netem
   setup you used, so I can run it before and after? The GQ no longer
   enters the qdisc, so I expect it to stop triggering by
   construction, but I'd want to see that before this goes in.

If you and Taehee like this direction, either of these works for me:
 - I post it as v3 with your KASAN trace in the commit message,
   keeping the existing Reported-by tags (you're already one), plus
   Co-developed-by if you'd like; that needs your Signed-off-by.
 - You fold it into your v3 with a Suggested-by.

Taehee, would this be acceptable to you?

Thanks,
Omar

-- >8 --
diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index bddc24e1856d..b53f8ec55661 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -80,15 +80,6 @@ static struct in6_addr mld2_all_node = MLD2_ALL_NODE_INIT;
 static struct mld2_grec mldv2_zero_grec;
 #endif
 
-static struct amt_skb_cb *amt_skb_cb(struct sk_buff *skb)
-{
-	BUILD_BUG_ON(sizeof(struct amt_skb_cb) + sizeof(struct tc_skb_cb) >
-		     sizeof_field(struct sk_buff, cb));
-
-	return (struct amt_skb_cb *)((void *)skb->cb +
-		sizeof(struct tc_skb_cb));
-}
-
 static void __amt_source_gc_work(void)
 {
 	struct amt_source_node *snode;
@@ -791,6 +782,11 @@ static void amt_send_request(struct amt_dev *amt, bool v6)
 	rcu_read_unlock();
 }
 
+static bool amt_send_membership_query(struct amt_dev *amt,
+				      struct sk_buff *skb,
+				      struct amt_tunnel_list *tunnel,
+				      bool v6);
+
 static void amt_send_igmp_gq(struct amt_dev *amt,
 			     struct amt_tunnel_list *tunnel)
 {
@@ -800,8 +796,11 @@ static void amt_send_igmp_gq(struct amt_dev *amt,
 	if (!skb)
 		return;
 
-	amt_skb_cb(skb)->tunnel = tunnel;
-	dev_queue_xmit(skb);
+	skb_pull(skb, sizeof(struct ethhdr));
+	if (amt_send_membership_query(amt, skb, tunnel, false)) {
+		amt->dev->stats.tx_dropped++;
+		kfree_skb(skb);
+	}
 }
 
 #if IS_ENABLED(CONFIG_IPV6)
@@ -885,8 +884,11 @@ static void amt_send_mld_gq(struct amt_dev *amt, struct amt_tunnel_list *tunnel)
 	if (!skb)
 		return;
 
-	amt_skb_cb(skb)->tunnel = tunnel;
-	dev_queue_xmit(skb);
+	skb_pull(skb, sizeof(struct ethhdr));
+	if (amt_send_membership_query(amt, skb, tunnel, 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)
@@ -1186,7 +1188,6 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 #endif
 	bool report = false;
 	struct igmphdr *ih;
-	bool query = false;
 	struct iphdr *iph;
 	bool data = false;
 	bool v6 = false;
@@ -1204,9 +1205,6 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 			case IGMP_HOST_MEMBERSHIP_REPORT:
 				report = true;
 				break;
-			case IGMP_HOST_MEMBERSHIP_QUERY:
-				query = true;
-				break;
 			default:
 				goto free;
 			}
@@ -1228,9 +1226,6 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 			case ICMPV6_MLD2_REPORT:
 				report = true;
 				break;
-			case ICMPV6_MGM_QUERY:
-				query = true;
-				break;
 			default:
 				goto free;
 			}
@@ -1261,19 +1256,6 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 			goto free;
 		goto unlock;
 	} else if (amt->mode == AMT_MODE_RELAY) {
-		if (query) {
-			tunnel = amt_skb_cb(skb)->tunnel;
-			if (!tunnel) {
-				WARN_ON(1);
-				goto free;
-			}
-
-			/* Do not forward unexpected query */
-			if (amt_send_membership_query(amt, skb, tunnel, v6))
-				goto free;
-			goto unlock;
-		}
-
 		if (!data)
 			goto free;
 		list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list) {
diff --git a/include/net/amt.h b/include/net/amt.h
index a0255491f5b0..2846dde0cadc 100644
--- a/include/net/amt.h
+++ b/include/net/amt.h
@@ -231,10 +231,6 @@ struct amt_relay_headers {
 	};
 } __packed;
 
-struct amt_skb_cb {
-	struct amt_tunnel_list *tunnel;
-};
-
 struct amt_tunnel_list {
 	struct list_head	list;
 	/* Protect All resources under an amt_tunne_list */

Re: [PATCH net v2] amt: do not store tunnel pointer in skb control block

[Cen Zhang (Microsoft Security FORGE Labs)]

Hi Omar,

On Thu, Sep 24, 2026 at 01:56:31AM +0300, Omar Ramadan wrote:
> Agreed on the refcount. Could we avoid both the refcount and the
> lookup by not sending the relay's GQ through amt_dev_xmit() at all?

Thanks for the exploration and the thoughtful solution. I did not
consider removing the round trip in the first place, since that
looked like a user-visible behaviour change, but your write-up
covers it well.

> I haven't reproduced the KASAN report. Could you share the netem
> setup you used, so I can run it before and after?

I will send you and Taehee the reproducer artifact in private soon.
It covers the base, base + your diff and base + v2.

> If you and Taehee like this direction, either of these works for me:
>  - I post it as v3 with your KASAN trace in the commit message,
>    keeping the existing Reported-by tags (you're already one), plus
>    Co-developed-by if you'd like; that needs your Signed-off-by.

If Taehee is fine with it, let's go with your v3. I am fine with the
credit as long as the CVE credits the report to Xiang and I (so for
my institution).

Thanks,
Cen

Re: [PATCH net v2] amt: do not store tunnel pointer in skb control block

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 1 (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 new identity-based re-lookup in amt_dev_xmit() can return
  a tunnel object that amt_request_handler() has published to the RCU…

Pre-existing issues:
- [High] amt_dev_stop() mutates amt->tunnel_list without holding
  amt->lock, racing with amt_tunnel_expire() which mutates the same list…
- [Medium] amt_dev_xmit() counts successfully transmitted packets as TX
  drops.

--

amt: do not store tunnel pointer in skb control block

This replaces the raw struct amt_tunnel_list pointer kept in skb->cb with
the tunnel identity (ip4 + source_port), and re-looks-up the tunnel under
RCU in amt_dev_xmit() so that an skb outliving its tunnel in a qdisc no
longer dereferences freed memory.  A few questions about the new lookup and
about surrounding code it now depends on.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index bddc24e1856de..b660cebf2484c 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -791,6 +791,18 @@ static void amt_send_request(struct amt_dev *amt, bool v6)
>  	rcu_read_unlock();
>  }
>  
> +static struct amt_tunnel_list *amt_lookup_tunnel(struct amt_dev *amt,
> +						 __be32 ip4, __be16 source_port)
> +{
> +	struct amt_tunnel_list *tunnel;
> +
> +	list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list)
> +		if (tunnel->ip4 == ip4 && tunnel->source_port == source_port)
> +			return tunnel;
> +
> +	return NULL;
> +}
> +

[Severity: High]
This is a pre-existing issue and not introduced here, but since this new
lockless walk now depends on the integrity of amt->tunnel_list, is the list
actually consistently protected?

amt_tunnel_expire() deletes under amt->lock:

drivers/net/amt.c:amt_tunnel_expire() {
	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);
}

amt_dev_stop() deletes the same entries with no lock at all, and only
cancels the per-tunnel gc work after it has already unlinked and while
other tunnels' gc works can still be running:

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_dev_stop() does not disable the gc_wq of any tunnel before the loop
(only req_wq, discovery_wq, secret_wq and event_wq are cancelled earlier),
so a gc timer firing while the device is being brought down runs
amt_tunnel_expire() concurrently.  Can that give two list_del_rcu() calls
on the same entry, where the second __list_del_entry() writes through
LIST_POISON2, plus a double kfree_rcu() of the same tunnel?  And can the
concurrent unsynchronized list and nr_tunnels updates leave a stale next
pointer for amt_lookup_tunnel() and for the data-forwarding
list_for_each_entry_rcu() walk in amt_dev_xmit() to follow?

Would taking amt->lock around the amt_dev_stop() removals (and stopping the
gc work before unlinking) be the right fix?

[ ... ]

> @@ -1262,15 +1276,17 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>  		goto unlock;
>  	} else if (amt->mode == AMT_MODE_RELAY) {
>  		if (query) {
> -			tunnel = amt_skb_cb(skb)->tunnel;
> -			if (!tunnel) {
> -				WARN_ON(1);
> -				goto free;
> -			}
> -
> +			rcu_read_lock();
> +			tunnel = amt_lookup_tunnel(amt,
> +						   amt_skb_cb(skb)->tunnel_ip4,
> +						   amt_skb_cb(skb)->tunnel_port);

[Severity: Medium]
Can this lookup return a tunnel that amt_request_handler() has published but
not finished initializing?

amt_request_handler() sets only source_port and ip4 before publishing, and
writes nonce/mac after dropping amt->lock, at the send: label:

drivers/net/amt.c:amt_request_handler() {
	tunnel->source_port = udph->source;
	tunnel->ip4 = iph->saddr;
	...
	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;
	...
	tunnel->mac = mac >> 16;
}

amt_lookup_tunnel() matches purely on ip4 + source_port, and both are
already visible at list_add_tail_rcu() time.  So if an older query skb for
an expired tunnel is still sitting in a qdisc (the netem case from the
commit message) and the same gateway re-sends a Request from the same UDP
source port, can the dequeue on another CPU match the new, half-initialized
tunnel and build the query from the kzalloc'ed zeros?

drivers/net/amt.c:amt_send_membership_query() {
	amtmq->nonce	= tunnel->nonce;
	amtmq->response_mac = tunnel->mac;
	...
	amt_update_relay_status(tunnel, AMT_STATUS_SENT_QUERY, true);
}

The gateway then discards such a query:

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

while the relay has already moved that tunnel to AMT_STATUS_SENT_QUERY.
Would completing the tunnel initialization (key, nonce, mac) before
list_add_tail_rcu(), or carrying the nonce as part of the identity stored in
skb->cb, close this window?

>  			/* Do not forward unexpected query */
> -			if (amt_send_membership_query(amt, skb, tunnel, v6))
> +			if (!tunnel ||
> +			    amt_send_membership_query(amt, skb, tunnel, v6)) {
> +				rcu_read_unlock();
>  				goto free;
> +			}
> +			rcu_read_unlock();
>  			goto unlock;

[Severity: Medium]
This isn't a bug introduced by this patch, but this goto unlock is the
success exit of the path being rewritten here, and it lands on the drop
counter:

drivers/net/amt.c:amt_dev_xmit() {
free:
	dev_kfree_skb(skb);
unlock:
	dev->stats.tx_dropped++;
	return NETDEV_TX_OK;
}

The free: label falls through into the same increment, so a query that was
successfully handed to udp_tunnel_xmit_skb() is counted as tx_dropped, the
same as one that was thrown away.  The gateway report path reaches the same
label after amt_send_membership_update() succeeds.  Since tx_dropped in
struct rtnl_link_stats64 is defined as packets dropped on their way to
transmission, and tx_packets/tx_bytes are never incremented for these
paths, should the success exits use a separate label that skips the
tx_dropped increment?

>  		}
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922214150.13970-1-cenzhang%40linux.microsoft.com

Re: [PATCH net v2] amt: do not store tunnel pointer in skb control block

[Omar Ramadan]

On Sat, Sep 26, 2026, Sashiko wrote:
> [Severity: Medium]
> Can this lookup return a tunnel that amt_request_handler() has published but
> not finished initializing?

Yes, I think this one is real for v2. amt_request_handler() makes a
new tunnel visible with list_add_tail_rcu() before it writes nonce and
mac at the send: label. A query left in the qdisc from an expired
tunnel can therefore match a re-created tunnel for the same gateway
and port during that window.

The direct-send approach proposed earlier in this thread avoids it by
construction. amt_send_igmp_gq() and amt_send_mld_gq() run at the end
of amt_request_handler(), after the same context has written nonce and
mac. They call amt_send_membership_query() for that tunnel, and nothing
is looked up again at dequeue. I've posted that as v3:
https://lore.kernel.org/netdev/20260928181601.85857-1-omar@blockcast.net/

I ran Cen's reproducer on net at 17741334d00 (KASAN, slub_debug=FZU):
the unpatched tree reports the slab-use-after-free in amt_dev_xmit(),
and both v2 and the direct-send diff run clean. The reproducer does
not exercise the re-creation window above, so this confirms the UAF
fix, not the v2 race.

> [Severity: Medium]
> [...] a query that was successfully handed to udp_tunnel_xmit_skb() is
> counted as tx_dropped

v3 removes the relay query branch from amt_dev_xmit(), so a General
Query that was sent is no longer counted as dropped. The gateway report
path (a successful amt_send_membership_update() followed by goto
unlock) has the same miscount. That is independent of the UAF; I'll
send a separate patch for it once v3 is in, since it applies on top.

> [Severity: High]
> amt_dev_stop() deletes the same entries with no lock at all

This predates the fix and is independent of it. Reading net, it looks
right to me: nothing disables the per-tunnel gc_wq before the unlocked
loop. cancel_delayed_work_sync() comes after list_del_rcu(), so it can
wait for a running amt_tunnel_expire() that then deletes the entry and
calls kfree_rcu() on it a second time. Cen already posted a fix for
this, "amt: fix tunnel list corruption on device stop":
https://patchwork.kernel.org/project/netdevbpf/patch/20260822045407.28983-1-blbllhy@gmail.com/
so I'll leave that one to that thread.

pw-bot: cr

Re: [PATCH net v2] amt: do not store tunnel pointer in skb control block

[Taehee Yoo]

On Thu, Sep 24, 2026 at 7:58 AM Omar Ramadan <omar@blockcast.net> wrote:
>

Hi Omar and Cen,

Thank you so much for this work!

> [Resending with the list and maintainers on Cc; my first copy went to
> Cen only. Sorry for the duplicate, Cen.]
>
> Hi Cen,
>
> On Tue, Sep 22, 2026 at 05:41:50PM -0400, Cen Zhang (Microsoft Security FORGE Labs) wrote:
> > An skb queued in a qdisc can outlive the tunnel it references
> > through a raw pointer in skb->cb. For example, a netem delay of
> > 180s exceeds the default tunnel lifetime of 135s (igmp_qrv=1);
> > when the tunnel expires and is freed, the subsequent dequeue
> > triggers a use-after-free in amt_dev_xmit().
>
> Thanks for finding this and for sticking with it. I agree with the
> analysis and the trace. Nothing pins the tunnel while the General
> Query waits in the qdisc, and amt_tunnel_expire() frees it with
> kfree_rcu() well before a 180s netem delay runs out.
>
> > Store the tunnel identity (ip4 + source_port) in skb->cb instead
> > of a pointer, and re-lookup the tunnel under RCU in amt_dev_xmit().
> > If the tunnel is gone, the query is simply dropped.
> >
> > A refcount fix would be hard to keep balanced here, as the skb may
> > be dropped or cloned by the qdisc layer before reaching
> > amt_dev_xmit().
>
> Agreed on the refcount. Could we avoid both the refcount and the
> lookup by not sending the relay's GQ through amt_dev_xmit() at all?
>
> amt_send_igmp_gq() and amt_send_mld_gq() have one caller,
> amt_request_handler(). It runs inside the rcu_read_lock_bh() section
> of amt_rcv() and already holds the right tunnel, whether it found the
> tunnel or just created it. For this skb, dev_queue_xmit() only leads
> back into amt_dev_xmit(), which strips the Ethernet header and calls
> amt_send_membership_query(amt, skb, tunnel, v6). The two senders can
> make that call themselves, in the same way that
> amt_send_advertisement() already transmits from this receive path.
>
> With that change:
>  - the skb never waits in a qdisc, so there is no lifetime window;
>  - the tunnel is only dereferenced inside the RCU section that found
>    or created it, and rcu_dereference_bh(amt->sk) in the sender stays
>    covered;
>  - it is O(1): nothing is stored in skb->cb, and there is no lookup
>    and no refcount.
>
> Once the round trip is gone, the query branch in amt_dev_xmit(),
> amt_skb_cb() and struct amt_skb_cb have no users left and can be
> removed.
>
> > +     list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list)
> > +             if (tunnel->ip4 == ip4 && tunnel->source_port == source_port)
> > +                     return tunnel;
>
> This keeps the per-Query walk of tunnel_list that Taehee raised on
> v1. Space isn't the issue: your (ip4, port) key fits in the free cb
> bytes, and so would a u32 tunnel id. The cost is that any key needs a
> lookup at dequeue time, plus a rule for a tunnel that expired or was
> recreated in the meantime, while the sender already has the tunnel in
> hand.
>
> These are the behaviour changes I'm aware of. The first two only
> affect the relay's own GQs; the third affects foreign queries:
>  - They no longer go through a qdisc or taps on the amt device. They
>    are still visible as UDP on the underlay.
>  - They are no longer counted as tx_dropped when sent successfully.
>    Today the query branch in amt_dev_xmit() exits through the unlock
>    label, which counts every sent query as dropped.
>  - A query that reaches amt_dev_xmit() on a relay from somewhere
>    else, such as a userspace querier, is now dropped at the IGMP/MLD
>    type switch. Before, it trusted whatever skb->cb held, and a NULL
>    hit the WARN_ON(1). I found this by reading the code and have not
>    exercised it.
>
> What I have and haven't tested:
>  - The diff below applies to net at 9c572a83037a. It builds with W=1
>    and no warnings (gcc 14, arm64, CONFIG_IPV6=y and =n), and
>    checkpatch --strict is clean. I haven't booted it.
>  - An earlier form of this direct send (without the dead-code removal
>    or the failure accounting) passed amt.sh 5/5 under vng in August,
>    on a v7.1 tree carrying our other pending AMT patches. That was not
>    a KASAN build, and it predates this diff. Our out-of-tree module's
>    source carries the same earlier form.
>  - I haven't reproduced the KASAN report. Could you share the netem
>    setup you used, so I can run it before and after? The GQ no longer
>    enters the qdisc, so I expect it to stop triggering by
>    construction, but I'd want to see that before this goes in.
>
> If you and Taehee like this direction, either of these works for me:
>  - I post it as v3 with your KASAN trace in the commit message,
>    keeping the existing Reported-by tags (you're already one), plus
>    Co-developed-by if you'd like; that needs your Signed-off-by.
>  - You fold it into your v3 with a Suggested-by.
>
> Taehee, would this be acceptable to you?

I agree with this direction. It's clearer and more maintainable.
The current datapath unnecessarily traverses more layers, which makes
the code more complex than it needs to be. Using skb->cb to carry the
tunnel pointer was not a good idea in the first place.

Please send v3 along with a selftest.

Thanks a lot!
Taehee Yoo

>
> Thanks,
> Omar
>
> -- >8 --
> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index bddc24e1856d..b53f8ec55661 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -80,15 +80,6 @@ static struct in6_addr mld2_all_node = MLD2_ALL_NODE_INIT;
>  static struct mld2_grec mldv2_zero_grec;
>  #endif
>
> -static struct amt_skb_cb *amt_skb_cb(struct sk_buff *skb)
> -{
> -       BUILD_BUG_ON(sizeof(struct amt_skb_cb) + sizeof(struct tc_skb_cb) >
> -                    sizeof_field(struct sk_buff, cb));
> -
> -       return (struct amt_skb_cb *)((void *)skb->cb +
> -               sizeof(struct tc_skb_cb));
> -}
> -
>  static void __amt_source_gc_work(void)
>  {
>         struct amt_source_node *snode;
> @@ -791,6 +782,11 @@ static void amt_send_request(struct amt_dev *amt, bool v6)
>         rcu_read_unlock();
>  }
>
> +static bool amt_send_membership_query(struct amt_dev *amt,
> +                                     struct sk_buff *skb,
> +                                     struct amt_tunnel_list *tunnel,
> +                                     bool v6);
> +
>  static void amt_send_igmp_gq(struct amt_dev *amt,
>                              struct amt_tunnel_list *tunnel)
>  {
> @@ -800,8 +796,11 @@ static void amt_send_igmp_gq(struct amt_dev *amt,
>         if (!skb)
>                 return;
>
> -       amt_skb_cb(skb)->tunnel = tunnel;
> -       dev_queue_xmit(skb);
> +       skb_pull(skb, sizeof(struct ethhdr));
> +       if (amt_send_membership_query(amt, skb, tunnel, false)) {
> +               amt->dev->stats.tx_dropped++;
> +               kfree_skb(skb);
> +       }
>  }
>
>  #if IS_ENABLED(CONFIG_IPV6)
> @@ -885,8 +884,11 @@ static void amt_send_mld_gq(struct amt_dev *amt, struct amt_tunnel_list *tunnel)
>         if (!skb)
>                 return;
>
> -       amt_skb_cb(skb)->tunnel = tunnel;
> -       dev_queue_xmit(skb);
> +       skb_pull(skb, sizeof(struct ethhdr));
> +       if (amt_send_membership_query(amt, skb, tunnel, 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)
> @@ -1186,7 +1188,6 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>  #endif
>         bool report = false;
>         struct igmphdr *ih;
> -       bool query = false;
>         struct iphdr *iph;
>         bool data = false;
>         bool v6 = false;
> @@ -1204,9 +1205,6 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>                         case IGMP_HOST_MEMBERSHIP_REPORT:
>                                 report = true;
>                                 break;
> -                       case IGMP_HOST_MEMBERSHIP_QUERY:
> -                               query = true;
> -                               break;
>                         default:
>                                 goto free;
>                         }
> @@ -1228,9 +1226,6 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>                         case ICMPV6_MLD2_REPORT:
>                                 report = true;
>                                 break;
> -                       case ICMPV6_MGM_QUERY:
> -                               query = true;
> -                               break;
>                         default:
>                                 goto free;
>                         }
> @@ -1261,19 +1256,6 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>                         goto free;
>                 goto unlock;
>         } else if (amt->mode == AMT_MODE_RELAY) {
> -               if (query) {
> -                       tunnel = amt_skb_cb(skb)->tunnel;
> -                       if (!tunnel) {
> -                               WARN_ON(1);
> -                               goto free;
> -                       }
> -
> -                       /* Do not forward unexpected query */
> -                       if (amt_send_membership_query(amt, skb, tunnel, v6))
> -                               goto free;
> -                       goto unlock;
> -               }
> -
>                 if (!data)
>                         goto free;
>                 list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list) {
> diff --git a/include/net/amt.h b/include/net/amt.h
> index a0255491f5b0..2846dde0cadc 100644
> --- a/include/net/amt.h
> +++ b/include/net/amt.h
> @@ -231,10 +231,6 @@ struct amt_relay_headers {
>         };
>  } __packed;
>
> -struct amt_skb_cb {
> -       struct amt_tunnel_list *tunnel;
> -};
> -
>  struct amt_tunnel_list {
>         struct list_head        list;
>         /* Protect All resources under an amt_tunne_list */