netdev - RE: [EXT] [PATCH net-next v2 1/2] nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp

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: Mon, 4 Dec 2023 09:33:34 +0000
From: Suman Ghosh <sumang@...vell.com>
To: Siddh Raman Pant <code@...dh.me>
CC: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        "David S. Miller"
	<davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        Samuel Ortiz <sameo@...ux.intel.com>,
        "syzbot+bbe84a4010eeea00982d@...kaller.appspotmail.com"
	<syzbot+bbe84a4010eeea00982d@...kaller.appspotmail.com>
Subject: RE: [EXT] [PATCH net-next v2 1/2] nfc: llcp_core: Hold a ref to
 llcp_local->dev when holding a ref to llcp_local

>>
>> >@@ -180,6 +183,7 @@ int nfc_llcp_local_put(struct nfc_llcp_local
>> >*local)
>> >     if (local == NULL)
>> >         return 0;
>> >
>> >+    nfc_put_device(local->dev);
>> [Suman] One question here, if we consider the path,
>> nfc_llcp_mac_is_down() ->
>> nfc_llcp_socket_release() -> nfc_llcp_local_put(), then inside
>> nfc_llcp_socket_release() we are already doing nfc_put_device() if
>> "sk->sk_state == LLCP_CONNECTED", with this change we are doing it
>again.
>> I guess you need to add some check to avoid that. Let me know if I am
>> missing something.
>
>The socket state is set to LLCP_CONNECTED in just two places:
>nfc_llcp_recv_connect() and nfc_llcp_recv_cc().
>
>nfc_get_device() is used prior to setting the socket state to
>LLCP_CONNECTED in nfc_llcp_recv_connect(). After that, it calls
>nfc_llcp_send_cc(), which I suppose is a connection PDU by some Google-
>fu (NFC specs is paywalled).
>
>In nfc_llcp_recv_cc(), we do not use nfc_get_device(), but since one
>must send cc (which is done in nfc_llcp_recv_connect()), I think we are
>good here.
>
>This patch change doesn't touch any other refcounting. We increment the
>refcount whenever we get the local, and decrement when we put it.
>nfc_llcp_find_local() involves getting it, so all users of that function
>increment the refcount, which is also the case with
>nfc_llcp_mac_is_down(). The last nfc_llcp_local_put() then correctly
>decrements the refcount.
>
>If there is indeed a refcount error due to LLCP_CONNECTED, it probably
>exists without this patch too.
[Suman] Thanks for the explanation.