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

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

[Siddh Raman Pant]

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.

Thanks,
Siddh

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 | 72 +++++++++++++++++++++++++++++----------------
 net/nfc/llcp_sock.c |  5 ++++
 2 files changed, 52 insertions(+), 25 deletions(-)

-- 
2.42.0

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

[Siddh Raman Pant]

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")
Reviewed-by: Suman Ghosh <sumang@marvell.com>
Signed-off-by: Siddh Raman Pant <code@siddh.me>
---
 net/nfc/llcp_core.c | 72 +++++++++++++++++++++++++++++----------------
 1 file changed, 47 insertions(+), 25 deletions(-)

diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
index 1dac28136e6a..2f77200a3720 100644
--- a/net/nfc/llcp_core.c
+++ b/net/nfc/llcp_core.c
@@ -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,
@@ -901,7 +916,7 @@ static void nfc_llcp_recv_connect(struct nfc_llcp_local *local,
 
 	if (dsap != LLCP_SAP_SDP) {
 		sock = nfc_llcp_sock_get(local, dsap, LLCP_SAP_SDP);
-		if (sock == NULL || sock->sk.sk_state != LLCP_LISTEN) {
+		if (!sock || sock->sk.sk_state != LLCP_LISTEN) {
 			reason = LLCP_DM_NOBOUND;
 			goto fail;
 		}
@@ -910,7 +925,7 @@ static void nfc_llcp_recv_connect(struct nfc_llcp_local *local,
 		size_t sn_len;
 
 		sn = nfc_llcp_connect_sn(skb, &sn_len);
-		if (sn == NULL) {
+		if (!sn) {
 			reason = LLCP_DM_NOBOUND;
 			goto fail;
 		}
@@ -918,7 +933,7 @@ static void nfc_llcp_recv_connect(struct nfc_llcp_local *local,
 		pr_debug("Service name length %zu\n", sn_len);
 
 		sock = nfc_llcp_sock_get_sn(local, sn, sn_len);
-		if (sock == NULL) {
+		if (!sock) {
 			reason = LLCP_DM_NOBOUND;
 			goto fail;
 		}
@@ -928,39 +943,31 @@ static void nfc_llcp_recv_connect(struct nfc_llcp_local *local,
 
 	parent = &sock->sk;
 
-	if (sk_acceptq_is_full(parent)) {
-		reason = LLCP_DM_REJ;
-		release_sock(&sock->sk);
-		sock_put(&sock->sk);
-		goto fail;
-	}
+	if (sk_acceptq_is_full(parent))
+		goto fail_put_sock;
 
 	if (sock->ssap == LLCP_SDP_UNBOUND) {
 		u8 ssap = nfc_llcp_reserve_sdp_ssap(local);
 
 		pr_debug("First client, reserving %d\n", ssap);
 
-		if (ssap == LLCP_SAP_MAX) {
-			reason = LLCP_DM_REJ;
-			release_sock(&sock->sk);
-			sock_put(&sock->sk);
-			goto fail;
-		}
+		if (ssap == LLCP_SAP_MAX)
+			goto fail_put_sock;
 
 		sock->ssap = ssap;
 	}
 
 	new_sk = nfc_llcp_sock_alloc(NULL, parent->sk_type, GFP_ATOMIC, 0);
-	if (new_sk == NULL) {
-		reason = LLCP_DM_REJ;
-		release_sock(&sock->sk);
-		sock_put(&sock->sk);
-		goto fail;
-	}
+	if (!new_sk)
+		goto fail_put_sock;
 
 	new_sock = nfc_llcp_sock(new_sk);
-	new_sock->dev = local->dev;
+
 	new_sock->local = nfc_llcp_local_get(local);
+	if (!new_sock->local)
+		goto fail_free_new_sock;
+
+	new_sock->dev = local->dev;
 	new_sock->rw = sock->rw;
 	new_sock->miux = sock->miux;
 	new_sock->nfc_protocol = sock->nfc_protocol;
@@ -1004,8 +1011,14 @@ static void nfc_llcp_recv_connect(struct nfc_llcp_local *local,
 
 	return;
 
+fail_free_new_sock:
+	sock_put(&new_sock->sk);
+	nfc_llcp_sock_free(new_sock);
+fail_put_sock:
+	reason = LLCP_DM_REJ;
+	release_sock(&sock->sk);
+	sock_put(&sock->sk);
 fail:
-	/* Send DM */
 	nfc_llcp_send_dm(local, dsap, ssap, reason);
 }
 
@@ -1597,7 +1610,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);
-- 
2.42.0

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

[Krzysztof Kozlowski]

On 12/12/2023 19:49, Siddh Raman Pant wrote:
> 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")
> Reviewed-by: Suman Ghosh <sumang@marvell.com>
> Signed-off-by: Siddh Raman Pant <code@siddh.me>
> ---
>  net/nfc/llcp_core.c | 72 +++++++++++++++++++++++++++++----------------
>  1 file changed, 47 insertions(+), 25 deletions(-)
> 
> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
> index 1dac28136e6a..2f77200a3720 100644
> --- a/net/nfc/llcp_core.c
> +++ b/net/nfc/llcp_core.c
> @@ -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,
> @@ -901,7 +916,7 @@ static void nfc_llcp_recv_connect(struct nfc_llcp_local *local,
>  
>  	if (dsap != LLCP_SAP_SDP) {
>  		sock = nfc_llcp_sock_get(local, dsap, LLCP_SAP_SDP);
> -		if (sock == NULL || sock->sk.sk_state != LLCP_LISTEN) {
> +		if (!sock || sock->sk.sk_state != LLCP_LISTEN) {

This is unrelated change. Please keep all cleanups separate from fixes.

>  			reason = LLCP_DM_NOBOUND;
>  			goto fail;
>  		}
> @@ -910,7 +925,7 @@ static void nfc_llcp_recv_connect(struct nfc_llcp_local *local,
>  		size_t sn_len;
>  
>  		sn = nfc_llcp_connect_sn(skb, &sn_len);
> -		if (sn == NULL) {
> +		if (!sn) {

Not related.

>  			reason = LLCP_DM_NOBOUND;
>  			goto fail;
>  		}
> @@ -918,7 +933,7 @@ static void nfc_llcp_recv_connect(struct nfc_llcp_local *local,
>  		pr_debug("Service name length %zu\n", sn_len);
>  
>  		sock = nfc_llcp_sock_get_sn(local, sn, sn_len);
> -		if (sock == NULL) {
> +		if (!sock) {

Not related.

>  			reason = LLCP_DM_NOBOUND;
>  			goto fail;
>  		}
> @@ -928,39 +943,31 @@ static void nfc_llcp_recv_connect(struct nfc_llcp_local *local,
>  
>  	parent = &sock->sk;
>  
> -	if (sk_acceptq_is_full(parent)) {
> -		reason = LLCP_DM_REJ;
> -		release_sock(&sock->sk);
> -		sock_put(&sock->sk);
> -		goto fail;
> -	}
> +	if (sk_acceptq_is_full(parent))
> +		goto fail_put_sock;

I would argue that you reshuffle here more code than needed for the fix.

This should fix only missing dev reference, not reshuffle code. It's a
bugfix, not cleanup.

>  
>  	if (sock->ssap == LLCP_SDP_UNBOUND) {
>  		u8 ssap = nfc_llcp_reserve_sdp_ssap(local);
>  
>  		pr_debug("First client, reserving %d\n", ssap);
>  
> -		if (ssap == LLCP_SAP_MAX) {
> -			reason = LLCP_DM_REJ;
> -			release_sock(&sock->sk);
> -			sock_put(&sock->sk);
> -			goto fail;
> -		}
> +		if (ssap == LLCP_SAP_MAX)
> +			goto fail_put_sock;
>  
>  		sock->ssap = ssap;
>  	}
>  


Best regards,
Krzysztof

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

[Siddh Raman Pant]

On Wed, 13 Dec 2023 13:10:16 +0530, Krzysztof Kozlowski wrote:
> > -	if (sk_acceptq_is_full(parent)) {
> > -		reason = LLCP_DM_REJ;
> > -		release_sock(&sock->sk);
> > -		sock_put(&sock->sk);
> > -		goto fail;
> > -	}
> > +	if (sk_acceptq_is_full(parent))
> > +		goto fail_put_sock;
> 
> I would argue that you reshuffle here more code than needed for the fix.
> 
> This should fix only missing dev reference, not reshuffle code. It's a
> bugfix, not cleanup.

So this should not be done? I did it because you told to extend the
cleanup label in v3 discussion.

Thanks,
Siddh

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

[Krzysztof Kozlowski]

On 14/12/2023 18:23, Siddh Raman Pant wrote:
> On Wed, 13 Dec 2023 13:10:16 +0530, Krzysztof Kozlowski wrote:
>>> -	if (sk_acceptq_is_full(parent)) {
>>> -		reason = LLCP_DM_REJ;
>>> -		release_sock(&sock->sk);
>>> -		sock_put(&sock->sk);
>>> -		goto fail;
>>> -	}
>>> +	if (sk_acceptq_is_full(parent))
>>> +		goto fail_put_sock;
>>
>> I would argue that you reshuffle here more code than needed for the fix.
>>
>> This should fix only missing dev reference, not reshuffle code. It's a
>> bugfix, not cleanup.
> 
> So this should not be done? I did it because you told to extend the
> cleanup label in v3 discussion.

It can be done but not in the same commit. You must not combine fixes
with other changes. Also, each commit is one logical change.

Best regards,
Krzysztof

[PATCH net-next v5 2/2] nfc: Do not send datagram if socket state isn't LLCP_BOUND

[Siddh Raman Pant]

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().

Signed-off-by: Siddh Raman Pant <code@siddh.me>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Suman Ghosh <sumang@marvell.com>
---
 net/nfc/llcp_sock.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
index 645677f84dba..819157bbb5a2 100644
--- a/net/nfc/llcp_sock.c
+++ b/net/nfc/llcp_sock.c
@@ -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