Commit dff90e4a authored Dec 25, 2023 by David S. Miller

Merge branch 'nfc-refcounting'



@ 2023-12-19 17:49 Siddh Raman Pant
  2023-12-19 17:49 ` [PATCH net-next v7 1/2] nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_local Siddh Raman Pant
  2023-12-19 17:49 ` [PATCH net-next v7 2/2] nfc: Do not send datagram if socket state isn't LLCP_BOUND Siddh Raman Pant
  0 siblings, 2 replies; 4+ messages in thread
Siddh Raman Pant says:

====================
[PATCH net-next v7 0/2] nfc: Fix UAF during datagram sending caused by missing refcounting

Changes in v7:
- Stupidly reverted ordering in recv() too, fix that.
- Remove redundant call to nfc_llcp_sock_free().

Changes in v6:
- Revert label introduction from v4, and thus also v5 entirely.

Changes in v5:
- Move reason = LLCP_DM_REJ under the fail_put_sock label.
- Checkpatch now warns about == NULL check for new_sk, so fix that,
  and also at other similar places in the same function.

Changes in v4:
- Fix put ordering and comments.
- Separate freeing in recv() into end labels.
- Remove obvious comment and add reasoning.
- Picked up r-bs by Suman.

Changes in v3:
- Fix missing freeing statements.

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.

---

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.
====================

Signed-off-by: David S. Miller <davem@davemloft.net>

parents 7c5e046b 6ec0d752

net/nfc/llcp_core.c

+36 −3

Original line number	Diff line number	Diff line
		@@ -145,6 +145,13 @@ 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)
		{
		/* Since using nfc_llcp_local may result in usage of nfc_dev, whenever
		* we hold a reference to local, we also need to hold a reference to
		* the device to avoid UAF.
		*/
		if (!nfc_get_device(local->dev->idx))
		return NULL;

		kref_get(&local->ref);

		return local;
		@@ -177,10 +184,18 @@ static void local_release(struct kref *ref)

		int nfc_llcp_local_put(struct nfc_llcp_local *local)
		{
		struct nfc_dev *dev;
		int ret;

		if (local == NULL)
		return 0;

		return kref_put(&local->ref, local_release);
		dev = local->dev;

		ret = kref_put(&local->ref, local_release);
		nfc_put_device(dev);

		return ret;
		}

		static struct nfc_llcp_sock nfc_llcp_sock_get(struct nfc_llcp_local local,
		@@ -959,8 +974,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;
		sock_put(&new_sock->sk);
		release_sock(&sock->sk);
		sock_put(&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 +1621,16 @@ int nfc_llcp_register_device(struct nfc_dev *ndev)
		if (local == NULL)
		return -ENOMEM;

		local->dev = ndev;
		/* As we are going to initialize local's refcount, we need to get the
		* nfc_dev to avoid UAF, otherwise there is no point in continuing.
		* See nfc_llcp_local_get().
		*/
		local->dev = nfc_get_device(ndev->idx);
		if (!local->dev) {
		kfree(local);
		return -ENODEV;
		}

		INIT_LIST_HEAD(&local->list);
		kref_init(&local->ref);
		mutex_init(&local->sdp_lock);

net/nfc/llcp_sock.c

+5 −0

Original line number	Diff line number	Diff line
		@@ -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);