Wind River Support Network

Description

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 update[1], 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 considered[2], but the follow-up discussion[3] 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 complex[5]. 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 -- deadlock[6]. This way we circled back to the peer check idea[2].  [1]: https://lore.kernel.org/netdev/ba5c50aa-1df4-40c2-ab33-a72022c5a32e@rbox.co/ [2]: https://lore.kernel.org/netdev/20240610174906.32921-1-kuniyu@amazon.com/ [3]: https://lore.kernel.org/netdev/7603c0e6-cd5b-452b-b710-73b64bd9de26@linux.dev/ [4]: https://lore.kernel.org/netdev/CAAVpQUA+8GL_j63CaKb8hbxoL21izD58yr1NvhOhU=j+35+3og@mail.gmail.com/ [5]: https://lore.kernel.org/bpf/CAAVpQUAHijOMext28Gi10dSLuMzGYh+jK61Ujn+fZ-wvcODR2A@mail.gmail.com/ [6]: https://lore.kernel.org/bpf/dd043c69-4d03-46fe-8325-8f97101435cf@linux.dev/  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---