Re: [PATCH bpf v3 2/2] bpf: Fix reference leak in bpf_sk_assign()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] bpf_sk_assign_tcp_reqsk() stores an unreferenced pointer to a TCP_LISTEN socket inside a request socket. If the packet is queued via NF_QUEUE, the listener socket can be freed, causing a Use-After-Free when the packet is reinjected and stolen by tcp_v4_rcv().
--

commit 8e6dd77792c2f082ee2b1a593a577a3112cbcf19
Author: Michal Luczaj <mhal@rbox.co>

bpf: Fix reference leak in bpf_sk_assign()

This patch aims to fix a reference leak in bpf_sk_assign() by freezing the
decision on whether a socket is refcounted at assign time. It introduces a
new destructor sock_pfree_refcounted() and modifies __nf_queue() to bump
references for prefetched sockets without references.

> diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
> index 73363ceedebe3..e2e9739c00ac5 100644
> --- a/net/netfilter/nf_queue.c
> +++ b/net/netfilter/nf_queue.c
> @@ -195,16 +195,16 @@ static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
>  		break;
>  	}
>  
> -	if (skb_sk_is_prefetched(skb)) {
> -		struct sock *sk = skb->sk;
> -
> -		if (!sk_is_refcounted(sk)) {
> -			if (!refcount_inc_not_zero(&sk->sk_refcnt))
> -				return -ENOTCONN;
> +	/*
> +	 * If the skb was prefetched without taking sock's ref, bump it.
> +	 * Skip sock_pfree-dtor'ed skbs coming from bpf_sk_assign_tcp_reqsk().
> +	 */
> +	if (skb_sk_is_prefetched_noref(skb) && sk_fullsock(skb->sk)) {
[Severity: Critical]
This isn't a bug introduced by this patch, but does maintaining this behavior
for request sockets leave an unreferenced listener socket exposed to a
use-after-free?

In bpf_sk_assign_tcp_reqsk(), a request socket is allocated without taking a
reference on the listener socket (attach_listener = false), and the
unreferenced listener is stored in req->rsk_listener.

Because sk_fullsock() evaluates to false for request sockets, this check
bypasses bumping the reference count for the SKB when it is handed to
netfilter queue. While the SKB is held in userspace, it escapes the RCU
read-side critical section. If the listener socket is closed and freed
during this time, what happens when the packet is reinjected?

It appears that when tcp_v4_rcv() processes the reinjected packet, it calls
skb_steal_sock() which extracts the freed req->rsk_listener, clears the
destructor (bypassing sock_pfree), and returns the freed socket for use,
triggering a use-after-free.

Could this be triggered by a user with privileges to set up a TC BPF rule
and netfilter queue?

> +		if (!refcount_inc_not_zero(&skb->sk->sk_refcnt))
> +			return -ENOTCONN;
>  
> -			/* drop refcount on skb_orphan */
> -			skb->destructor = sock_edemux;
> -		}
> +		/* drop refcount on skb_orphan */
> +		skb->destructor = sock_edemux;
>  	}
>  
>  	entry = kmalloc(sizeof(*entry) + route_key_size, GFP_ATOMIC);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902-sockmap-lookup-tcp-leak-v3-0-b998b3d49d07@rbox.co?part=2