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

[Jiayuan Chen]

on 9/3/26 10:01 PM, bot+bpf-ci@xxxxxxxxxx wrote:
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 61940e7535523..1bbb72138ac64 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -12912,8 +12912,8 @@ __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, EINVAL.
> > + * EOPNOTSUPP if protocol specific destroy handler is not supported.
> >    * 0 otherwise
> The EPROTONOSUPPORT -> EOPNOTSUPP correction is right, but the rewritten
> error list is incomplete. tcp_abort() returns -ENOENT when the socket is
> already closed (net/ipv4/tcp.c, around the sk_state == TCP_CLOSE check):
>
>      /* Avoid closing the same socket twice. */
>      if (sk->sk_state == TCP_CLOSE) {
>          if (!has_current_bpf_ctx())
>              release_sock(sk);
>          return -ENOENT;
>      }
>
> That error propagates through sk->sk_prot->diag_destroy(sk, ECONNABORTED)
> and is reachable from a BPF iterator (destroying a sock the iterator
> already destroyed, or one closed concurrently). In contrast, EINVAL is only
> reachable through the narrower tcp_abort_override() path for MPTCP listener
> subflows.
>
> Should ENOENT be added to the list?


I'd like to describe it as "others" since diag_destroy may change it's 
return value.