[PATCH bpf v2 1/2] bpf: Fix out-of-bounds read of sk_protocol in bpf_sock_destroy()

[PATCH bpf v2 1/2] bpf: Fix out-of-bounds read of sk_protocol in bpf_sock_destroy()

[Jiayuan Chen]

sk_protocol lives in struct sock, not in struct sock_common. A timewait
or request sock handed to bpf_sock_destroy() by the tcp iterator is
neither, so reading sk->sk_protocol runs past the object:

==================================================================
BUG: KASAN: slab-out-of-bounds in bpf_sock_destroy+0xc7/0xe0
Read of size 2 at addr ffff8881047d11b4 by task test_progs/428

Tainted: [W]=WARN
Call Trace:
 <TASK>
 dump_stack_lvl+0x91/0xf0
 print_report+0xd1/0x630
 kasan_report+0xf3/0x130
 __asan_report_load2_noabort+0x14/0x30
 bpf_sock_destroy+0xc7/0xe0
 bpf_prog_c3dd61f9d9cd9f37_iter_tcp6_timewait+0x9f/0xb7
 bpf_iter_run_prog+0x538/0xde0
 bpf_iter_tcp_seq_show+0x26b/0x4b0
 bpf_seq_read+0x424/0x1210
 vfs_read+0x197/0xe40
 ksys_read+0x119/0x240
 __x64_sys_read+0x72/0xc0
 x64_sys_call+0x647/0x27e0
 do_syscall_64+0xe5/0x610
 entry_SYSCALL_64_after_hwframe+0x76/0x7e

Only check sk_protocol on full socks. tcp_abort() already knows how to
deal with TIME_WAIT and NEW_SYN_RECV socks. Also fix the comment, it
never matched the code.

Fixes: 4ddbcb886268 ("bpf: Add bpf_sock_destroy kfunc")
Reported-by: Xiang Mei (Microsoft) <xmei5@asu.edu>
Closes: https://lore.kernel.org/bpf/20260702224519.800135-1-xmei5@asu.edu/
Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
A reviewer asked to add ENOENT to the list of errors in the comment.
I'd rather not list what the handlers return, that can change any time,
so the comment now says "EOPNOTSUPP, or whatever the protocol specific
destroy handler returns".

v1 -> v2: modify comment AND avoid flaky about selftest
v1: https://lore.kernel.org/bpf/20260903125306.299943-1-jiayuan.chen@linux.dev/
---
 net/core/filter.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 61940e753552..a41cc60a401a 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -12912,8 +12912,9 @@ __bpf_kfunc_start_defs();
  * @sock: Pointer to socket to be destroyed
  *
  * Return:
- * On error, may return EPROTONOSUPPORT, EINVAL.
- * EPROTONOSUPPORT if protocol specific destroy handler is not supported.
+ * On error, may return EOPNOTSUPP, or whatever the protocol specific
+ * destroy handler returns.
+ * EOPNOTSUPP if protocol specific destroy handler is not supported.
  * 0 otherwise
  */
 __bpf_kfunc int bpf_sock_destroy(struct sock_common *sock)
@@ -12925,8 +12926,12 @@ __bpf_kfunc int bpf_sock_destroy(struct sock_common *sock)
 	 * Supporting protocols will need to acquire sock lock in the BPF context
 	 * prior to invoking this kfunc.
 	 */
-	if (!sk->sk_prot->diag_destroy || (sk->sk_protocol != IPPROTO_TCP &&
-					   sk->sk_protocol != IPPROTO_UDP))
+	if (!sk->sk_prot->diag_destroy)
+		return -EOPNOTSUPP;
+
+	if (sk_fullsock(sk) &&
+	    sk->sk_protocol != IPPROTO_TCP &&
+	    sk->sk_protocol != IPPROTO_UDP)
 		return -EOPNOTSUPP;
 
 	return sk->sk_prot->diag_destroy(sk, ECONNABORTED);
-- 
2.43.0

[PATCH bpf v2 2/2] selftests/bpf: Test bpf_sock_destroy() on a TIME_WAIT sock

[Jiayuan Chen]

Add a tcp_timewait subtest. The client shuts down first and the server
closes after it, so the client sock ends up in TIME_WAIT. A tcp iterator
then finds the timewait sock by the cookie it inherited from the client
sock and destroys it. Iterate once more to make sure it is gone.

Without the previous fix bpf_sock_destroy() reads past the timewait sock
and KASAN complains.

  ./test_progs -a sock_destroy

  #444/1   sock_destroy/tcp_client:OK
  #444/2   sock_destroy/tcp_server:OK
  #444/3   sock_destroy/tcp_timewait:OK
  #444/4   sock_destroy/udp_client:OK
  #444/5   sock_destroy/udp_server:OK
  #444/6   sock_destroy/trace_tcp_destroy_sock:OK
  #444     sock_destroy:OK
  Summary: 1/6 PASSED, 0 SKIPPED, 0/0 FAILED

Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 .../selftests/bpf/prog_tests/sock_destroy.c   | 71 ++++++++++++++++++-
 .../selftests/bpf/progs/sock_destroy_prog.c   | 30 ++++++++
 2 files changed, 100 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
index 9c11938fe597..b90fab707338 100644
--- a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
+++ b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
@@ -110,6 +110,74 @@ static void test_tcp_server(struct sock_destroy_prog *skel)
 		close(serv);
 }
 
+static void test_tcp_timewait(struct sock_destroy_prog *skel)
+{
+	int serv = -1, clien = -1, accept_serv = -1, n;
+	struct timeval tv = {};
+	char buf[1];
+
+	serv = start_server(AF_INET6, SOCK_STREAM, NULL, 0, 0);
+	if (!ASSERT_GE(serv, 0, "start_server"))
+		goto cleanup;
+
+	clien = connect_to_fd(serv, 0);
+	if (!ASSERT_GE(clien, 0, "connect_to_fd"))
+		goto cleanup;
+
+	accept_serv = accept(serv, NULL, NULL);
+	if (!ASSERT_GE(accept_serv, 0, "serv accept"))
+		goto cleanup;
+
+	/*
+	 * Active close from the client, then close the server side. Once
+	 * recv() sees EOF the server FIN has been processed and the client
+	 * sock is in TIME_WAIT. Block without timeout so a loaded CI box
+	 * can't race us.
+	 */
+	if (!ASSERT_OK(setsockopt(clien, SOL_SOCKET, SO_RCVTIMEO, &tv,
+				  sizeof(tv)), "clear rcvtimeo"))
+		goto cleanup;
+	if (!ASSERT_OK(shutdown(clien, SHUT_WR), "client shutdown"))
+		goto cleanup;
+
+	/*
+	 * Make sure the server has seen the client FIN before it closes,
+	 * so the two FINs never cross.
+	 */
+	n = recv(accept_serv, buf, sizeof(buf), 0);
+	if (!ASSERT_EQ(n, 0, "server recv EOF"))
+		goto cleanup;
+
+	close(accept_serv);
+	accept_serv = -1;
+
+	/* block until return EOF */
+	n = recv(clien, buf, sizeof(buf), 0);
+	if (!ASSERT_EQ(n, 0, "client recv EOF"))
+		goto cleanup;
+
+	/* Run iterator program that destroys the timewait client sock. */
+	skel->bss->tw_found = 0;
+	start_iter_sockets(skel->progs.iter_tcp6_timewait);
+	if (!ASSERT_EQ(skel->bss->tw_found, 1, "timewait sock found"))
+		goto cleanup;
+
+	ASSERT_OK(skel->bss->tw_destroy_err, "destroy timewait sock");
+
+	/* The destroyed timewait sock must be gone. */
+	skel->bss->tw_found = 0;
+	start_iter_sockets(skel->progs.iter_tcp6_timewait);
+	ASSERT_EQ(skel->bss->tw_found, 0, "timewait sock destroyed");
+
+cleanup:
+	if (clien != -1)
+		close(clien);
+	if (accept_serv != -1)
+		close(accept_serv);
+	if (serv != -1)
+		close(serv);
+}
+
 static void test_udp_client(struct sock_destroy_prog *skel)
 {
 	int serv = -1, clien = -1, n = 0;
@@ -204,11 +272,12 @@ void test_sock_destroy(void)
 		test_tcp_client(skel);
 	if (test__start_subtest("tcp_server"))
 		test_tcp_server(skel);
+	if (test__start_subtest("tcp_timewait"))
+		test_tcp_timewait(skel);
 	if (test__start_subtest("udp_client"))
 		test_udp_client(skel);
 	if (test__start_subtest("udp_server"))
 		test_udp_server(skel);
-
 	RUN_TESTS(sock_destroy_prog_fail);
 
 cleanup:
diff --git a/tools/testing/selftests/bpf/progs/sock_destroy_prog.c b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
index 9e0bf7a54cec..0a8887543218 100644
--- a/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
+++ b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
@@ -7,6 +7,8 @@
 #include "bpf_tracing_net.h"
 
 __be16 serv_port = 0;
+int tw_found = 0;
+int tw_destroy_err = 0;
 
 int bpf_sock_destroy(struct sock_common *sk) __ksym;
 
@@ -100,6 +102,34 @@ int iter_tcp6_server(struct bpf_iter__tcp *ctx)
 	return 0;
 }
 
+SEC("iter/tcp")
+int iter_tcp6_timewait(struct bpf_iter__tcp *ctx)
+{
+	struct sock_common *sk_common = ctx->sk_common;
+	__u64 *val;
+	int key = 0;
+
+	if (!sk_common)
+		return 0;
+
+	if (sk_common->skc_family != AF_INET6)
+		return 0;
+
+	if (!bpf_skc_to_tcp_timewait_sock(sk_common))
+		return 0;
+
+	val = bpf_map_lookup_elem(&tcp_conn_sockets, &key);
+	if (!val)
+		return 0;
+	/* The timewait sock inherits the cookie of the closed client sock. */
+	if (bpf_get_socket_cookie(sk_common) != *val)
+		return 0;
+
+	tw_found++;
+	tw_destroy_err = bpf_sock_destroy(sk_common);
+
+	return 0;
+}
 
 SEC("iter/udp")
 int iter_udp6_client(struct bpf_iter__udp *ctx)
-- 
2.43.0

Re: [PATCH bpf v2 1/2] bpf: Fix out-of-bounds read of sk_protocol in bpf_sock_destroy()

[sashiko-bot]

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

Pre-existing issues:
- [High] bpf_sock_destroy bypasses socket locking for iterators that do not pre-lock the socket, leading to data corruption and kernel panics.
- [High] Calling cond_resched() while holding rcu_read_lock() via bpf_sock_destroy() on TCP_LISTEN sockets.
--

commit 1a9df13e7482333dfb4a7ad04982dba6254ff808
Author: Jiayuan Chen <jiayuan.chen@linux.dev>

bpf: Fix out-of-bounds read of sk_protocol in bpf_sock_destroy()

sk_protocol lives in struct sock, not in struct sock_common. A timewait
or request sock handed to bpf_sock_destroy() by the tcp iterator is
neither, so reading sk->sk_protocol runs past the object.

Closes: https://lore.kernel.org/bpf/20260702224519.800135-1-xmei5@asu.edu/

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 61940e7535523..a41cc60a401a5 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
[ ... ]
> @@ -12925,8 +12926,12 @@ __bpf_kfunc int bpf_sock_destroy(struct sock_common *sock)
>  	 * Supporting protocols will need to acquire sock lock in the BPF context
>  	 * prior to invoking this kfunc.
>  	 */
> -	if (!sk->sk_prot->diag_destroy || (sk->sk_protocol != IPPROTO_TCP &&
> -					   sk->sk_protocol != IPPROTO_UDP))
> +	if (!sk->sk_prot->diag_destroy)
> +		return -EOPNOTSUPP;
> +
> +	if (sk_fullsock(sk) &&
> +	    sk->sk_protocol != IPPROTO_TCP &&
> +	    sk->sk_protocol != IPPROTO_UDP)
>  		return -EOPNOTSUPP;
>  
>  	return sk->sk_prot->diag_destroy(sk, ECONNABORTED);

[Severity: High]
This is a pre-existing issue, but does this invoke the protocol's destroy
handler without proper locking?

The bpf_sock_destroy() kfunc can be called from iterators. It calls the
protocol's diag_destroy() handler:

net/core/filter.c:bpf_sock_destroy() {
    ...
    return sk->sk_prot->diag_destroy(sk, ECONNABORTED);
}

For TCP sockets, this resolves to tcp_abort(), which skips acquiring the
socket lock because has_current_bpf_ctx() is always true inside a BPF
program:

net/ipv4/tcp.c:tcp_abort() {
    ...
    if (!has_current_bpf_ctx())
        lock_sock(sk);
    ...
}

While some iterators pre-lock the socket, others (like sockmap iterators) do
not. Does this result in tcp_abort() running lockless and potentially
corrupting socket state?

Additionally, if a raw socket with IPPROTO_TCP is passed, raw_abort() is
invoked instead, which unconditionally calls lock_sock():

net/ipv4/raw.c:raw_abort() {
    ...
    lock_sock(sk);
    ...
}

Since iterators run under rcu_read_lock(), will this sleep in an atomic
context and trigger a BUG?

[Severity: High]
This is also a pre-existing issue, but can this call cond_resched() while
holding an RCU read lock?

When bpf_sock_destroy() processes a TCP socket in the TCP_LISTEN state,
it calls tcp_abort(), which then calls inet_csk_listen_stop():

net/ipv4/tcp.c:tcp_abort() {
    ...
    inet_csk_listen_stop(sk);
    ...
}

Inside inet_csk_listen_stop(), there is a loop that explicitly yields:

net/ipv4/inet_connection_sock.c:inet_csk_listen_stop() {
    ...
    cond_resched();
    ...
}

Since bpf_iter_run_prog() runs iterators under rcu_read_lock_dont_migrate(),
will calling cond_resched() from this atomic context trigger a "sleeping
function called from invalid context" BUG?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904094954.63447-1-jiayuan.chen@linux.dev?part=1

Re: [PATCH bpf v2 1/2] bpf: Fix out-of-bounds read of sk_protocol in bpf_sock_destroy()

[Jiayuan Chen]

on 9/4/26 6:19 PM, sashiko-bot@kernel.org write:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] bpf_sock_destroy bypasses socket locking for iterators that do not pre-lock the socket, leading to data corruption and kernel panics.


Hallucination


> - [High] Calling cond_resched() while holding rcu_read_lock() via bpf_sock_destroy() on TCP_LISTEN sockets.


It's True


> --
[...]
> [ ... ]
>> @@ -12925,8 +12926,12 @@ __bpf_kfunc int bpf_sock_destroy(struct sock_common *sock)
>>   	 * Supporting protocols will need to acquire sock lock in the BPF context
>>   	 * prior to invoking this kfunc.
>>   	 */
>> -	if (!sk->sk_prot->diag_destroy || (sk->sk_protocol != IPPROTO_TCP &&
>> -					   sk->sk_protocol != IPPROTO_UDP))
>> +	if (!sk->sk_prot->diag_destroy)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (sk_fullsock(sk) &&
>> +	    sk->sk_protocol != IPPROTO_TCP &&
>> +	    sk->sk_protocol != IPPROTO_UDP)
>>   		return -EOPNOTSUPP;
>>   
>>   	return sk->sk_prot->diag_destroy(sk, ECONNABORTED);
> [Severity: High]
> This is a pre-existing issue, but does this invoke the protocol's destroy
> handler without proper locking?
>
> The bpf_sock_destroy() kfunc can be called from iterators. It calls the
> protocol's diag_destroy() handler:
>
> net/core/filter.c:bpf_sock_destroy() {
>      ...
>      return sk->sk_prot->diag_destroy(sk, ECONNABORTED);
> }
>
> For TCP sockets, this resolves to tcp_abort(), which skips acquiring the
> socket lock because has_current_bpf_ctx() is always true inside a BPF
> program:
>
> net/ipv4/tcp.c:tcp_abort() {
>      ...
>      if (!has_current_bpf_ctx())
>          lock_sock(sk);
>      ...
> }
>
> While some iterators pre-lock the socket, others (like sockmap iterators) do
> not. Does this result in tcp_abort() running lockless and potentially
> corrupting socket state?

No.


I try load "SEC("iter/sockmap")" but verifier rejected it.

Only the tcp/udp iters mark sk as PTR_TRUSTED and bpf_sock_destroy() 
needs a trusted arg.


> Additionally, if a raw socket with IPPROTO_TCP is passed, raw_abort() is
> invoked instead, which unconditionally calls lock_sock():
>
> net/ipv4/raw.c:raw_abort() {
>      ...
>      lock_sock(sk);
>      ...
> }
>
> Since iterators run under rcu_read_lock(), will this sleep in an atomic
> context and trigger a BUG?
>
> [Severity: High]
> This is also a pre-existing issue, but can this call cond_resched() while
> holding an RCU read lock?
>
> When bpf_sock_destroy() processes a TCP socket in the TCP_LISTEN state,
> it calls tcp_abort(), which then calls inet_csk_listen_stop():
>
> net/ipv4/tcp.c:tcp_abort() {
>      ...
>      inet_csk_listen_stop(sk);
>      ...
> }
>
> Inside inet_csk_listen_stop(), there is a loop that explicitly yields:
>
> net/ipv4/inet_connection_sock.c:inet_csk_listen_stop() {
>      ...
>      cond_resched();
>      ...
> }
> Since bpf_iter_run_prog() runs iterators under rcu_read_lock_dont_migrate(),
> will calling cond_resched() from this atomic context trigger a "sleeping
> function called from invalid context" BUG?


Yes, reproduced with a listener that still has an unaccepted child.

Let me think how to fix it...

Re: [PATCH bpf v2 2/2] selftests/bpf: Test bpf_sock_destroy() on a TIME_WAIT sock

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]

> diff --git a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
> index 9c11938fe597d..b90fab7073386 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c

[ ... ]

> @@ -204,11 +272,12 @@ void test_sock_destroy(void)
>  		test_tcp_client(skel);
>  	if (test__start_subtest("tcp_server"))
>  		test_tcp_server(skel);
> +	if (test__start_subtest("tcp_timewait"))
> +		test_tcp_timewait(skel);
>  	if (test__start_subtest("udp_client"))
>  		test_udp_client(skel);
>  	if (test__start_subtest("udp_server"))
>  		test_udp_server(skel);
> -
>  	RUN_TESTS(sock_destroy_prog_fail);

This isn't a bug, but was dropping the blank line before RUN_TESTS()
intended as part of this change? The deleted line had been separating the
block of test__start_subtest() calls from RUN_TESTS(), which is a
different kind of step. If the grouping was intentional, could that line
stay so the diff is limited to registering the new subtest?

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33861180631