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@xxxxxxxxx> 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@xxxxxxxxxxxxx
> Reported-by: Xiang Mei (Microsoft) <xmei5@xxxxxxx>
> Reported-by: Cen Zhang (Microsoft) <blbllhy@xxxxxxxxx>
> Signed-off-by: Cen Zhang (Microsoft) <blbllhy@xxxxxxxxx>
> ---
>  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
>