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

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

[Siddh Raman Pant]

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

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

-- 
2.42.0

[PATCH net-next v3 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")
Signed-off-by: Siddh Raman Pant <code@siddh.me>
---
 net/nfc/llcp_core.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
index 1dac28136e6a..9d45ce6dcdca 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,18 @@ 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);
+		nfc_llcp_sock_free(new_sock);
+		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 +1611,13 @@ 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) {
+		kfree(local);
+		return -ENODEV;
+	}
+
 	INIT_LIST_HEAD(&local->list);
 	kref_init(&local->ref);
 	mutex_init(&local->sdp_lock);
-- 
2.42.0

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

[Suman Ghosh]

>Reported-and-tested-by:
>syzbot+bbe84a4010eeea00982d@syzkaller.appspotmail.com
>Closes: https://urldefense.proofpoint.com/v2/url?u=https-
>3A__syzkaller.appspot.com_bug-3Fextid-
>3Dbbe84a4010eeea00982d&d=DwIDAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=7si3Xn9Ly-
>Se1a655kvEPIYU0nQ9HPeN280sEUv5ROU&m=Y51GaQT3fKSNuGmD-
>9dRkeRq59DJDzTJ7yEh86VaPxCJP7TBLuK6eYKAw-fnG7-
>c&s=NUdyCeltsQ_6_rZPnNvgfyPleIYGMaSIN1sC1zqpV7Y&e=
>Fixes: c7aa12252f51 ("NFC: Take a reference on the LLCP local pointer
>when creating a socket")
>Signed-off-by: Siddh Raman Pant <code@siddh.me>
>---
Reviewed-by: Suman Ghosh <sumang@marvell.com>

> net/nfc/llcp_core.c | 24 ++++++++++++++++++++++--
> 1 file changed, 22 insertions(+), 2 deletions(-)

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

[Krzysztof Kozlowski]

On 04/12/2023 14:08, 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")
> Signed-off-by: Siddh Raman Pant <code@siddh.me>
> ---
>  net/nfc/llcp_core.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
> index 1dac28136e6a..9d45ce6dcdca 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);

Mismatched order with get. Unwinding is always in reversed order. Or
maybe other order is here on purpose? Then it needs to be explained.

>  	return kref_put(&local->ref, local_release);
>  }
>  
> @@ -959,8 +963,18 @@ 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) {

There is already an cleanup path/label, so extend it. Existing code
needs some improvements in that matter as well.

> +		reason = LLCP_DM_REJ;
> +		release_sock(&sock->sk);
> +		sock_put(&sock->sk);
> +		sock_put(&new_sock->sk);
> +		nfc_llcp_sock_free(new_sock);
> +		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 +1611,13 @@ int nfc_llcp_register_device(struct nfc_dev *ndev)
>  	if (local == NULL)
>  		return -ENOMEM;
>  
> -	local->dev = ndev;
> +	/* Hold a reference to the device. */

That's obvious. Instead write something not obvious - why you call
nfc_get_device() while not incrementing reference to llcp_local.

> +	local->dev = nfc_get_device(ndev->idx);

This looks confusing. If you can access ndev->idx, then ndev reference
was already increased. In such case iterating through all devices to
find it, is unnecessary and confusing.



Best regards,
Krzysztof

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

[Siddh Raman Pant]

On Tue, 05 Dec 2023 22:10:00 +0530, Krzysztof Kozlowski wrote:
> > @@ -180,6 +183,7 @@ int nfc_llcp_local_put(struct nfc_llcp_local *local)
> >  	if (local == NULL)
> >  		return 0;
> >  
> > +	nfc_put_device(local->dev);
> 
> Mismatched order with get. Unwinding is always in reversed order. Or
> maybe other order is here on purpose? Then it needs to be explained.

Yes, local_release() will free local, so local->dev cannot be accessed.
Will add a comment.

> > @@ -959,8 +963,18 @@ 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) {
> 
> There is already an cleanup path/label, so extend it. Existing code
> needs some improvements in that matter as well.

Sure.

> > +		reason = LLCP_DM_REJ;
> > +		release_sock(&sock->sk);
> > +		sock_put(&sock->sk);
> > +		sock_put(&new_sock->sk);
> > +		nfc_llcp_sock_free(new_sock);
> > +		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 +1611,13 @@ int nfc_llcp_register_device(struct nfc_dev *ndev)
> >  	if (local == NULL)
> >  		return -ENOMEM;
> >  
> > -	local->dev = ndev;
> > +	/* Hold a reference to the device. */
> 
> That's obvious. Instead write something not obvious - why you call
> nfc_get_device() while not incrementing reference to llcp_local.

Should I move it after kref_init()? Here, I'm bailing out early so we
don't have to do unnecessary init first, and the rest of the function
will never fail.

> > +	local->dev = nfc_get_device(ndev->idx);
> 
> This looks confusing. If you can access ndev->idx, then ndev reference
> was already increased. In such case iterating through all devices to
> find it, is unnecessary and confusing.

I agree, it was something I thought about as well. There should be a
new function for refcount increment. Maybe the existing one could be
renamed to nfc_get_device_from_idx() and a new nfc_get_device() be
defined.

I didn't want to introduce improvement patches in this UAF series, as
that would be an independent unit of change.

Thanks,
Siddh

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

[Siddh Raman Pant]

On Tue, 05 Dec 2023 22:51:17 +0530, Siddh Raman Pant wrote:
> I agree, it was something I thought about as well. There should be a
> new function for refcount increment. Maybe the existing one could be
> renamed to nfc_get_device_from_idx() and a new nfc_get_device() be
> defined.

Or nfc_find_device() instead of nfc_get_device_from_idx().

Thanks,
Siddh

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

[Krzysztof Kozlowski]

On 05/12/2023 18:21, Siddh Raman Pant wrote:
> On Tue, 05 Dec 2023 22:10:00 +0530, Krzysztof Kozlowski wrote:
>>> @@ -180,6 +183,7 @@ int nfc_llcp_local_put(struct nfc_llcp_local *local)
>>>  	if (local == NULL)
>>>  		return 0;
>>>  
>>> +	nfc_put_device(local->dev);
>>
>> Mismatched order with get. Unwinding is always in reversed order. Or
>> maybe other order is here on purpose? Then it needs to be explained.
> 
> Yes, local_release() will free local, so local->dev cannot be accessed.
> Will add a comment.

So the problem is just storing the pointer? That's not really the valid
reason.

> 
>>> @@ -959,8 +963,18 @@ 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) {
>>
>> There is already an cleanup path/label, so extend it. Existing code
>> needs some improvements in that matter as well.
> 
> Sure.
> 
>>> +		reason = LLCP_DM_REJ;
>>> +		release_sock(&sock->sk);
>>> +		sock_put(&sock->sk);
>>> +		sock_put(&new_sock->sk);
>>> +		nfc_llcp_sock_free(new_sock);
>>> +		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 +1611,13 @@ int nfc_llcp_register_device(struct nfc_dev *ndev)
>>>  	if (local == NULL)
>>>  		return -ENOMEM;
>>>  
>>> -	local->dev = ndev;
>>> +	/* Hold a reference to the device. */
>>
>> That's obvious. Instead write something not obvious - why you call
>> nfc_get_device() while not incrementing reference to llcp_local.
> 
> Should I move it after kref_init()? Here, I'm bailing out early so we
> don't have to do unnecessary init first, and the rest of the function
> will never fail.

I meant, comment is obvious.

> 
>>> +	local->dev = nfc_get_device(ndev->idx);
>>
>> This looks confusing. If you can access ndev->idx, then ndev reference
>> was already increased. In such case iterating through all devices to
>> find it, is unnecessary and confusing.
> 
> I agree, it was something I thought about as well. There should be a
> new function for refcount increment. Maybe the existing one could be
> renamed to nfc_get_device_from_idx() and a new nfc_get_device() be
> defined.
> 
> I didn't want to introduce improvement patches in this UAF series, as
> that would be an independent unit of change.

Best regards,
Krzysztof

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

[Siddh Raman Pant]

On Tue, 05 Dec 2023 22:57:28 +0530, Krzysztof Kozlowski wrote:
> >> Mismatched order with get. Unwinding is always in reversed order. Or
> >> maybe other order is here on purpose? Then it needs to be explained.
> > 
> > Yes, local_release() will free local, so local->dev cannot be accessed.
> > Will add a comment.
> 
> So the problem is just storing the pointer? That's not really the valid
> reason.

Oops, my bad. What would be the valid reason? (if any)

Thanks,
Siddh

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

[Krzysztof Kozlowski]

On 05/12/2023 19:01, Siddh Raman Pant wrote:
> On Tue, 05 Dec 2023 22:57:28 +0530, Krzysztof Kozlowski wrote:
>>>> Mismatched order with get. Unwinding is always in reversed order. Or
>>>> maybe other order is here on purpose? Then it needs to be explained.
>>>
>>> Yes, local_release() will free local, so local->dev cannot be accessed.
>>> Will add a comment.
>>
>> So the problem is just storing the pointer? That's not really the valid
>> reason.
> 
> Oops, my bad. What would be the valid reason? (if any)

Just store the local->dev pointer and use correct order.

Best regards,
Krzysztof

[PATCH net-next v3 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>
---
 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

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

[Suman Ghosh]

>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 +++++