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) <[email protected]>
Closes: https://lore.kernel.org/bpf/[email protected]/
Signed-off-by: Jiayuan Chen <[email protected]>
---
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/[email protected]/
---
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
