Commit 65c6ef02 authored Dec 19, 2023 by Siddh Raman Pant Committed by Greg Kroah-Hartman Jan 15, 2024

nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_local



[ Upstream commit c95f919567d6f1914f13350af61a1b044ac85014 ]

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: c7aa1225 ("NFC: Take a reference on the LLCP local pointer when creating a socket")
Reviewed-by: Suman Ghosh <sumang@marvell.com>
Signed-off-by: Siddh Raman Pant <code@siddh.me>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sasha Levin <sashal@kernel.org>

parent 4410df70

net/nfc/llcp_core.c

+36 −3

Original line number	Diff line number	Diff line
		@@ -147,6 +147,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;
		@@ -179,10 +186,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,
		@@ -968,8 +983,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;
		@@ -1607,7 +1631,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);