Re: [PATCH net] tcp: do not resurrect a former listener with SO_ATTACH_REUSEPORT_[CE]BPF

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new check in reuseport_attach_prog()
  (net/core/sock_reuseport.c) decides the socket is an…

--

tcp: do not resurrect a former listener with SO_ATTACH_REUSEPORT_[CE]BPF

This patch makes reuseport_attach_prog() return -EINVAL when it is called
on an unhashed TCP socket whose unhashed_state is TCP_LISTEN. Without this,
a socket that was shutdown() after listen() could be put back into the
listening section of a new reuseport group. reuseport_migrate_sock() could
then pick that dead socket, which leads to a use-after-free in
espintcp_destruct().

> diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c
> index fccbdd0430b05..41a25be1bfff6 100644
> --- a/net/core/sock_reuseport.c
> +++ b/net/core/sock_reuseport.c

[ ... ]

> @@ -692,6 +693,14 @@ int reuseport_attach_prog(struct sock *sk, struct bpf_prog *prog)
>  		if (!sk->sk_reuseport)
>  			return -EINVAL;
>  
> +		/* Do not put a shutdown()ed TCP listener back into the
> +		 * listening section of a reuseport group while it is in
> +		 * TCP_CLOSE: requests can still migrate to it.
> +		 */
> +		if (sk->sk_protocol == IPPROTO_TCP &&
> +		    inet_csk(sk)->unhashed_state == TCP_LISTEN)
> +			return -EINVAL;

[Severity: High]
Can a raw socket reach this check with sk_protocol == IPPROTO_TCP? For
socket(AF_INET, SOCK_RAW, IPPROTO_TCP), inet_create() does:

    if (SOCK_RAW == sock->type) {
        inet->inet_num = protocol;
    ...
    sk->sk_protocol = protocol;

In that case the object is a struct raw_sock (or raw6_sock for AF_INET6),
not an inet_connection_sock.

As far as I can tell, this path gets there from userspace:

  socket(AF_INET or AF_INET6, SOCK_RAW, IPPROTO_TCP)
    raw_hash_sk() hashes it, since inet_num is 6
  connect(AF_UNSPEC)
    inet_dgram_connect()->__udp_disconnect()->sk->sk_prot->unhash()
    (raw_unhash_sk()), so sk_unhashed() is now true
  setsockopt(SO_REUSEPORT, 1)
    only sk_is_inet() is checked
  setsockopt(SO_ATTACH_REUSEPORT_CBPF), or SO_ATTACH_REUSEPORT_EBPF with a
  BPF_PROG_TYPE_SOCKET_FILTER program
    sk_reuseport_attach_filter()->reuseport_attach_prog()

Neither attach path checks sk_type before it calls reuseport_attach_prog().
Only the SK_REUSEPORT program type does.

In inet_connection_sock, unhashed_state comes after icsk_accept_queue, two
bind bucket pointers, two timer_lists and five u32s. struct raw_sock only
adds icmp_filter, ipmr_table and numa_drop_counters on top of inet_sock.

On !SMP builds, or with lockdep or debug spinlock configs that make
spinlock_t and timer_list larger, does this become a slab-out-of-bounds
read past the raw_sock object? On other configs the read lands in padding
or in the drop counters, which may be uninitialized.

For raw6_sock it reads an unrelated byte inside ipv6_pinfo. If that byte
happens to equal TCP_LISTEN, the attach fails with -EINVAL.

The only privilege needed is CAP_NET_RAW in the netns user namespace, which
an unprivileged user can get with unshare(CLONE_NEWUSER | CLONE_NEWNET).

Would the existing sk_is_tcp() helper in include/net/sock.h work better
here than the bare sk_protocol test?

    return sk_is_inet(sk) &&
           sk->sk_type == SOCK_STREAM &&
           sk->sk_protocol == IPPROTO_TCP;

> +
>  		err = reuseport_alloc(sk, false);
>  		if (err)
>  			return err;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105164830.1682714-1-edumazet%40kernel.org