netdev - Re: [PATCH net] af_unix: Fix a data-race in unix_dgram_peer_wake

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: <20220607141453.48225-1-kuniyu@amazon.com>
Date:   Tue, 7 Jun 2022 07:14:53 -0700
From:   Kuniyuki Iwashima <kuniyu@...zon.com>
To:     <pabeni@...hat.com>
CC:     <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <kuni1840@...il.com>, <kuniyu@...zon.com>,
        <netdev@...r.kernel.org>, <rweikusat@...ileactivedefense.com>
Subject: Re: [PATCH net] af_unix: Fix a data-race in unix_dgram_peer_wake_me().

From:   Paolo Abeni <pabeni@...hat.com>
Date:   Tue, 07 Jun 2022 12:35:13 +0200
> On Sun, 2022-06-05 at 16:23 -0700, Kuniyuki Iwashima wrote:
>> unix_dgram_poll() calls unix_dgram_peer_wake_me() without `other`'s
>> lock held and check if its receive queue is full.  Here we need to
>> use unix_recvq_full_lockless() instead of unix_recvq_full(), otherwise
>> KCSAN will report a data-race.
>> 
>> Fixes: 7d267278a9ec ("unix: avoid use-after-free in ep_remove_wait_queue")
>> Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
>> ---
>> As Eric noted in commit 04f08eb44b501, I think rest of unix_recvq_full()
>> can be turned into the lockless version.  After this merge window, I can
>> send a follow-up patch if there is no objection.
> 
> It looks like replacing the remaining instances of unix_recvq_full()
> with unix_recvq_full_lockless() should be safe, but I'm wondering if
> doing that while retaining the current state lock scope it's worthy?!? 
> 
> It may trick later readers of the relevant code to think that such code
> may be reached without a lock. Or are you suggesting to additionally
> shrink the state lock scope? that latter part looks much more tricky,
> IMHO.

I thought removing unix_recvq_full() will prevent the same mistakes, but
I agree that it is confusing for later readers.

Thank you!