[PATCH net v2] amt: don't read the IP source address from a reallocated skb header

[PATCH net v2] amt: don't read the IP source address from a reallocated skb header

[Michael Bommarito]

amt_update_handler() caches iph = ip_hdr(skb) and then calls
pskb_may_pull(). pskb_may_pull() can reallocate the skb head: the new
head is allocated and the old one is freed. The cached iph is not
refreshed, so the following tunnel lookup reads iph->saddr from the
freed head. On an AMT relay this lookup runs for every incoming
membership update, before the update's nonce and response MAC are
validated.

The sibling handlers amt_multicast_data_handler() and
amt_membership_query_handler() re-read ip_hdr() after the pull and are
not affected; only amt_update_handler() keeps the pre-pull pointer.

Snapshot the source address before the pulls and match against the
snapshot.

The stale read was confirmed by instrumentation rather than a sanitizer:
after the head is reallocated the comparison reads from the freed old
head. KASAN does not flag it because the skb head is released through
the page-fragment free path, which is not poisoned on free.

Fixes: cbc21dc1cfe9 ("amt: add data plane of amt interface")
Acked-by: Taehee Yoo <ap420073@gmail.com>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
v2: per Taehee Yoo's review
    (https://lore.kernel.org/all/CAMArcTWCg4x1bxrzr+XHc_FqbzJELCMu+tE=x8Jhewgr-_A3Rw@mail.gmail.com/):
    - retag the subject as [PATCH net] (this is a bug fix);
    - drop Cc: stable -- the Fixes tag is enough for the stable backport
      process to pick it up;
    - carry Taehee Yoo's Acked-by.
    No code change from v1.
    v1: https://lore.kernel.org/all/20260614155539.3106537-1-michael.bommarito@gmail.com/

Confirmed on x86_64 by instrumenting the comparison: with the update
packet built so the first pskb_may_pull() reallocates the head (it pulls
bytes out of a page fragment with no tailroom), the read runs against
the freed old head -- the head pointer moves and the old page's refcount
is 0. Neither generic KASAN nor arm64 HW-tag KASAN reports it: page-
fragment frees are not synchronously poisoned, and under MTE the freed
page keeps a tag matching the stale pointer, so this class of stale-
header read escapes the usual fuzzing oracles. On a live relay the freed
head is also exposed to reuse by later skb allocations.

  amtdbg: cmp reads iph=...e000 (skb->head=...384380) stale_head=1 ref=0

A KUnit covering the re-read can follow separately.

 drivers/net/amt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index f2f3139..af6e28d 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -2455,8 +2455,10 @@ 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);
+	saddr = iph->saddr;
 
 	hdr_size = sizeof(*amtmu) + sizeof(struct udphdr);
 	if (!pskb_may_pull(skb, hdr_size))
@@ -2472,7 +2474,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,
base-commit: 5200f5f493f79f14bbdc349e402a40dfb32f23c8
-- 
2.53.0

Re: [PATCH net v2] amt: don't read the IP source address from a reallocated skb header

[Jakub Kicinski]

On Wed, 17 Jun 2026 08:34:43 -0400 Michael Bommarito wrote:
> amt_update_handler() caches iph = ip_hdr(skb) and then calls
> pskb_may_pull(). pskb_may_pull() can reallocate the skb head: the new
> head is allocated and the old one is freed. The cached iph is not
> refreshed, so the following tunnel lookup reads iph->saddr from the
> freed head. On an AMT relay this lookup runs for every incoming
> membership update, before the update's nonce and response MAC are
> validated.
> 
> The sibling handlers amt_multicast_data_handler() and
> amt_membership_query_handler() re-read ip_hdr() after the pull and are
> not affected; only amt_update_handler() keeps the pre-pull pointer.

Sashikos point out a bunch more of these in AMT:
https://sashiko.dev/#/patchset/20260617123443.3586930-1-michael.bommarito@gmail.com
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260617123443.3586930-1-michael.bommarito@gmail.com

Let's fix them all with one patch?
-- 
pw-bot: cr

Re: [PATCH net v2] amt: don't read the IP source address from a reallocated skb header

[Taehee Yoo]

On Mon, Jun 22, 2026 at 7:00 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 17 Jun 2026 08:34:43 -0400 Michael Bommarito wrote:
> > amt_update_handler() caches iph = ip_hdr(skb) and then calls
> > pskb_may_pull(). pskb_may_pull() can reallocate the skb head: the new
> > head is allocated and the old one is freed. The cached iph is not
> > refreshed, so the following tunnel lookup reads iph->saddr from the
> > freed head. On an AMT relay this lookup runs for every incoming
> > membership update, before the update's nonce and response MAC are
> > validated.
> >
> > The sibling handlers amt_multicast_data_handler() and
> > amt_membership_query_handler() re-read ip_hdr() after the pull and are
> > not affected; only amt_update_handler() keeps the pre-pull pointer.
>
> Sashikos point out a bunch more of these in AMT:
> https://sashiko.dev/#/patchset/20260617123443.3586930-1-michael.bommarito@gmail.com
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260617123443.3586930-1-michael.bommarito@gmail.com
>
> Let's fix them all with one patch?

Agreed.
Michael, could you please fix the remaining ones Sashiko flagged?

Thanks a lot!
Taehee Yoo

> --
> pw-bot: cr

Re: [PATCH net v2] amt: don't read the IP source address from a reallocated skb header

[Michael Bommarito]

On Mon, Jun 22, 2026 at 4:58 AM Taehee Yoo <ap420073@gmail.com> wrote:
> > Let's fix them all with one patch?
>
> Agreed.
> Michael, could you please fix the remaining ones Sashiko flagged?

Sure, will do

Thanks,
Mike

[PATCH net v3] amt: don't read the IP header from a reallocated skb head

[Michael Bommarito]

Several AMT receive paths cache a pointer into the skb header
(ip_hdr() / ipv6_hdr()) and then call a helper that can reallocate
the skb head before reading the cached pointer:

  amt_rcv()                 caches ip_hdr(skb), then amt_parse_type()
                            does pskb_may_pull() before reading
                            iph->saddr;
  amt_update_handler()      caches ip_hdr(skb), then pskb_may_pull() /
                            iptunnel_pull_header() before reading
                            iph->saddr;
  amt_igmpv3_report_handler() caches ip_hdr(skb), then ip_mc_may_pull()
                            in the record loop before reading
                            iph->saddr;
  amt_mldv2_report_handler() caches ipv6_hdr(skb), then
                            ipv6_mc_may_pull() in the record loop
                            before reading ip6h->saddr.

pskb_may_pull() and the *_mc_may_pull() helpers can reallocate the
skb head; when they do, the old head is freed and the cached pointer
dangles, so the later source-address read is a use-after-free of the
freed head.

The only field used after the pull in each case is the source
address, which is stable across the pull. Snapshot it before the
pull and use the snapshot.

The sibling handlers that re-derive ip_hdr() after the pull
(amt_multicast_data_handler(), amt_membership_query_handler()) and
the handlers that read the source address with no intervening pull
(amt_discovery_handler(), amt_request_handler(), the IGMPv2/MLDv1
report and leave handlers) are not affected.

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>
---
v3: address Jakub Kicinski's and Taehee Yoo's review of v2: fix all of
    the stale-iph reads Sashiko flagged in amt.c in one patch, not just
    amt_update_handler(). v2 fixed only amt_update_handler(); this also
    covers amt_rcv(), amt_igmpv3_report_handler() and
    amt_mldv2_report_handler(), which have the same pre-pull cached
    header read. The amt_update_handler() change is functionally the
    same as v2 (snapshot the source address before the pull).
    v2: https://lore.kernel.org/all/20260617123443.3586930-1-michael.bommarito@gmail.com/

Built for x86_64 (CONFIG_AMT=m) with W=1, no new warnings; checkpatch
--strict clean.

 drivers/net/amt.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index 724a8163a5142..7094bdab0f463 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -2000,13 +2000,15 @@ 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;
 	int i;
 
+	saddr = ip_hdr(skb)->saddr;
+
 	for (i = 0; i < ntohs(ihrv3->ngrec); i++) {
 		len += sizeof(*grec);
 		if (!ip_mc_may_pull(skb, len))
@@ -2022,7 +2024,7 @@ static void amt_igmpv3_report_handler(struct amt_dev *amt, struct sk_buff *skb,
 		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,13 +2164,15 @@ 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;
 	int i;
 
+	saddr = ipv6_hdr(skb)->saddr;
+
 	for (i = 0; i < ntohs(mld2r->mld2r_ngrec); i++) {
 		len += sizeof(*grec);
 		if (!ipv6_mc_may_pull(skb, len))
@@ -2184,7 +2188,7 @@ static void amt_mldv2_report_handler(struct amt_dev *amt, struct sk_buff *skb,
 		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,
@@ -2455,8 +2459,10 @@ 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);
+	saddr = iph->saddr;
 
 	hdr_size = sizeof(*amtmu) + sizeof(struct udphdr);
 	if (!pskb_may_pull(skb, hdr_size))
@@ -2472,7 +2478,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,
@@ -2772,7 +2778,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 +2791,7 @@ static int amt_rcv(struct sock *sk, struct sk_buff *skb)
 	}
 
 	skb->dev = amt->dev;
-	iph = ip_hdr(skb);
+	saddr = ip_hdr(skb)->saddr;
 	type = amt_parse_type(skb);
 	if (type == -1) {
 		err = true;
@@ -2795,7 +2801,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 +2813,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 +2824,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;

base-commit: ab9de95c9cf952332ab79453b4b5d1bfca8e514f
-- 
2.53.0

Re: [PATCH net v3] amt: don't read the IP header from a reallocated skb head

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
says: "Sashiko points out more, let's fix them all cause the approach
of making a copy is fairy unusual, IDK if it will work for all headers."
---
amt: don't read the IP header from a reallocated skb head

This commit fixes several use-after-free bugs in AMT receive paths by
caching the source address before calling pskb_may_pull() or similar
helpers. It notes that pskb_may_pull() can reallocate the skb head,
making cached pointers like ip_hdr() dangling.

> The sibling handlers that re-derive ip_hdr() after the pull
> (amt_multicast_data_handler(), amt_membership_query_handler()) and
> the handlers that read the source address with no intervening pull
> (amt_discovery_handler(), amt_request_handler(), the IGMPv2/MLDv1
> report and leave handlers) are not affected.

[Severity: Critical]
This isn't a bug introduced by this patch, but the commit message states
that amt_multicast_data_handler() and amt_membership_query_handler() are
not affected.

Looking at amt_multicast_data_handler(), while ip_hdr() is re-derived,
the eth pointer is cached before calling pskb_may_pull():

amt_multicast_data_handler() {
    ...
    eth = eth_hdr(skb);

    if (!pskb_may_pull(skb, sizeof(*iph)))
        return true;

    iph = ip_hdr(skb);
    if (iph->version == 4) {
        ...
        eth->h_proto = htons(ETH_P_IP);
        ip_eth_mc_map(iph->daddr, eth->h_dest);
    }
}

Does this result in a use-after-free write if pskb_may_pull() reallocates
the skb head and frees the old buffer?

[Severity: Critical]
This isn't a bug introduced by this patch, but a similar issue exists in
amt_membership_query_handler(). The pointers amtmq, oeth, eth, and iph
are cached before calling pskb_may_pull().

If pskb_may_pull() reallocates the skb head, do these pointers become
dangling, causing use-after-free reads and writes when the function later
accesses amtmq->response_mac, iph->daddr, and modifies eth->h_proto?

[Severity: High]
This isn't a bug introduced by this patch, but in amt_multicast_data_handler(),
the code directly modifies the skb headroom by writing to eth->h_proto and
eth->h_dest.

Since the ethernet header overlays the pulled outer headers, this writes
directly into the skb's underlying data buffer. If the received skb is cloned
(for example, via tcpdump), does this corrupt the packet data seen by other
consumers since skb_unshare() or skb_cow_head() is not called?

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 724a8163a5142..7094bdab0f463 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -2000,13 +2000,15 @@ 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 amt_group_node *gnode;
>  	union amt_addr group, host;
>  	struct igmpv3_grec *grec;
> +	__be32 saddr;
>  	u16 nsrcs;
>  	int i;
>  
> +	saddr = ip_hdr(skb)->saddr;
> +
>  	for (i = 0; i < ntohs(ihrv3->ngrec); i++) {
>  		len += sizeof(*grec);
>  		if (!ip_mc_may_pull(skb, len))

[Severity: Critical]
This isn't a bug introduced by this patch, but since ip_mc_may_pull() can
reallocate the skb head, does evaluating ihrv3->ngrec in the loop condition
result in a use-after-free on the next iteration?

Additionally, grec is assigned inside the loop, followed immediately by
another ip_mc_may_pull() call. If this second pull reallocates the head,
does grec become dangling before grec->grec_mca and grec->grec_type are
accessed?

[ ... ]

> @@ -2162,13 +2164,15 @@ 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 amt_group_node *gnode;
>  	union amt_addr group, host;
>  	struct mld2_grec *grec;
> +	struct in6_addr saddr;
>  	u16 nsrcs;
>  	int i;
>  
> +	saddr = ipv6_hdr(skb)->saddr;
> +
>  	for (i = 0; i < ntohs(mld2r->mld2r_ngrec); i++) {
>  		len += sizeof(*grec);
>  		if (!ipv6_mc_may_pull(skb, len))

[Severity: Critical]
This isn't a bug introduced by this patch, but similarly, does evaluating
mld2r->mld2r_ngrec in this loop condition cause a use-after-free if
ipv6_mc_may_pull() reallocates the skb head?

[ ... ]

> @@ -2455,8 +2459,10 @@ 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);
> +	saddr = iph->saddr;
>  
>  	hdr_size = sizeof(*amtmu) + sizeof(struct udphdr);
>  	if (!pskb_may_pull(skb, hdr_size))

[Severity: Critical]
This isn't a bug introduced by this patch, but iph is cached here and used
later in the function to read iph->daddr:

amt_update_handler() {
    ...
    iph = ip_hdr(skb);
    if (iph->version == 4) {
        if (ip_mc_check_igmp(skb)) { ... }

        spin_lock_bh(&tunnel->lock);
        amt_igmp_report_handler(amt, skb, tunnel);
        spin_unlock_bh(&tunnel->lock);

        ...
        ip_eth_mc_map(iph->daddr, eth->h_dest);
    }
}

Both ip_mc_check_igmp() and amt_igmp_report_handler() can internally invoke
pskb_may_pull() or similar helpers. Does this cause a use-after-free read
of iph->daddr if the skb head is reallocated?

A similar pattern appears to exist in amt_dev_xmit() where iph->daddr is
read after ip_mc_check_igmp().

[Severity: High]
This isn't a bug introduced by this patch, but similar to the issue in
amt_multicast_data_handler(), amt_update_handler() directly modifies the
skb headroom:

amt_update_handler() {
    ...
    eth->h_proto = htons(ETH_P_IP);
    ip_eth_mc_map(iph->daddr, eth->h_dest);
}

Does this modify shared skb data without unsharing, potentially causing
silent data corruption if the skb is cloned?
-- 
pw-bot: cr