diff mbox series

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

Message ID 6a26e3b65817bb31cb11c8dde5b1b420071d944e.1702404519.git.code@siddh.me (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series nfc: Fix UAF during datagram sending caused by missing refcounting | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1115 this patch: 1115
netdev/cc_maintainers success CCed 5 of 6 maintainers
netdev/build_clang success Errors and warnings before: 1142 this patch: 1142
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1142 this patch: 1142
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 138 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Siddh Raman Pant Dec. 12, 2023, 6:49 p.m. UTC
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(-)

Comments

Krzysztof Kozlowski Dec. 13, 2023, 7:40 a.m. UTC | #1
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
Siddh Raman Pant Dec. 14, 2023, 5:23 p.m. UTC | #2
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
Krzysztof Kozlowski Dec. 15, 2023, 7:22 a.m. UTC | #3
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
diff mbox series

Patch

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);