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

From: Jiayuan Chen

Date: Thu Sep 10 2026 - 08:28:23 EST


On 9/10/26 8:07 PM, bot+bpf-ci@xxxxxxxxxx wrote:
diff --git a/net/core/filter.c b/net/core/filter.c
index 2a84f9d01131..cae43b999162 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -12913,8 +12913,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)
@@ -12926,8 +12927,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);
This isn't a bug, but would it help to keep the TCP/UDP rationale comment
next to the protocol check, and add a short note that sk_protocol is only
valid on full socks? The block comment explaining the locking semantics
and TCP/UDP restriction now sits two statements away from the protocol
test it documents, and the newly added sk_fullsock() guard doesn't have
any nearby note explaining why it's there. The reason (sk_protocol is a
struct sock member not present in sock_common) is only in the changelog.


Current commit message is enough.

That's why we encourage high quality commit message.