diff mbox series

[net-next,v2,1/2] nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_local

Message ID 476cccdcb57645784889fc82f0c7c10ff4c8b8c0.1701530776.git.code@siddh.me
State New
Headers
Series nfc: Fix UAF during datagram sending caused by missing refcounting |

Commit Message

Siddh Raman Pant Dec. 2, 2023, 3:40 p.m. UTC 
  llcp_sock_sendmsg() calls nfc_llcp_send_ui_frame() which in turn calls
nfc_alloc_send_skb(), which accesses the nfc_dev from the llcp_sock for
getting the headroom and tailroom needed for skb allocation.

Parallelly the nfc_dev can be freed, as the refcount is decreased via
nfc_free_device(), leading to a UAF reported by Syzkaller, which can
be summarized as follows:

(1) llcp_sock_sendmsg() -> nfc_llcp_send_ui_frame()
	-> nfc_alloc_send_skb() -> Dereference *nfc_dev
(2) virtual_ncidev_close() -> nci_free_device() -> nfc_free_device()
	-> put_device() -> nfc_release() -> Free *nfc_dev

When a reference to llcp_local is acquired, we do not acquire the same
for the nfc_dev. This leads to freeing even when the llcp_local is in
use, and this is the case with the UAF described above too.

Thus, when we acquire a reference to llcp_local, we should acquire a
reference to nfc_dev, and release the references appropriately later.

References for llcp_local is initialized in nfc_llcp_register_device()
(which is called by nfc_register_device()). Thus, we should acquire a
reference to nfc_dev there.

nfc_unregister_device() calls nfc_llcp_unregister_device() which in
turn calls nfc_llcp_local_put(). Thus, the reference to nfc_dev is
appropriately released later.

Reported-and-tested-by: syzbot+bbe84a4010eeea00982d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=bbe84a4010eeea00982d
Fixes: c7aa12252f51 ("NFC: Take a reference on the LLCP local pointer when creating a socket")
Signed-off-by: Siddh Raman Pant <code@siddh.me>
---
 net/nfc/llcp_core.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

Comments

Suman Ghosh Dec. 3, 2023, 4:59 p.m. UTC | #1 
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.

> 	return kref_put(&local->ref, local_release);  }
>
>@@ -959,8 +963,17 @@ static void nfc_llcp_recv_connect(struct
>nfc_llcp_local *local,
> 	}
>
> 	new_sock = nfc_llcp_sock(new_sk);
>-	new_sock->dev = local->dev;
>+
> 	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()?
>+		goto fail;
>+	}
>+
>+	new_sock->dev = local->dev;
> 	new_sock->rw = sock->rw;
> 	new_sock->miux = sock->miux;
> 	new_sock->nfc_protocol = sock->nfc_protocol; @@ -1597,7 +1610,11 @@
>int nfc_llcp_register_device(struct nfc_dev *ndev)
> 	if (local == NULL)
> 		return -ENOMEM;
>
>-	local->dev = ndev;
>+	/* Hold a reference to the device. */
>+	local->dev = nfc_get_device(ndev->idx);
>+	if (!local->dev)
>+		return -ENODEV;
[Suman] Memory leak here. Need to call kfree(local).
>+
> 	INIT_LIST_HEAD(&local->list);
> 	kref_init(&local->ref);
> 	mutex_init(&local->sdp_lock);
>--
>2.42.0
>
Siddh Raman Pant Dec. 3, 2023, 6:08 p.m. UTC | #2 
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
Suman Ghosh Dec. 4, 2023, 9:33 a.m. UTC | #3 
>>
>> >@@ -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.
diff mbox series

Patch

diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
index 1dac28136e6a..a574c653e5d2 100644
--- a/net/nfc/llcp_core.c
+++ b/net/nfc/llcp_core.c
@@ -145,6 +145,9 @@  static void nfc_llcp_socket_release(struct nfc_llcp_local *local, bool device,
 
 static struct nfc_llcp_local *nfc_llcp_local_get(struct nfc_llcp_local *local)
 {
+	if (!nfc_get_device(local->dev->idx))
+		return NULL;
+
 	kref_get(&local->ref);
 
 	return 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);
 	return kref_put(&local->ref, local_release);
 }
 
@@ -959,8 +963,17 @@  static void nfc_llcp_recv_connect(struct nfc_llcp_local *local,
 	}
 
 	new_sock = nfc_llcp_sock(new_sk);
-	new_sock->dev = local->dev;
+
 	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);
+		goto fail;
+	}
+
+	new_sock->dev = local->dev;
 	new_sock->rw = sock->rw;
 	new_sock->miux = sock->miux;
 	new_sock->nfc_protocol = sock->nfc_protocol;
@@ -1597,7 +1610,11 @@  int nfc_llcp_register_device(struct nfc_dev *ndev)
 	if (local == NULL)
 		return -ENOMEM;
 
-	local->dev = ndev;
+	/* Hold a reference to the device. */
+	local->dev = nfc_get_device(ndev->idx);
+	if (!local->dev)
+		return -ENODEV;
+
 	INIT_LIST_HEAD(&local->list);
 	kref_init(&local->ref);
 	mutex_init(&local->sdp_lock);