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

[Taehee Yoo]

On Wed, Jul 8, 2026 at 4:32 AM Michael Bommarito
<michael.bommarito@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxx/
> v2: https://lore.kernel.org/all/20260617123443.3586930-1-michael.bommarito@xxxxxxxxx/
>

Hi Michael,

Thanks a lot for this work!

Sashiko flagged one more issue, please take a look:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260707193243.3448201-1-michael.bommarito%40gmail.com

Also, please remove the unnecessary comments in the patch.
AI-generated code tends to include too many comments.

One more thing: please follow the reverse Christmas tree order
(longest to shortest) for local variable declarations.

Thanks,
Taehee

> 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
>