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

Message-ID: <18c30ddee40.70f9a1a945075.1438711881490299499@siddh.me>
Date: Sun, 03 Dec 2023 23:38:20 +0530
From: Siddh Raman Pant <code@...dh.me>
To: "Suman Ghosh" <sumang@...vell.com>
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

On Sun, 03 Dec 2023 22:29:39 +0530, Suman Ghosh wrote:
> Hi Siddh,
> 
> >@@ -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.

> >     new_sock->local = nfc_llcp_local_get(local);
> >+    if (!new_sock->local) {
> >+        reason = LLCP_DM_REJ;
> >+        release_sock(&sock->sk);
> >+        sock_put(&sock->sk);
> >+        sock_put(&new_sock->sk);
> [Suman] don't we need to free new_sock? nfc_llcp_sock_free()?
>
> [...]
>
> >+    local->dev = nfc_get_device(ndev->idx);
> >+    if (!local->dev)
> >+        return -ENODEV;
> [Suman] Memory leak here. Need to call kfree(local).

Yes, you are correct. Very stupid of me. Will send a v3.

Thanks,
Siddh