TLDRVuln - Cybersecurity Vulnerabilities Made Simple

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/[email protected]/

[2]: https://lore.kernel.org/netdev/[email protected]/

[3]: https://lore.kernel.org/netdev/[email protected]/

[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/[email protected]/

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