Re: [PATCH net] af_unix: fix races in sk_peer_pid and sk_peer_cred accesses

在 2022/3/24 17:04, Kuniyuki Iwashima 写道:

From:   "wanghai (M)" <redacted>
Date:   Thu, 24 Mar 2022 16:03:31 +0800

quoted

在 2021/9/30 6:57, Eric Dumazet 写道:

quoted

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

   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.

To simply avoid dead lock.  These locks must be acquired in the same
order.  The smaller address lock is acquired first, then larger one.

   e.g.) CPU-A calls copy_peercred(sk-A, sk-B), and
         CPU-B calls copy_peercred(sk-B, sk-A).

There are some implementations like this:

   $ grep -rn double_lock

I got it, thank you for your patient explanation.

quoted

Any feedback would be appreciated, thanks.

quoted

+	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

.

-- 
Wang Hai