[PATCH net v4 0/2] amt: fix use-after-free of the skb head across pulls

[PATCH net v4 0/2] amt: fix use-after-free of the skb head across pulls

[Michael Bommarito]

Several AMT receive and transmit paths cache a pointer into the skb head
and then call a helper that can reallocate that head before the cached
pointer is used again, so the later access reads or writes freed memory.

v3 addressed only the source-address reads in a subset of the handlers
and described amt_membership_query_handler() and
amt_multicast_data_handler() as unaffected.  As the review pointed out,
that was incomplete: those handlers keep stale eth_hdr() and AMT-header
pointers across later pulls, the record loops in the IGMPv3 and MLDv2
report handlers read the record count and the group record across the
*_mc_may_pull() calls, and amt_update_handler() and amt_dev_xmit() read
the destination address after further pulls.

Patch 1 walks every AMT path and, for each pointer used after a
reallocating call, either snapshots the value before the first pull or
re-derives the pointer after the last one.  This uses the re-derive
approach rather than the per-value snapshot of v3, because the write
sites cannot be expressed as a snapshot and re-derivation is already the
idiom used elsewhere in the file.

Patch 2 is a smaller, separable hardening change: the three handlers
that rewrite the ethernet header do so in place without making the head
private, which corrupts a cloned skb (for example one held by a packet
tap).  It adds skb_cow_head() before the rewrite, split out so the
use-after-free fix is not held up by discussion of the clone case.

Both patches build cleanly (x86_64, CONFIG_AMT=m, W=1) and are
checkpatch --strict clean.

Changes since v3:
 - Rework from the per-value source-address snapshot to re-deriving the
   header pointers after the last reallocating pull, and cover every
   affected handler (amt_dev_xmit, amt_multicast_data_handler,
   amt_membership_query_handler, the IGMPv3 and MLDv2 record loops, and
   the remaining reads in amt_update_handler), not just the
   source-address reads.
 - Correct the v3 commit-message claim that the query and multicast-data
   handlers were unaffected.
 - Add patch 2 (skb_cow_head() before the L2 rewrite).
 - Drop the v2 Acked-by from Taehee Yoo: this series is materially larger
   than what was acked.

v3: https://lore.kernel.org/all/20260626111917.802243-1-michael.bommarito@gmail.com/
v2: https://lore.kernel.org/all/20260617123443.3586930-1-michael.bommarito@gmail.com/

Michael Bommarito (2):
  amt: re-read skb header pointers after every pull
  amt: make the head writable before rewriting the L2 header

 drivers/net/amt.c | 117 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 96 insertions(+), 21 deletions(-)


base-commit: 5200f5f493f79f14bbdc349e402a40dfb32f23c8
-- 
2.53.0

[PATCH net v4 1/2] amt: re-read skb header pointers after every pull

[Michael Bommarito]

Several AMT receive and transmit paths cache a pointer into the skb
header (ip_hdr(), ipv6_hdr(), eth_hdr() or the AMT message header) and
then call a helper that can reallocate the skb head before the cached
pointer is used again.  pskb_may_pull(), ip_mc_may_pull(),
ipv6_mc_may_pull(), iptunnel_pull_header(), ip_mc_check_igmp() and
ipv6_mc_check_mld() can all free the old head and move the data, so a
pointer taken before the call dangles afterwards and the later access
is a use-after-free of the freed head.

The affected sites are:

  amt_rcv() caches ip_hdr() before amt_parse_type() pulls, then reads
  iph->saddr.

  amt_dev_xmit() caches ip_hdr()/ipv6_hdr() before ip_mc_check_igmp()/
  ipv6_mc_check_mld() and pskb_may_pull(), then reads the group
  address.

  amt_multicast_data_handler() caches eth_hdr() before pskb_may_pull(),
  then writes the L2 header.

  amt_membership_query_handler() caches the AMT header, the outer and
  inner eth_hdr() and ip_hdr() before several pulls, then reads and
  writes them.

  amt_igmpv3_report_handler() and amt_mldv2_report_handler() cache
  ip_hdr()/ipv6_hdr() and the current group record and read the record
  count from the report header inside the record loop, across the
  *_mc_may_pull() calls.

  amt_update_handler() caches ip_hdr() before pskb_may_pull(),
  iptunnel_pull_header(), ip_mc_check_igmp() and the report handler,
  then reads iph->daddr.

Fix each site by either snapshotting the scalar that is used after the
pull before the first pull runs, or re-deriving the header pointer from
the skb after the last pull that can move the head.  Values that are
stable across the pull (source and group address, the response MAC, the
record count, the outer source MAC) are snapshotted; pointers that are
written through or read repeatedly are re-derived.

An earlier fix snapshotted only the source address in a subset of the
handlers and described amt_membership_query_handler() and
amt_multicast_data_handler() as unaffected.  That was wrong: both
re-derive ip_hdr() once but keep stale eth_hdr() and AMT-header
pointers across later pulls, so they are covered here as well.

Fixes: cbc21dc1cfe9 ("amt: add data plane of amt interface")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 drivers/net/amt.c | 100 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 79 insertions(+), 21 deletions(-)

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index f2f3139e38a59..1d6b2de4b73cc 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -1211,7 +1211,7 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 			data = true;
 		}
 		v6 = false;
-		group.ip4 = iph->daddr;
+		group.ip4 = ip_hdr(skb)->daddr;
 #if IS_ENABLED(CONFIG_IPV6)
 	} else if (iph->version == 6) {
 		ip6h = ipv6_hdr(skb);
@@ -1235,7 +1235,7 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 			data = true;
 		}
 		v6 = true;
-		group.ip6 = ip6h->daddr;
+		group.ip6 = ipv6_hdr(skb)->daddr;
 #endif
 	} else {
 		dev->stats.tx_errors++;
@@ -1278,12 +1278,12 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 			hlist_for_each_entry_rcu(gnode, &tunnel->groups[hash],
 						 node) {
 				if (!v6) {
-					if (gnode->group_addr.ip4 == iph->daddr)
+					if (gnode->group_addr.ip4 == group.ip4)
 						goto found;
 #if IS_ENABLED(CONFIG_IPV6)
 				} else {
 					if (ipv6_addr_equal(&gnode->group_addr.ip6,
-							    &ip6h->daddr))
+							    &group.ip6))
 						goto found;
 #endif
 				}
@@ -2000,14 +2000,22 @@ static void amt_igmpv3_report_handler(struct amt_dev *amt, struct sk_buff *skb,
 	struct igmpv3_report *ihrv3 = igmpv3_report_hdr(skb);
 	int len = skb_transport_offset(skb) + sizeof(*ihrv3);
 	void *zero_grec = (void *)&igmpv3_zero_grec;
-	struct iphdr *iph = ip_hdr(skb);
 	struct amt_group_node *gnode;
 	union amt_addr group, host;
 	struct igmpv3_grec *grec;
+	__be32 saddr;
 	u16 nsrcs;
+	u16 ngrec;
 	int i;
 
-	for (i = 0; i < ntohs(ihrv3->ngrec); i++) {
+	/* ihrv3 and ip_hdr() point into the skb head, which the
+	 * ip_mc_may_pull() calls below can reallocate.  Read every field
+	 * that is used after a pull before the first one runs.
+	 */
+	saddr = ip_hdr(skb)->saddr;
+	ngrec = ntohs(ihrv3->ngrec);
+
+	for (i = 0; i < ngrec; i++) {
 		len += sizeof(*grec);
 		if (!ip_mc_may_pull(skb, len))
 			break;
@@ -2019,10 +2027,14 @@ static void amt_igmpv3_report_handler(struct amt_dev *amt, struct sk_buff *skb,
 		if (!ip_mc_may_pull(skb, len))
 			break;
 
+		/* ip_mc_may_pull() may have reallocated the head. */
+		grec = (void *)(skb->data + len - sizeof(*grec) -
+				nsrcs * sizeof(__be32));
+
 		memset(&group, 0, sizeof(union amt_addr));
 		group.ip4 = grec->grec_mca;
 		memset(&host, 0, sizeof(union amt_addr));
-		host.ip4 = iph->saddr;
+		host.ip4 = saddr;
 		gnode = amt_lookup_group(tunnel, &group, &host, false);
 		if (!gnode) {
 			gnode = amt_add_group(amt, tunnel, &group, &host,
@@ -2162,14 +2174,22 @@ static void amt_mldv2_report_handler(struct amt_dev *amt, struct sk_buff *skb,
 	struct mld2_report *mld2r = (struct mld2_report *)icmp6_hdr(skb);
 	int len = skb_transport_offset(skb) + sizeof(*mld2r);
 	void *zero_grec = (void *)&mldv2_zero_grec;
-	struct ipv6hdr *ip6h = ipv6_hdr(skb);
 	struct amt_group_node *gnode;
 	union amt_addr group, host;
 	struct mld2_grec *grec;
+	struct in6_addr saddr;
 	u16 nsrcs;
+	u16 ngrec;
 	int i;
 
-	for (i = 0; i < ntohs(mld2r->mld2r_ngrec); i++) {
+	/* mld2r and ipv6_hdr() point into the skb head, which the
+	 * ipv6_mc_may_pull() calls below can reallocate.  Read every field
+	 * that is used after a pull before the first one runs.
+	 */
+	saddr = ipv6_hdr(skb)->saddr;
+	ngrec = ntohs(mld2r->mld2r_ngrec);
+
+	for (i = 0; i < ngrec; i++) {
 		len += sizeof(*grec);
 		if (!ipv6_mc_may_pull(skb, len))
 			break;
@@ -2181,10 +2201,14 @@ static void amt_mldv2_report_handler(struct amt_dev *amt, struct sk_buff *skb,
 		if (!ipv6_mc_may_pull(skb, len))
 			break;
 
+		/* ipv6_mc_may_pull() may have reallocated the head. */
+		grec = (void *)(skb->data + len - sizeof(*grec) -
+				nsrcs * sizeof(struct in6_addr));
+
 		memset(&group, 0, sizeof(union amt_addr));
 		group.ip6 = grec->grec_mca;
 		memset(&host, 0, sizeof(union amt_addr));
-		host.ip6 = ip6h->saddr;
+		host.ip6 = saddr;
 		gnode = amt_lookup_group(tunnel, &group, &host, true);
 		if (!gnode) {
 			gnode = amt_add_group(amt, tunnel, &group, &host,
@@ -2305,7 +2329,6 @@ static bool amt_multicast_data_handler(struct amt_dev *amt, struct sk_buff *skb)
 	skb_push(skb, sizeof(*eth));
 	skb_reset_mac_header(skb);
 	skb_pull(skb, sizeof(*eth));
-	eth = eth_hdr(skb);
 
 	if (!pskb_may_pull(skb, sizeof(*iph)))
 		return true;
@@ -2315,6 +2338,8 @@ static bool amt_multicast_data_handler(struct amt_dev *amt, struct sk_buff *skb)
 		if (!ipv4_is_multicast(iph->daddr))
 			return true;
 		skb->protocol = htons(ETH_P_IP);
+		/* pskb_may_pull() above may have reallocated the head. */
+		eth = eth_hdr(skb);
 		eth->h_proto = htons(ETH_P_IP);
 		ip_eth_mc_map(iph->daddr, eth->h_dest);
 #if IS_ENABLED(CONFIG_IPV6)
@@ -2328,6 +2353,8 @@ static bool amt_multicast_data_handler(struct amt_dev *amt, struct sk_buff *skb)
 		if (!ipv6_addr_is_multicast(&ip6h->daddr))
 			return true;
 		skb->protocol = htons(ETH_P_IPV6);
+		/* pskb_may_pull() above may have reallocated the head. */
+		eth = eth_hdr(skb);
 		eth->h_proto = htons(ETH_P_IPV6);
 		ipv6_eth_mc_map(&ip6h->daddr, eth->h_dest);
 #endif
@@ -2353,8 +2380,10 @@ static bool amt_membership_query_handler(struct amt_dev *amt,
 	struct amt_header_membership_query *amtmq;
 	struct igmpv3_query *ihv3;
 	struct ethhdr *eth, *oeth;
+	u8 h_source[ETH_ALEN];
 	struct iphdr *iph;
 	int hdr_size, len;
+	u64 response_mac;
 
 	hdr_size = sizeof(*amtmq) + sizeof(struct udphdr);
 	if (!pskb_may_pull(skb, hdr_size))
@@ -2367,6 +2396,12 @@ static bool amt_membership_query_handler(struct amt_dev *amt,
 	if (amtmq->nonce != amt->nonce)
 		return true;
 
+	/* iptunnel_pull_header() and the pskb_may_pull() calls below can
+	 * reallocate the skb head, so amtmq would dangle.  Snapshot the only
+	 * field used afterwards.
+	 */
+	response_mac = amtmq->response_mac;
+
 	hdr_size -= sizeof(*eth);
 	if (iptunnel_pull_header(skb, hdr_size, htons(ETH_P_TEB), false))
 		return true;
@@ -2376,6 +2411,8 @@ static bool amt_membership_query_handler(struct amt_dev *amt,
 	skb_pull(skb, sizeof(*eth));
 	skb_reset_network_header(skb);
 	eth = eth_hdr(skb);
+	/* Snapshot the outer source MAC before a later pull can free it. */
+	ether_addr_copy(h_source, oeth->h_source);
 	if (!pskb_may_pull(skb, sizeof(*iph)))
 		return true;
 
@@ -2388,6 +2425,8 @@ static bool amt_membership_query_handler(struct amt_dev *amt,
 				   sizeof(*ihv3)))
 			return true;
 
+		/* pskb_may_pull() above may have reallocated the head. */
+		iph = ip_hdr(skb);
 		if (!ipv4_is_multicast(iph->daddr))
 			return true;
 
@@ -2395,10 +2434,11 @@ static bool amt_membership_query_handler(struct amt_dev *amt,
 		skb_reset_transport_header(skb);
 		skb_push(skb, sizeof(*iph) + AMT_IPHDR_OPTS);
 		WRITE_ONCE(amt->ready4, true);
-		amt->mac = amtmq->response_mac;
+		amt->mac = response_mac;
 		amt->req_cnt = 0;
 		amt->qi = ihv3->qqic;
 		skb->protocol = htons(ETH_P_IP);
+		eth = eth_hdr(skb);
 		eth->h_proto = htons(ETH_P_IP);
 		ip_eth_mc_map(iph->daddr, eth->h_dest);
 #if IS_ENABLED(CONFIG_IPV6)
@@ -2421,10 +2461,11 @@ static bool amt_membership_query_handler(struct amt_dev *amt,
 		skb_reset_transport_header(skb);
 		skb_push(skb, sizeof(*ip6h) + AMT_IP6HDR_OPTS);
 		WRITE_ONCE(amt->ready6, true);
-		amt->mac = amtmq->response_mac;
+		amt->mac = response_mac;
 		amt->req_cnt = 0;
 		amt->qi = mld2q->mld2q_qqic;
 		skb->protocol = htons(ETH_P_IPV6);
+		eth = eth_hdr(skb);
 		eth->h_proto = htons(ETH_P_IPV6);
 		ipv6_eth_mc_map(&ip6h->daddr, eth->h_dest);
 #endif
@@ -2432,7 +2473,7 @@ static bool amt_membership_query_handler(struct amt_dev *amt,
 		return true;
 	}
 
-	ether_addr_copy(eth->h_source, oeth->h_source);
+	ether_addr_copy(eth->h_source, h_source);
 	skb->pkt_type = PACKET_MULTICAST;
 	skb->ip_summed = CHECKSUM_NONE;
 	len = skb->len;
@@ -2455,8 +2496,13 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
 	struct ethhdr *eth;
 	struct iphdr *iph;
 	int len, hdr_size;
+	__be32 saddr;
 
-	iph = ip_hdr(skb);
+	/* ip_hdr() points into the skb head, which the pskb_may_pull() /
+	 * iptunnel_pull_header() calls below can reallocate.  Snapshot the
+	 * source address used for the tunnel lookup before the first pull.
+	 */
+	saddr = ip_hdr(skb)->saddr;
 
 	hdr_size = sizeof(*amtmu) + sizeof(struct udphdr);
 	if (!pskb_may_pull(skb, hdr_size))
@@ -2472,7 +2518,7 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
 	skb_reset_network_header(skb);
 
 	list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list) {
-		if (tunnel->ip4 == iph->saddr) {
+		if (tunnel->ip4 == saddr) {
 			if ((amtmu->nonce == tunnel->nonce &&
 			     amtmu->response_mac == tunnel->mac)) {
 				mod_delayed_work(amt_wq, &tunnel->gc_wq,
@@ -2508,6 +2554,10 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
 		eth = eth_hdr(skb);
 		skb->protocol = htons(ETH_P_IP);
 		eth->h_proto = htons(ETH_P_IP);
+		/* ip_mc_check_igmp() and the report handler may have
+		 * reallocated the head; re-derive iph.
+		 */
+		iph = ip_hdr(skb);
 		ip_eth_mc_map(iph->daddr, eth->h_dest);
 #if IS_ENABLED(CONFIG_IPV6)
 	} else if (iph->version == 6) {
@@ -2527,6 +2577,10 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
 		eth = eth_hdr(skb);
 		skb->protocol = htons(ETH_P_IPV6);
 		eth->h_proto = htons(ETH_P_IPV6);
+		/* ipv6_mc_check_mld() and the report handler may have
+		 * reallocated the head; re-derive ip6h.
+		 */
+		ip6h = ipv6_hdr(skb);
 		ipv6_eth_mc_map(&ip6h->daddr, eth->h_dest);
 #endif
 	} else {
@@ -2772,7 +2826,7 @@ static void amt_gw_rcv(struct amt_dev *amt, struct sk_buff *skb)
 static int amt_rcv(struct sock *sk, struct sk_buff *skb)
 {
 	struct amt_dev *amt;
-	struct iphdr *iph;
+	__be32 saddr;
 	int type;
 	bool err;
 
@@ -2785,7 +2839,11 @@ static int amt_rcv(struct sock *sk, struct sk_buff *skb)
 	}
 
 	skb->dev = amt->dev;
-	iph = ip_hdr(skb);
+	/* amt_parse_type() calls pskb_may_pull(), which can reallocate the
+	 * skb head and leave a cached ip_hdr() dangling.  Snapshot the source
+	 * address used below before the pull.
+	 */
+	saddr = ip_hdr(skb)->saddr;
 	type = amt_parse_type(skb);
 	if (type == -1) {
 		err = true;
@@ -2795,7 +2853,7 @@ static int amt_rcv(struct sock *sk, struct sk_buff *skb)
 	if (amt->mode == AMT_MODE_GATEWAY) {
 		switch (type) {
 		case AMT_MSG_ADVERTISEMENT:
-			if (iph->saddr != amt->discovery_ip) {
+			if (saddr != amt->discovery_ip) {
 				netdev_dbg(amt->dev, "Invalid Relay IP\n");
 				err = true;
 				goto drop;
@@ -2807,7 +2865,7 @@ static int amt_rcv(struct sock *sk, struct sk_buff *skb)
 			}
 			goto out;
 		case AMT_MSG_MULTICAST_DATA:
-			if (iph->saddr != amt->remote_ip) {
+			if (saddr != amt->remote_ip) {
 				netdev_dbg(amt->dev, "Invalid Relay IP\n");
 				err = true;
 				goto drop;
@@ -2818,7 +2876,7 @@ static int amt_rcv(struct sock *sk, struct sk_buff *skb)
 			else
 				goto out;
 		case AMT_MSG_MEMBERSHIP_QUERY:
-			if (iph->saddr != amt->remote_ip) {
+			if (saddr != amt->remote_ip) {
 				netdev_dbg(amt->dev, "Invalid Relay IP\n");
 				err = true;
 				goto drop;
-- 
2.53.0

[PATCH net v4 2/2] amt: make the head writable before rewriting the L2 header

[Michael Bommarito]

amt_multicast_data_handler(), amt_membership_query_handler() and
amt_update_handler() rewrite the ethernet header of the decapsulated
skb in place (eth->h_proto, eth->h_dest and, for the query, also
eth->h_source) before handing it up the stack.  The skb head may be
shared, for example when a packet tap has cloned it on the underlay
interface, so writing through it corrupts the other reader's copy.

Call skb_cow_head() before the rewrite so the head is private.  It is
placed before the pointers into the head are (re-)derived, so a
reallocation caused by the copy is picked up by those derivations.

Fixes: cbc21dc1cfe9 ("amt: add data plane of amt interface")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 drivers/net/amt.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index 1d6b2de4b73cc..f7375e40bc8f2 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -2330,6 +2330,12 @@ static bool amt_multicast_data_handler(struct amt_dev *amt, struct sk_buff *skb)
 	skb_reset_mac_header(skb);
 	skb_pull(skb, sizeof(*eth));
 
+	/* The L2 header is rewritten below; make the head private first so a
+	 * cloned skb (for example one taken by a packet tap) is not modified.
+	 */
+	if (skb_cow_head(skb, 0))
+		return true;
+
 	if (!pskb_may_pull(skb, sizeof(*iph)))
 		return true;
 	iph = ip_hdr(skb);
@@ -2413,6 +2419,11 @@ static bool amt_membership_query_handler(struct amt_dev *amt,
 	eth = eth_hdr(skb);
 	/* Snapshot the outer source MAC before a later pull can free it. */
 	ether_addr_copy(h_source, oeth->h_source);
+	/* The L2 header is rewritten below; make the head private first so a
+	 * cloned skb (for example one taken by a packet tap) is not modified.
+	 */
+	if (skb_cow_head(skb, 0))
+		return true;
 	if (!pskb_may_pull(skb, sizeof(*iph)))
 		return true;
 
@@ -2538,6 +2549,12 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
 	if (!pskb_may_pull(skb, sizeof(*iph)))
 		return true;
 
+	/* The L2 header is rewritten below; make the head private first so a
+	 * cloned skb (for example one taken by a packet tap) is not modified.
+	 */
+	if (skb_cow_head(skb, 0))
+		return true;
+
 	iph = ip_hdr(skb);
 	if (iph->version == 4) {
 		if (ip_mc_check_igmp(skb)) {
-- 
2.53.0