nfc: Fix UAF during datagram sending caused by missing refcounting
Subsystems (3)
v5 cover nfc: Fix UAF during datagram sending caused by missing refcounting
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(-)
Signature · 1 line
2.42.0
PATCH v5 1/2 nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_local
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.
--- net/nfc/llcp_core.c | 72 +++++++++++++++++++++++++++++---------------- 1 file changed, 47 insertions(+), 25 deletions(-)
net/nfc/llcp_core.c +47 -25
@@ -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);
Signature · 1 line
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
Quoted 76 lines · Siddh Raman Pant
llcp_sock_sendmsg()callsnfc_llcp_send_ui_frame()which in turn callsnfc_alloc_send_skb(), which accesses thenfc_devfrom thellcp_sockfor getting the headroom and tailroom needed for skb allocation.Parallelly the
nfc_devcan be freed, as the refcount is decreased vianfc_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_devWhen a reference to
llcp_localis acquired, we do not acquire the same for thenfc_dev. This leads to freeing even when thellcp_localis 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 tonfc_dev, and release the references appropriately later.References for
llcp_localis initialized innfc_llcp_register_device()(which is called bynfc_register_device()). Thus, we should acquire a reference tonfc_devthere.
nfc_unregister_device()callsnfc_llcp_unregister_device()which in turn callsnfc_llcp_local_put(). Thus, the reference tonfc_devis appropriately released later.Reported-and-tested-by [email protected]Fixes c7aa12252f51 ("NFC: Take a reference on the LLCP local pointer when creating a socket")Reviewed-by Suman Ghosh <[email protected]>Signed-off-by Siddh Raman Pant <[email protected]>--- net/nfc/llcp_core.c | 72 +++++++++++++++++++++++++++++---------------- 1 file changed, 47 insertions(+), 25 deletions(-)net/nfc/llcp_core.c +16
-1@@ -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,
net/nfc/llcp_core.c static void nfc_llcp_recv_connect(struct nfc_llcp_local *local, · line 919
Quoted 5 lines
net/nfc/llcp_core.c +1
-1@@ -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.
net/nfc/llcp_core.c static void nfc_llcp_recv_connect(struct nfc_llcp_local *local, · line 928
Quoted 9 lines
net/nfc/llcp_core.c +1
-1reason =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.
net/nfc/llcp_core.c static void nfc_llcp_recv_connect(struct nfc_llcp_local *local, · line 936
Quoted 9 lines
net/nfc/llcp_core.c +1
-1reason =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.
Quoted 5 lines
net/nfc/llcp_core.c +0
-0reason =LLCP_DM_NOBOUND ; goto fail; }@@ -928,39 +943,31 @@ static void nfc_llcp_recv_connect(struct nfc_llcp_local *local,
net/nfc/llcp_core.c static void nfc_llcp_recv_connect(struct nfc_llcp_local *local, · line 947
Quoted 10 lines
net/nfc/llcp_core.c +2
-6parent = &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.
Quoted 8 lines
net/nfc/llcp_core.c +0
-2if (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 ;
net/nfc/llcp_core.c static void nfc_llcp_recv_connect(struct nfc_llcp_local *local, · line 959
Quoted 10 lines
net/nfc/llcp_core.c +2
-4-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
Quoted 11 lines · Krzysztof Kozlowski
net/nfc/llcp_core.c static void nfc_llcp_recv_connect(struct nfc_llcp_local *local, · line 947
Quoted 8 lines
net/nfc/llcp_core.c +2
-6-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.
Quoted 2 lines
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
Quoted 12 lines · Siddh Raman Pant
Quoted 11 lines · Krzysztof Kozlowski
net/nfc/llcp_core.c static void nfc_llcp_recv_connect(struct nfc_llcp_local *local, · line 947
Quoted 8 lines
net/nfc/llcp_core.c +2
-6-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.
Quoted 5 lines
Quoted 2 lines
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 v5 2/2 nfc: Do not send datagram if socket state isn't LLCP_BOUND
In reply to Siddh Raman PantAs 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().
--- net/nfc/llcp_sock.c | 5 +++++ 1 file changed, 5 insertions(+)
net/nfc/llcp_sock.c +5 -0
@@ -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);
Signature · 1 line
2.42.0