In the Linux kernel, the following vulnerability has been resolved: bpf, sockmap: Fix af_unix...

In the Linux kernel, the following vulnerability has been resolved:

bpf, sockmap: Fix af_unix null-ptr-deref in proto update

unix_stream_connect() sets sk_state (WRITE_ONCE(sk->sk_state, TCP_ESTABLISHED)) before it assigns a peer (unix_peer(sk) = newsk). sk_state == TCP_ESTABLISHED makes sock_map_sk_state_allowed() believe that socket is properly set up, which would include having a defined peer. IOW, there's a window when unix_stream_bpf_update_proto() can be called on socket which still has unix_peer(sk) == NULL.

     CPU0 bpf                            CPU1 connect
     --------                            ------------

                            WRITE_ONCE(sk->sk_state, TCP_ESTABLISHED)

sock_map_sk_state_allowed(sk) ... sk_pair = unix_peer(sk) sock_hold(sk_pair) sock_hold(newsk) smp_mb__after_atomic() unix_peer(sk) = newsk

BUG: kernel NULL pointer dereference, address: 0000000000000080 RIP: 0010:unix_stream_bpf_update_proto+0xa0/0x1b0 Call Trace: sock_map_link+0x564/0x8b0 sock_map_update_common+0x6e/0x340 sock_map_update_elem_sys+0x17d/0x240 __sys_bpf+0x26db/0x3250 __x64_sys_bpf+0x21/0x30 do_syscall_64+0x6b/0x3a0 entry_SYSCALL_64_after_hwframe+0x76/0x7e

Initial idea was to move peer assignment before the sk_state update1, but that involved an additional memory barrier, and changing the hot path was rejected. Then a NULL check during proto update in unix_stream_bpf_update_proto() was considered2, but the follow-up discussion3 focused on the root cause, i.e. sockmap update taking a wrong lock. Or, more specifically, missing unix_state_lock()4. In the end it was concluded that teaching sockmap about the af_unix locking would be unnecessarily complex5. Complexity aside, since BPF_PROG_TYPE_SCHED_CLS and BPF_PROG_TYPE_SCHED_ACT are allowed to update sockmaps, sock_map_update_elem() taking the unix lock, as it is currently implemented in unix_state_lock(): spin_lock(&unix_sk(s)->lock), would be problematic. unix_state_lock() taken in a process context, followed by a softirq-context TC BPF program attempting to take the same spinlock -- deadlock6. This way we circled back to the peer check idea2.

Summary of scenarios where af_unix/stream connect() may race a sockmap update:

1. connect() vs. bpf(BPF_MAP_UPDATE_ELEM), i.e. sock_map_update_elem_sys()

   Implemented NULL check is sufficient. Once assigned, socket peer won't be released until socket fd is released. And that's not an issue because sock_map_update_elem_sys() bumps fd refcnf.

2. connect() vs BPF program doing update

   Update restricted per verifier.c:may_update_sockmap() to

   BPF_PROG_TYPE_TRACING/BPF_TRACE_ITER BPF_PROG_TYPE_SOCK_OPS (bpf_sock_map_update() only) BPF_PROG_TYPE_SOCKET_FILTER BPF_PROG_TYPE_SCHED_CLS BPF_PROG_TYPE_SCHED_ACT BPF_PROG_TYPE_XDP BPF_PROG_TYPE_SK_REUSEPORT BPF_PROG_TYPE_FLOW_DISSECTOR BPF_PROG_TYPE_SK_LOOKUP

   Plus one more race to consider:

        CPU0 bpf                            CPU1 connect
        --------                            ------------
   
                               WRITE_ONCE(sk->sk_state, TCP_ESTABLISHED)
   

   sock_map_sk_state_allowed(sk) sock_hold(newsk) smp_mb__after_atomic()

---truncated---