nfc: Fix UAF during datagram sending caused by missing refcounting

For connectionless transmission, llcp_sock_sendmsg() codepath will eventually call nfc_alloc_send_skb() which takes in an nfc_dev as an argument for calculating the total size for skb allocation.

virtual_ncidev_close() codepath eventually releases socket by calling nfc_llcp_socket_release() (which sets the sk->sk_state to LLCP_CLOSED) and afterwards the nfc_dev will be eventually freed.

When an ndev gets freed, llcp_sock_sendmsg() will result in an use-after-free as it

(1) doesn't have any checks in place for avoiding the datagram sending.

(2) calls nfc_llcp_send_ui_frame(), which also has a do-while loop which can race with freeing. This loop contains the call to nfc_alloc_send_skb() where we dereference the nfc_dev pointer.

nfc_dev is being freed because we do not hold a reference to it when we hold a reference to llcp_local. Thus, virtual_ncidev_close() eventually calls nfc_release() due to refcount going to 0.

Since state has to be LLCP_BOUND for datagram sending, we can bail out early in llcp_sock_sendmsg().

Please review and let me know if any errors are there, and hopefully this gets accepted.

Thanks,
Siddh

Changes in v2:
- Add net-next in patch subject.
- Removed unnecessary extra lock and hold nfc_dev ref when holding llcp_sock.
- Remove last formatting patch.
- Picked up r-b from Krzysztof for LLCP_BOUND patch.

Siddh Raman Pant (2):
nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to
llcp_local
nfc: Do not send datagram if socket state isn't LLCP_BOUND

 net/nfc/llcp_core.c | 21 +++++++++++++++++++--
 net/nfc/llcp_sock.c |  5 +++++
 2 files changed, 24 insertions(+), 2 deletions(-)

2.42.0

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.

---
 net/nfc/llcp_core.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

index 1dac28136e6a..a574c653e5d2 100644
@@ -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);

2.42.0

Hi Siddh,

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.

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

Thanks,
Siddh

[Suman] Thanks for the explanation.

As we know we cannot send the datagram (state can be set to LLCP_CLOSED by nfc_llcp_socket_release()), there is no need to proceed further.

Thus, bail out early from llcp_sock_sendmsg().

---
 net/nfc/llcp_sock.c | 5 +++++
 1 file changed, 5 insertions(+)

index 645677f84dba..819157bbb5a2 100644
@@ -796,6 +796,11 @@ static int llcp_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 	}
 
 	if (sk->sk_type == SOCK_DGRAM) {
+		if (sk->sk_state != LLCP_BOUND) {
+			release_sock(sk);
+			return -ENOTCONN;
+		}
+
 		DECLARE_SOCKADDR(struct sockaddr_nfc_llcp *, addr,
 				 msg->msg_name);
 

2.42.0