netdev - Re: [PATCH net v2] unix: Fix race in SOCK_SEQPACKET's unix_dgram

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:   Sat, 3 Dec 2022 13:37:29 +0300
From:   Kirill Tkhai <tkhai@...ru>
To:     Paolo Abeni <pabeni@...hat.com>, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, netdev@...r.kernel.org,
        Kuniyuki Iwashima <kuniyu@...zon.com>
Subject: Re: [PATCH net v2] unix: Fix race in SOCK_SEQPACKET's
 unix_dgram_sendmsg()

On 03.12.2022 01:43, Kirill Tkhai wrote:
> On 01.12.2022 12:30, Paolo Abeni wrote:
>> On Sun, 2022-11-27 at 01:46 +0300, Kirill Tkhai wrote:
>>> There is a race resulting in alive SOCK_SEQPACKET socket
>>> may change its state from TCP_ESTABLISHED to TCP_CLOSE:
>>>
>>> unix_release_sock(peer)                  unix_dgram_sendmsg(sk)
>>>   sock_orphan(peer)
>>>     sock_set_flag(peer, SOCK_DEAD)
>>>                                            sock_alloc_send_pskb()
>>>                                              if !(sk->sk_shutdown & SEND_SHUTDOWN)
>>>                                                OK
>>>                                            if sock_flag(peer, SOCK_DEAD)
>>>                                              sk->sk_state = TCP_CLOSE
>>>   sk->sk_shutdown = SHUTDOWN_MASK
>>>
>>>
>>> After that socket sk remains almost normal: it is able to connect, listen, accept
>>> and recvmsg, while it can't sendmsg.
>>>
>>> Since this is the only possibility for alive SOCK_SEQPACKET to change
>>> the state in such way, we should better fix this strange and potentially
>>> danger corner case.
>>>
>>> Also, move TCP_CLOSE assignment for SOCK_DGRAM sockets under state lock
>>> to fix race with unix_dgram_connect():
>>>
>>> unix_dgram_connect(other)            unix_dgram_sendmsg(sk)
>>>                                        unix_peer(sk) = NULL
>>>                                        unix_state_unlock(sk)
>>>   unix_state_double_lock(sk, other)
>>>   sk->sk_state  = TCP_ESTABLISHED
>>>   unix_peer(sk) = other
>>>   unix_state_double_unlock(sk, other)
>>>                                        sk->sk_state  = TCP_CLOSED
>>>
>>> This patch fixes both of these races.
>>>
>>> Fixes: 83301b5367a9 ("af_unix: Set TCP_ESTABLISHED for datagram sockets too")
>>
>> I don't think this commmit introduces the issues, both behavior
>> described above appear to be present even before?
> 
> 1)Hm, I pointed to the commit suggested by Kuniyuki without checking it.
> 
> Possible, the real problem commit is dc56ad7028c5 "af_unix: fix potential NULL deref in unix_dgram_connect()",
> since it added TCP_CLOSED assignment to unix_dgram_sendmsg().
> 
> 2)What do you think about initial version of fix?
> 
> https://patchwork.kernel.org/project/netdevbpf/patch/38a920a7-cfba-7929-886d-c3c6effc0c43@ya.ru/
> 
> Despite there are some arguments, I'm not still sure that v2 is better.

Rethinking again, I think v1 is better, and we don't have to introduce optimizations,
which works only in very rare race cases. So, I'm going to return to V1 version,
which is better.

Kirill