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

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

[Cen Zhang (Microsoft)]

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.

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) <blbllhy@gmail.com>
Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
---
 drivers/net/amt.c | 30 +++++++++++++++++++++++++-----
 include/net/amt.h |  4 +++-
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index 182a41d59a75..a85ba0dfe18a 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -789,6 +789,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)
 {
@@ -798,7 +810,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);
 }
 
@@ -883,7 +896,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
@@ -1259,15 +1273,21 @@ 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;
+			rcu_read_lock();
+			tunnel = amt_lookup_tunnel(amt,
+						   amt_skb_cb(skb)->tunnel_ip4,
+						   amt_skb_cb(skb)->tunnel_port);
 			if (!tunnel) {
-				WARN_ON(1);
+				rcu_read_unlock();
 				goto free;
 			}
 
 			/* Do not forward unexpected query */
-			if (amt_send_membership_query(amt, skb, tunnel, v6))
+			if (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 a0255491f5b0..59c4bb88fb1e 100644
--- a/include/net/amt.h
+++ b/include/net/amt.h
@@ -231,8 +231,10 @@ struct amt_relay_headers {
 	};
 } __packed;
 
+/* Tunnel identity for re-lookup; do not store a pointer here. */
 struct amt_skb_cb {
-	struct amt_tunnel_list *tunnel;
+	__be32			tunnel_ip4;
+	__be16			tunnel_port;
 };
 
 struct amt_tunnel_list {
-- 
2.52.0

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

[Taehee Yoo]

On Wed, Aug 19, 2026 at 1:48 AM Cen Zhang (Microsoft) <blbllhy@gmail.com> wrote:
>

Hi Cen Zhang,
Thanks a lot for this work!

> 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.
>
> 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) <blbllhy@gmail.com>
> Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
> ---
>  drivers/net/amt.c | 30 +++++++++++++++++++++++++-----
>  include/net/amt.h |  4 +++-
>  2 files changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 182a41d59a75..a85ba0dfe18a 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -789,6 +789,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;
> +}

This looks like it adds a per-packet linear scan over all tunnels,
so I'm concerned about performance when there are many tunnels.
Do you have any idea how to avoid this regression?

> +
>  static void amt_send_igmp_gq(struct amt_dev *amt,
>                              struct amt_tunnel_list *tunnel)
>  {
> @@ -798,7 +810,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);
>  }
>
> @@ -883,7 +896,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
> @@ -1259,15 +1273,21 @@ 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;
> +                       rcu_read_lock();
> +                       tunnel = amt_lookup_tunnel(amt,
> +                                                  amt_skb_cb(skb)->tunnel_ip4,
> +                                                  amt_skb_cb(skb)->tunnel_port);
>                         if (!tunnel) {
> -                               WARN_ON(1);
> +                               rcu_read_unlock();
>                                 goto free;
>                         }
>
>                         /* Do not forward unexpected query */
> -                       if (amt_send_membership_query(amt, skb, tunnel, v6))
> +                       if (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 a0255491f5b0..59c4bb88fb1e 100644
> --- a/include/net/amt.h
> +++ b/include/net/amt.h
> @@ -231,8 +231,10 @@ struct amt_relay_headers {
>         };
>  } __packed;
>
> +/* Tunnel identity for re-lookup; do not store a pointer here. */

I think this comment is not necessary. Please remove it.

>  struct amt_skb_cb {
> -       struct amt_tunnel_list *tunnel;
> +       __be32                  tunnel_ip4;
> +       __be16                  tunnel_port;
>  };
>
>  struct amt_tunnel_list {
> --
> 2.52.0
>

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

[Cen Zhang (Microsoft)]

Thanks for the feedback. Just getting back to this thread.

On Thu, Aug 20, 2026 at 10:18:43PM +0900, Taehee Yoo wrote:
> This looks like it adds a per-packet linear scan over all tunnels,
> so I'm concerned about performance when there are many tunnels.
> Do you have any idea how to avoid this regression?

I see two possible approaches:

1. Keep the v1 implementation. It is the simplest fix, but changes the
   Query dequeue path from an O(1) pointer access to an O(n) lookup. I am
   not sure whether that cost is significant in practice for two reasons:
   this handles IGMP/MLD Query control packets rather than multicast data
   packets [1], while the hotter multicast data path already walks
   tunnel_list for each skb [2]; and max_tunnels defaults to 128. However,
   max_tunnels is configurable, so users can set it much higher.

2. Add a small per-device bucket hash keyed by (ip4, source_port) to [1],
   while keeping tunnel_list for multicast fan-out in [2]. This avoids
   a new linear lookup cost, but adds a second index that must be
   maintained during tunnel creation, expiry, and device teardown.
   I am not sure whether that is overkill for this fix.

I would appreciate your further feedback here and will prepare v2 after
settling on the approach.

> I think this comment is not necessary. Please remove it.

Agreed. I will remove it in v2.

[1] https://github.com/torvalds/linux/blob/0f23d56f17fdfc7db69d51f64c8b91bbab947aa9/drivers/net/amt.c#L1261
[2] https://github.com/torvalds/linux/blob/0f23d56f17fdfc7db69d51f64c8b91bbab947aa9/drivers/net/amt.c#L1276

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

[Taehee Yoo]

On Wed, Aug 26, 2026 at 1:15 PM Cen Zhang (Microsoft) <blbllhy@gmail.com> wrote:
>
> Thanks for the feedback. Just getting back to this thread.
>
> On Thu, Aug 20, 2026 at 10:18:43PM +0900, Taehee Yoo wrote:
> > This looks like it adds a per-packet linear scan over all tunnels,
> > so I'm concerned about performance when there are many tunnels.
> > Do you have any idea how to avoid this regression?
>
> I see two possible approaches:
>
> 1. Keep the v1 implementation. It is the simplest fix, but changes the
>    Query dequeue path from an O(1) pointer access to an O(n) lookup. I am
>    not sure whether that cost is significant in practice for two reasons:
>    this handles IGMP/MLD Query control packets rather than multicast data
>    packets [1], while the hotter multicast data path already walks
>    tunnel_list for each skb [2]; and max_tunnels defaults to 128. However,
>    max_tunnels is configurable, so users can set it much higher.
>
> 2. Add a small per-device bucket hash keyed by (ip4, source_port) to [1],
>    while keeping tunnel_list for multicast fan-out in [2]. This avoids
>    a new linear lookup cost, but adds a second index that must be
>    maintained during tunnel creation, expiry, and device teardown.
>    I am not sure whether that is overkill for this fix.
>
> I would appreciate your further feedback here and will prepare v2 after
> settling on the approach.

Thanks for the investigation.

How about protecting amt_tunnel_list with a refcount?
Currently amt_tunnel_expire() frees the tunnel immediately, but with a
refcount we could just drop the reference there and let the memory be
freed once it reaches 0.

What do you think?

>
> > I think this comment is not necessary. Please remove it.
>
> Agreed. I will remove it in v2.
>
> [1] https://github.com/torvalds/linux/blob/0f23d56f17fdfc7db69d51f64c8b91bbab947aa9/drivers/net/amt.c#L1261
> [2] https://github.com/torvalds/linux/blob/0f23d56f17fdfc7db69d51f64c8b91bbab947aa9/drivers/net/amt.c#L1276

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

[Cen Zhang (Microsoft Security FORGE Labs)]

On Wed, Aug 26, 2026 at 09:56:35PM +0900, Taehee Yoo wrote:
> How about protecting amt_tunnel_list with a refcount?
> Currently amt_tunnel_expire() frees the tunnel immediately, but with a
> refcount we could just drop the reference there and let the memory be
> freed once it reaches 0.
>
> What do you think?

Sorry for the late reply, I was busy with something else and am back
on this now.

I checked this for a while and I believe we cannot keep a refcount
balanced from skb->cb: once the skb enters the qdisc/tc layer it can
be dropped, cloned or orphaned before it reaches amt_dev_xmit(),
depending on the user's qdisc/tc commands. So a put inside
amt_dev_xmit() cannot be balanced against the get taken before
queueing, and the tunnel would either leak or be freed early.

I will send a v2 with the comment removed, from my corporate address
cenzhang@linux.microsoft.com.

Thanks,
Cen