An skb queued in a qdisc can outlive the tunnel it references
through a raw pointer in skb->cb. For example, with igmp_qrv set
to 1 on the relay a tunnel lives for 135s, so a netem delay of
180s on the amt device outlives it; 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
amt_send_igmp_gq() and amt_send_mld_gq() are only called from
amt_request_handler(), inside the rcu_read_lock_bh() section of
amt_rcv(). amt_request_handler() already has the tunnel the query is
for: it found or created it inside that section. Queuing the query with
dev_queue_xmit() only leads back into amt_dev_xmit(), which strips the
Ethernet header and calls amt_send_membership_query() for that tunnel.
Make that call directly from the two senders instead, the same way
amt_send_advertisement() transmits from the receive path. The query
never waits in a qdisc, the tunnel is only dereferenced inside the
RCU section that found or created it, and nothing is stored in
skb->cb, so no lookup or refcount is needed. Remove the query branch
of amt_dev_xmit(), amt_skb_cb() and struct amt_skb_cb, which have no
users left.
Behaviour changes:
- The relay's own General Queries no longer pass through the amt
device's egress path: its qdisc, tc egress (clsact/tcx), the
netfilter egress hook and packet taps. They are still visible as
UDP on the underlay.
- A query that is sent successfully is no longer counted as
tx_dropped. The old query branch left through the unlock label,
which counted every sent query as dropped.
- A query that reaches amt_dev_xmit() on a relay from elsewhere,
such as a userspace querier, is now dropped at the IGMP/MLD type
switch. Before, it trusted whatever skb->cb held, and a NULL
tunnel hit the WARN_ON(1).
Fixes: cbc21dc1cfe9 ("amt: add data plane of amt interface")
Reported-by: [email protected]
Reported-by: Xiang Mei (Microsoft) <[email protected]>
Reported-by: Cen Zhang (Microsoft Security FORGE Labs)
<[email protected]>
Signed-off-by: Omar Ramadan <[email protected]>
---
v4: no code change. Add a selftest (patch 2), as Taehee asked in the v2
thread:
https://lore.kernel.org/netdev/camarctxsu+ybuzjf8bolsvjl2l_quasv54mdm8iijn5qdoa...@mail.gmail.com/
v3: https://lore.kernel.org/netdev/[email protected]/
v3 (Omar): send the GQ directly from amt_request_handler()'s RCU
section, instead of storing (ip4, source_port) in skb->cb and looking
the tunnel up again at dequeue. This also removes the per-query
tunnel_list walk that Taehee raised on v1, and the v2 window Sashiko
found in which a re-created tunnel could be matched before its nonce
and mac were written. Cen agreed in the v2 thread to go this way if
Taehee is fine with it.
v2:
https://lore.kernel.org/netdev/[email protected]/
v1: https://lore.kernel.org/netdev/[email protected]/
Testing: Cen's KASAN reproducer, in which netem holds the General Query
for 160s past a 135s tunnel lifetime, reports the slab-use-after-free
in amt_dev_xmit() on net, and nothing with this patch or with v2
applied. tools/testing/selftests/net/amt.sh passes 5/5 with and without
this patch on a KASAN + lockdep kernel.
drivers/net/amt.c | 48 +++++++++++++++--------------------------------
include/net/amt.h | 4 ----
2 files changed, 15 insertions(+), 37 deletions(-)
diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index bddc24e18..b53f8ec55 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 a0255491f..2846dde0c 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 */
--
2.47.3
