netdev - Re: [PATCH net] nfc: avoid potential race condition

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:   Mon, 27 Sep 2021 09:44:08 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Samuel Ortiz <sameo@...ux.intel.com>,
        "David S. Miller" <davem@...emloft.net>,
        "John W. Linville" <linville@...driver.com>,
        netdev@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH net] nfc: avoid potential race condition

On 24/09/2021 22:14, Jakub Kicinski wrote:
> On Fri, 24 Sep 2021 10:21:33 +0200 Krzysztof Kozlowski wrote:
>> On 23/09/2021 14:22, Dan Carpenter wrote:
>>> On Thu, Sep 23, 2021 at 09:26:51AM +0200, Krzysztof Kozlowski wrote:  
>>>> On 23/09/2021 08:50, Dan Carpenter wrote:  
>>  [...]  
>>>>
>>>> I think the difference between this llcp_sock code and above transport,
>>>> is lack of writer to llcp_sock->local with whom you could race.
>>>>
>>>> Commits c0cfa2d8a788fcf4 and 6a2c0962105ae8ce causing the
>>>> multi-transport race show nicely assigns to vsk->transport when module
>>>> is unloaded.
>>>>
>>>> Here however there is no writer to llcp_sock->local, except bind and
>>>> connect and their error paths. The readers which you modify here, have
>>>> to happen after bind/connect. You cannot have getsockopt() or release()
>>>> before bind/connect, can you? Unless you mean here the bind error path,
>>>> where someone calls getsockopt() in the middle of bind()? Is it even
>>>> possible?
>>>>  
>>>
>>> I don't know if this is a real issue either.
>>>
>>> Racing with bind would be harmless.  The local pointer would be NULL and
>>> it would return harmlessly.  You would have to race with release and
>>> have a third trying to release local devices.  (Again that might be
>>> wild imagination.  It may not be possible).  
>>
>> Indeed. The code looks reasonable, though, so even if race is not really
>> reproducible:
>>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
> 
> Would you mind making a call if this is net (which will mean stable) or
> net-next material (without the Fixes tags) and reposting? Thanks! :)

Hi Jakub,

Material is net-next. However I don't understand why it should be
without "Fixes" in such case?

The material going to current release (RC, so I understood: net), should
fix only issues introduced in current merge window. Linus made it clear
several times.

The issue here was introduced long time ago, not in current merge
window, however it is still an issue to fix. It's still a bug which
should have a commit with "Fixes" for all the stable tress and
downstream distros relying on stable kernels. Also for some statistics
on LWN.


Best regards,
Krzysztof