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

Message-ID: <6e1cb672-cfe4-8adb-e618-1915f04562c0@ya.ru>
Date:   Mon, 5 Dec 2022 20:07:10 +0300
From:   Kirill Tkhai <tkhai@...ru>
To:     Paolo Abeni <pabeni@...hat.com>,
        "Iwashima, Kuniyuki" <kuniyu@...zon.co.jp>
Cc:     "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net v2] unix: Fix race in SOCK_SEQPACKET's
 unix_dgram_sendmsg()

On 05.12.2022 12:22, Paolo Abeni wrote:
> On Fri, 2022-12-02 at 23:18 +0000, Iwashima, Kuniyuki wrote:
>>
>>> On Dec 3, 2022, at 7:44, Kirill Tkhai <tkhai@...ru> 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().
>>
>> The commit just moved the assignment.
>>
>> Note unix_dgram_disconnected() is called for SOCK_SEQPACKET 
>> after releasing the lock, and 83301b5367a9 introduced the 
>> TCP_CLOSE assignment.
> 
> I'm sorry for the back and forth, I think I initally misread the code.
> I agree 83301b5367a9 is good fixes tag.
> 
>>> 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.
> 
> v1 introduces quite a few behavior changes (different error code,
> different cleanup schema) that could be IMHO more risky for a stable
> patch. I suggest to pick the minimal change that addresses the issue
> (v2 in this case).

Hm, not exactly. EPIPE is regular return value, which is normally returned from
unix_dgram_sendmsg()->sock_alloc_send_pskb (see SEND_SHUTDOWN check).
ECONNREFUSED is a race case return value, it does not returned normally.

What different cleanup scheme do you mean? IMO, there is the same behavior
as we get, when race is failed, and sock_alloc_send_pskb() returns EPIPE as in regular case.