netdev - Re: [PATCH net] af_unix: fix races in sk_peer_pid and sk_peer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 24 Mar 2022 18:04:55 +0900
From:   Kuniyuki Iwashima <kuniyu@...zon.co.jp>
To:     <wanghai38@...wei.com>
CC:     <davem@...emloft.net>, <ebiederm@...ssion.com>,
        <edumazet@...gle.com>, <eric.dumazet@...il.com>,
        <jannh@...gle.com>, <kuba@...nel.org>, <luiz.von.dentz@...el.com>,
        <marcel@...tmann.org>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net] af_unix: fix races in sk_peer_pid and sk_peer_cred accesses

From:   "wanghai (M)" <wanghai38@...wei.com>
Date:   Thu, 24 Mar 2022 16:03:31 +0800
> 在 2021/9/30 6:57, Eric Dumazet 写道:
>> From: Eric Dumazet <edumazet@...gle.com>
>>
>> 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 <edumazet@...gle.com>
>> Reported-by: Jann Horn <jannh@...gle.com>
>> Cc: Eric W. Biederman <ebiederm@...ssion.com>
>> Cc: Luiz Augusto von Dentz <luiz.von.dentz@...el.com>
>> Cc: Marcel Holtmann <marcel@...tmann.org>
>> ---
> ...
>>   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


> 
> Any feedback would be appreciated, thanks.
>> +	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