Re: [PATCH net] af_unix: fix races in sk_peer_pid and sk_peer_cred accesses
flat view
From: wanghai (M) <hidden>
Date: 2022-03-24 08:03:39
在 2021/9/30 6:57, Eric Dumazet 写道:
From: Eric Dumazet <redacted>
Jann Horn reported that SO_PEERCRED and SO_PEERGROUPS implementations
are racy, as af_unix can concurrently change sk_peer_pid and sk_peer_cred.
In order to fix this issue, this patch adds a new spinlock that needs
to be used whenever these fields are read or written.
Jann also pointed out that l2cap_sock_get_peer_pid_cb() is currently
reading sk->sk_peer_pid which makes no sense, as this field
is only possibly set by AF_UNIX sockets.
We will have to clean this in a separate patch.
This could be done by reverting b48596d1dc25 "Bluetooth: L2CAP: Add get_peer_pid callback"
or implementing what was truly expected.
Fixes: 109f6e39fa07 ("af_unix: Allow SO_PEERCRED to work across namespaces.")
Signed-off-by: Eric Dumazet <redacted>
Reported-by: Jann Horn <jannh@google.com>
Cc: Eric W. Biederman <redacted>
Cc: Luiz Augusto von Dentz <redacted>
Cc: Marcel Holtmann <marcel@holtmann.org>
---...
quoted hunk ↗ jump to hunk
static void copy_peercred(struct sock *sk, struct sock *peersk) { - put_pid(sk->sk_peer_pid); - if (sk->sk_peer_cred) - put_cred(sk->sk_peer_cred); + const struct cred *old_cred; + struct pid *old_pid; + + if (sk < peersk) { + spin_lock(&sk->sk_peer_lock); + spin_lock_nested(&peersk->sk_peer_lock, SINGLE_DEPTH_NESTING); + } else { + spin_lock(&peersk->sk_peer_lock); + spin_lock_nested(&sk->sk_peer_lock, SINGLE_DEPTH_NESTING); + }
Hi, ALL. I'm sorry to bother you. This patch adds sk_peer_lock to solve the problem that af_unix may concurrently change sk_peer_pid and sk_peer_cred. I am confused as to why the order of locks is needed here based on the address size of sk and peersk. Any feedback would be appreciated, thanks.
quoted hunk ↗ jump to hunk
+ old_pid = sk->sk_peer_pid; + old_cred = sk->sk_peer_cred; sk->sk_peer_pid = get_pid(peersk->sk_peer_pid); sk->sk_peer_cred = get_cred(peersk->sk_peer_cred); + + spin_unlock(&sk->sk_peer_lock); + spin_unlock(&peersk->sk_peer_lock); + + put_pid(old_pid); + put_cred(old_cred); } static int unix_listen(struct socket *sock, int backlog)
-- Wang Hai