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

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) <redacted>
Reported-by: Cen Zhang (Microsoft) <redacted>
Signed-off-by: Cen Zhang (Microsoft) <redacted>
---
 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. */