Skip to content

Commit 584f6c7

Browse files
jrfastabryanbsull
authored andcommitted
bpf, sockmap: af_unix stream sockets need to hold ref for pair sock
JIRA: https://issues.redhat.com/browse/RHEL-94688 Conflict(s): - Relocation errors in: - include/linux/skmsg.h - include/net/af_unix.h AF_UNIX stream sockets are a paired socket. So sending on one of the pairs will lookup the paired socket as part of the send operation. It is possible however to put just one of the pairs in a BPF map. This currently increments the refcnt on the sock in the sockmap to ensure it is not free'd by the stack before sockmap cleans up its state and stops any skbs being sent/recv'd to that socket. But we missed a case. If the peer socket is closed it will be free'd by the stack. However, the paired socket can still be referenced from BPF sockmap side because we hold a reference there. Then if we are sending traffic through BPF sockmap to that socket it will try to dereference the free'd pair in its send logic creating a use after free. And following splat: [59.900375] BUG: KASAN: slab-use-after-free in sk_wake_async+0x31/0x1b0 [59.901211] Read of size 8 at addr ffff88811acbf060 by task kworker/1:2/954 [...] [59.905468] Call Trace: [59.905787] <TASK> [59.906066] dump_stack_lvl+0x130/0x1d0 [59.908877] print_report+0x16f/0x740 [59.910629] kasan_report+0x118/0x160 [59.912576] sk_wake_async+0x31/0x1b0 [59.913554] sock_def_readable+0x156/0x2a0 [59.914060] unix_stream_sendmsg+0x3f9/0x12a0 [59.916398] sock_sendmsg+0x20e/0x250 [59.916854] skb_send_sock+0x236/0xac0 [59.920527] sk_psock_backlog+0x287/0xaa0 To fix let BPF sockmap hold a refcnt on both the socket in the sockmap and its paired socket. It wasn't obvious how to contain the fix to bpf_unix logic. The primarily problem with keeping this logic in bpf_unix was: In the sock close() we could handle the deref by having a close handler. But, when we are destroying the psock through a map delete operation we wouldn't have gotten any signal thorugh the proto struct other than it being replaced. If we do the deref from the proto replace its too early because we need to deref the sk_pair after the backlog worker has been stopped. Given all this it seems best to just cache it at the end of the psock and eat 8B for the af_unix and vsock users. Notice dgram sockets are OK because they handle locking already. Fixes: 94531cf ("af_unix: Add unix_stream_proto for sockmap") Signed-off-by: John Fastabend <john.fastabend@gmail.com> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com> Link: https://lore.kernel.org/bpf/20231129012557.95371-2-john.fastabend@gmail.com (cherry picked from commit 8866730)
1 parent cde61aa commit 584f6c7

File tree

5 files changed

+9
-2
lines changed

5 files changed

+9
-2
lines changed

include/linux/skmsg.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ struct sk_psock {
106106
struct mutex work_mutex;
107107
struct sk_psock_work_state work_state;
108108
struct work_struct work;
109+
struct sock *sk_pair;
109110
struct rcu_work rwork;
110111
};
111112

include/net/af_unix.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ static inline void unix_state_lock_nested(struct sock *sk,
8888
spin_lock_nested(&unix_sk(sk)->lock, subclass);
8989
}
9090

91+
#define unix_peer(sk) (unix_sk(sk)->peer)
9192
#define peer_wait peer_wq.wait
9293

9394
long unix_inq_len(struct sock *sk);

net/core/skmsg.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -833,6 +833,8 @@ static void sk_psock_destroy(struct work_struct *work)
833833

834834
if (psock->sk_redir)
835835
sock_put(psock->sk_redir);
836+
if (psock->sk_pair)
837+
sock_put(psock->sk_pair);
836838
sock_put(psock->sk);
837839
kfree(psock);
838840
}

net/unix/af_unix.c

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -205,8 +205,6 @@ static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff *skb)
205205
}
206206
#endif /* CONFIG_SECURITY_NETWORK */
207207

208-
#define unix_peer(sk) (unix_sk(sk)->peer)
209-
210208
static inline int unix_our_peer(struct sock *sk, struct sock *osk)
211209
{
212210
return unix_peer(osk) == sk;

net/unix/unix_bpf.c

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,12 +160,17 @@ int unix_dgram_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool re
160160

161161
int unix_stream_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool restore)
162162
{
163+
struct sock *sk_pair;
164+
163165
if (restore) {
164166
sk->sk_write_space = psock->saved_write_space;
165167
sock_replace_proto(sk, psock->sk_proto);
166168
return 0;
167169
}
168170

171+
sk_pair = unix_peer(sk);
172+
sock_hold(sk_pair);
173+
psock->sk_pair = sk_pair;
169174
unix_stream_bpf_check_needs_rebuild(psock->sk_proto);
170175
sock_replace_proto(sk, &unix_stream_bpf_prot);
171176
return 0;

0 commit comments

Comments
 (0)