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] [day] [month] [year] [list]

Date:   Thu, 24 Mar 2022 19:15:49 +0800
From:   "wanghai (M)" <wanghai38@...wei.com>
To:     Kuniyuki Iwashima <kuniyu@...zon.co.jp>
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


在 2022/3/24 17:04, Kuniyuki Iwashima 写道:
> 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
I got it, thank you for your patient explanation.
>
>
>> 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
> .
>
-- 
Wang Hai