Re: [PATCH bpf] bpf: fix UAF in sock clone early bailouts

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] BPF map corruption via shallow-copied sk_user_data and sk_reuseport_cb during sk_clone early bailouts.
- [High] Unbalanced decrement of global network timestamp static key during sk_clone early bailouts.
--

--- Patch [1]: [PATCH bpf] bpf: fix UAF in sock clone early bailouts ---
commit 81577dfc9f1b10000a3b1dfc19a5b577e986ec33
Author: Matt Bobrowski <mattbobrowski@google.com>

bpf: fix UAF in sock clone early bailouts

This commit fixes a use-after-free on parent socket's BPF local storage
caused by an early bailout in sk_clone() prior to clearing
newsk->sk_bpf_storage. By resetting newsk->sk_bpf_storage to NULL
immediately after sock_copy(), the deallocation path won't destroy the
parent socket's storage.

> diff --git a/net/core/sock.c b/net/core/sock.c
> index 8a59bfaa8096e..498a57f34f5b5 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2492,6 +2492,9 @@ struct sock *sk_clone(const struct sock *sk, const gfp_t priority,
>  	sock_copy(newsk, sk);
>  
>  	newsk->sk_prot_creator = prot;
> +#ifdef CONFIG_BPF_SYSCALL
> +	RCU_INIT_POINTER(newsk->sk_bpf_storage, NULL);
> +#endif

[Severity: High]
This is a pre-existing issue, but I noticed there might be a similar issue
with sk_reuseport_cb and sk_user_data. Since sock_copy() does a shallow
copy, newsk initially points to the parent's sk_reuseport_cb and
sk_user_data.

If sk_clone() bails out early (e.g., sk_filter_charge() fails):

net/core/sock.c:sk_clone() {
    ...
    if (unlikely(!is_charged || xfrm_sk_clone_policy(newsk, sk))) {
        ...
        goto free;
    }

    RCU_INIT_POINTER(newsk->sk_reuseport_cb, NULL);
    ...
}

The bailout happens before sk_reuseport_cb is cleared. When newsk is freed,
bpf_sk_reuseport_detach() can follow the copied sk_user_data to the parent's
BPF map slot and write NULL to it:

kernel/bpf/reuseport_array.c:bpf_sk_reuseport_detach() {
    ...
    RCU_INIT_POINTER(*socks, NULL);
    ...
}

Does this corrupt the parent socket's BPF map during early bailouts?


[Severity: High]
This is also a pre-existing issue, but could the same early bailout pattern
cause an unbalanced decrement of the global network timestamp static key?

sock_copy() copies SK_FLAGS_TIMESTAMP into newsk->sk_flags. The global
timestamp counter is only incremented at the end of sk_clone():

net/core/sock.c:sk_clone() {
    ...
    if (sock_needs_netstamp(sk) && newsk->sk_flags & SK_FLAGS_TIMESTAMP)
        net_enable_timestamp();
}

However, during destruction of the aborted clone, the global counter is
decremented unconditionally because the copied flag was set:

net/core/sock.c:__sk_destruct() {
    ...
    sock_disable_timestamp(sk, SK_FLAGS_TIMESTAMP);
    ...
}

Could an early bailout skip the net_enable_timestamp() call but still
execute the sock_disable_timestamp() call on free, leading to an underflow?

>  	/* SANITY */
>  	if (likely(newsk->sk_net_refcnt)) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709025316.999913-1-mattbobrowski@google.com?part=1