[PATCH bpf] bpf, sockmap: fix use-after-free when the stream parser resizes the skb

[PATCH bpf] bpf, sockmap: fix use-after-free when the stream parser resizes the skb

[Sechang Lim]

sk_psock_strp_parse() runs the BPF_PROG_TYPE_SK_SKB stream-parser program to
find the length of the next message. strparser assembles a message out of
several received skbs by chaining them onto the head's frag_list and recording
where to append the next one in strp->skb_nextp:

	*strp->skb_nextp = skb;
	strp->skb_nextp = &skb->next;

and then calls the parser on the head:

	len = (*strp->cb.parse_msg)(strp, head);

The parser is only meant to inspect the skb, but the program may call
bpf_skb_change_tail() -- or the sibling bpf_skb_pull_data(),
bpf_skb_change_head(), bpf_skb_adjust_room(), all allowed for SK_SKB. Once the
head carries a frag_list these go

	... -> skb_ensure_writable -> pskb_may_pull -> __pskb_pull_tail

and __pskb_pull_tail() frees the frag_list skbs that strparser still tracks
through skb_nextp:

	while ((list = skb_shinfo(skb)->frag_list) != insp) {
		skb_shinfo(skb)->frag_list = list->next;
		consume_skb(list);
	}

strp->skb_nextp now points into a freed sk_buff. The next segment of the same
message arrives in __strp_recv(), which links it with *strp->skb_nextp = skb,
an 8-byte write into the freed skb. The free and the write happen in different
__strp_recv() calls, so the message has to span at least three segments before
it triggers.

  BUG: KASAN: slab-use-after-free in __strp_recv+0x447/0xda0
  Write of size 8 at addr ffff88810db86140 by task repro/349

  Call Trace:
   <IRQ>
   __strp_recv+0x447/0xda0
   __tcp_read_sock+0x13d/0x590
   tcp_bpf_strp_read_sock+0x195/0x320
   strp_data_ready+0x267/0x340
   sk_psock_strp_data_ready+0x1ce/0x350
   tcp_data_queue+0x1364/0x2fd0
   tcp_rcv_established+0xe07/0x1640
   [...]

  Allocated by task 349:
   skb_clone+0x17b/0x210
   __strp_recv+0x2c3/0xda0
   __tcp_read_sock+0x13d/0x590
   [...]

  Freed by task 349:
   kmem_cache_free+0x150/0x570
   __pskb_pull_tail+0x57b/0xc20
   skb_ensure_writable+0x236/0x260
   __bpf_skb_change_tail+0x1d4/0x590
   sk_skb_change_tail+0x2a/0x40
   bpf_prog_1b285dcd6c41373e+0x27/0x30
   bpf_prog_run_pin_on_cpu+0xf3/0x260
   sk_psock_strp_parse+0x118/0x1e0
   __strp_recv+0x4f6/0xda0
   [...]

The same resize also leaves the head's length inconsistent with its frags, so
a later __pskb_pull_tail() can instead hit the BUG_ON(skb_copy_bits(...)) in
net/core/skbuff.c.

Run the parser on a private clone of the head whenever the message spans more
than one skb, so a resizing helper can only touch the clone and strparser's
head and skb_nextp stay valid. Single-skb messages have no frag_list and are
still parsed in place.

Fixes: 8a31db561566 ("bpf: add access to sock fields and pkt data from sk_skb programs")
Signed-off-by: Sechang Lim <rhkrqnwk98@gmail.com>
---
 net/core/skmsg.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index e1850caf1a71..d5b10f9b0ba8 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -1146,14 +1146,30 @@ static int sk_psock_strp_parse(struct strparser *strp, struct sk_buff *skb)
 	struct bpf_prog *prog;
 	int ret = skb->len;
 
-	rcu_read_lock();
+	guard(rcu)();
 	prog = READ_ONCE(psock->progs.stream_parser);
 	if (likely(prog)) {
-		skb->sk = psock->sk;
-		ret = bpf_prog_run_pin_on_cpu(prog, skb);
-		skb->sk = NULL;
+		struct sk_buff *parse_skb = skb;
+
+		/*
+		 * strparser chains the message skbs through skb->frag_list and
+		 * keeps a pointer into that list in strp->skb_nextp.  The parser
+		 * program may call bpf_skb_change_tail() and friends, which go
+		 * through __pskb_pull_tail() and free the frag_list skbs that
+		 * strparser still tracks.  Run the program on a clone when a
+		 * frag_list is present so it cannot drop frags strparser owns.
+		 */
+		if (skb_has_frag_list(skb)) {
+			parse_skb = skb_clone(skb, GFP_ATOMIC);
+			if (!parse_skb)
+				return -ENOMEM;
+		}
+		parse_skb->sk = psock->sk;
+		ret = bpf_prog_run_pin_on_cpu(prog, parse_skb);
+		parse_skb->sk = NULL;
+		if (parse_skb != skb)
+			kfree_skb(parse_skb);
 	}
-	rcu_read_unlock();
 	return ret;
 }
 
-- 
2.43.0

Re: [PATCH bpf] bpf, sockmap: fix use-after-free when the stream parser resizes the skb

[Bobby Eshleman]

On Tue, Jun 09, 2026 at 11:23:03AM +0000, Sechang Lim wrote:
> sk_psock_strp_parse() runs the BPF_PROG_TYPE_SK_SKB stream-parser program to
> find the length of the next message. strparser assembles a message out of
> several received skbs by chaining them onto the head's frag_list and recording
> where to append the next one in strp->skb_nextp:
> 
> 	*strp->skb_nextp = skb;
> 	strp->skb_nextp = &skb->next;
> 
> and then calls the parser on the head:
> 
> 	len = (*strp->cb.parse_msg)(strp, head);
> 
> The parser is only meant to inspect the skb, but the program may call
> bpf_skb_change_tail() -- or the sibling bpf_skb_pull_data(),
> bpf_skb_change_head(), bpf_skb_adjust_room(), all allowed for SK_SKB. Once the
> head carries a frag_list these go
> 
> 	... -> skb_ensure_writable -> pskb_may_pull -> __pskb_pull_tail
> 
> and __pskb_pull_tail() frees the frag_list skbs that strparser still tracks
> through skb_nextp:
> 
> 	while ((list = skb_shinfo(skb)->frag_list) != insp) {
> 		skb_shinfo(skb)->frag_list = list->next;
> 		consume_skb(list);
> 	}
> 
> strp->skb_nextp now points into a freed sk_buff. The next segment of the same
> message arrives in __strp_recv(), which links it with *strp->skb_nextp = skb,
> an 8-byte write into the freed skb. The free and the write happen in different
> __strp_recv() calls, so the message has to span at least three segments before
> it triggers.
> 
>   BUG: KASAN: slab-use-after-free in __strp_recv+0x447/0xda0
>   Write of size 8 at addr ffff88810db86140 by task repro/349
> 
>   Call Trace:
>    <IRQ>
>    __strp_recv+0x447/0xda0
>    __tcp_read_sock+0x13d/0x590
>    tcp_bpf_strp_read_sock+0x195/0x320
>    strp_data_ready+0x267/0x340
>    sk_psock_strp_data_ready+0x1ce/0x350
>    tcp_data_queue+0x1364/0x2fd0
>    tcp_rcv_established+0xe07/0x1640
>    [...]
> 
>   Allocated by task 349:
>    skb_clone+0x17b/0x210
>    __strp_recv+0x2c3/0xda0
>    __tcp_read_sock+0x13d/0x590
>    [...]
> 
>   Freed by task 349:
>    kmem_cache_free+0x150/0x570
>    __pskb_pull_tail+0x57b/0xc20
>    skb_ensure_writable+0x236/0x260
>    __bpf_skb_change_tail+0x1d4/0x590
>    sk_skb_change_tail+0x2a/0x40
>    bpf_prog_1b285dcd6c41373e+0x27/0x30
>    bpf_prog_run_pin_on_cpu+0xf3/0x260
>    sk_psock_strp_parse+0x118/0x1e0
>    __strp_recv+0x4f6/0xda0
>    [...]
> 
> The same resize also leaves the head's length inconsistent with its frags, so
> a later __pskb_pull_tail() can instead hit the BUG_ON(skb_copy_bits(...)) in
> net/core/skbuff.c.
> 
> Run the parser on a private clone of the head whenever the message spans more
> than one skb, so a resizing helper can only touch the clone and strparser's
> head and skb_nextp stay valid. Single-skb messages have no frag_list and are
> still parsed in place.
> 
> Fixes: 8a31db561566 ("bpf: add access to sock fields and pkt data from sk_skb programs")
> Signed-off-by: Sechang Lim <rhkrqnwk98@gmail.com>
> ---
>  net/core/skmsg.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index e1850caf1a71..d5b10f9b0ba8 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -1146,14 +1146,30 @@ static int sk_psock_strp_parse(struct strparser *strp, struct sk_buff *skb)
>  	struct bpf_prog *prog;
>  	int ret = skb->len;
>  
> -	rcu_read_lock();
> +	guard(rcu)();

I'd probably not mix this rcu change into this patch, though I
understand it drops an unlock() call in the new err return.

>  	prog = READ_ONCE(psock->progs.stream_parser);
>  	if (likely(prog)) {
> -		skb->sk = psock->sk;
> -		ret = bpf_prog_run_pin_on_cpu(prog, skb);
> -		skb->sk = NULL;
> +		struct sk_buff *parse_skb = skb;
> +
> +		/*
> +		 * strparser chains the message skbs through skb->frag_list and
> +		 * keeps a pointer into that list in strp->skb_nextp.  The parser
> +		 * program may call bpf_skb_change_tail() and friends, which go
> +		 * through __pskb_pull_tail() and free the frag_list skbs that
> +		 * strparser still tracks.  Run the program on a clone when a
> +		 * frag_list is present so it cannot drop frags strparser owns.
> +		 */
> +		if (skb_has_frag_list(skb)) {
> +			parse_skb = skb_clone(skb, GFP_ATOMIC);
> +			if (!parse_skb)
> +				return -ENOMEM;

I wonder if this could be further gated by prog->aux->changes_pkt_data?

One possible issue with adding an allocation here and returning -ENOMEM
is that this will shutdown stream parsing permanently for this sk when
under memory pressure, which is new behavior. Maybe, if we return 0, it
looks like the caller's accounting stays true by taking the "needs more
header" path, and then we can try again from the same place on the next
data_ready, and so be resilient even under memory pressure? I guess the
trade-off being that their may not come another data_ready to cause
progress... not sure which is better. Maybe strp_start_timer could be
used here too?

Best,
Bobby