nfc: Fix UAF during datagram sending caused by missing refcounting
Subsystems (3)
v3 cover nfc: Fix UAF during datagram sending caused by missing refcounting
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(-)
Signature · 1 line
2.42.0
PATCH v3 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 | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)
net/nfc/llcp_core.c +22 -2
@@ -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);
Signature · 1 line
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
Quoted 8 lines
Reported-and-tested-by:
[email protected]3A__syzkaller.appspot.com_bug-3Fextid- 3Dbbe84a4010eeea00982d&d=DwIDAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=7si3Xn9Ly- Se1a655kvEPIYU0nQ9HPeN280sEUv5ROU&m=Y51GaQT3fKSNuGmD- 9dRkeRq59DJDzTJ7yEh86VaPxCJP7TBLuK6eYKAw-fnG7- c&s=NUdyCeltsQ_6_rZPnNvgfyPleIYGMaSIN1sC1zqpV7Y&e=
Quoted 4 lines
Fixes c7aa12252f51 ("NFC: Take a reference on the LLCP local pointerwhen creating a socket")
Signed-off-by Siddh Raman Pant <[email protected]>---
Quoted 2 lines
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
In reply to Siddh Raman PantQuoted 51 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")Signed-off-by Siddh Raman Pant <[email protected]>--- net/nfc/llcp_core.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)net/nfc/llcp_core.c +3
-0@@ -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;
net/nfc/llcp_core.c int nfc_llcp_local_put(struct nfc_llcp_local *local) · line 186
Quoted 4 lines
net/nfc/llcp_core.c +1
-0@@ -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.
net/nfc/llcp_core.c static void nfc_llcp_recv_connect(struct nfc_llcp_local *local, · line 968
Quoted 11 lines
net/nfc/llcp_core.c +2
-1return 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.
Quoted 13 lines
net/nfc/llcp_core.c +9
-0+ 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;
net/nfc/llcp_core.c int nfc_llcp_register_device(struct nfc_dev *ndev) · line 1614
Quoted 5 lines
net/nfc/llcp_core.c +1
-1@@ -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.
net/nfc/llcp_core.c int nfc_llcp_register_device(struct nfc_dev *ndev) · line 1615
Quoted 1 line
net/nfc/llcp_core.c +1
-0+ 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
Quoted 8 lines · Krzysztof Kozlowski
net/nfc/llcp_core.c int nfc_llcp_local_put(struct nfc_llcp_local *local) · line 186
Quoted 5 lines
net/nfc/llcp_core.c +1
-0@@ -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.
Quoted 11 lines
net/nfc/llcp_core.c static void nfc_llcp_recv_connect(struct nfc_llcp_local *local, · line 968
Quoted 8 lines
net/nfc/llcp_core.c +2
-1@@ -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.
Quoted 16 lines
Quoted 16 lines
net/nfc/llcp_core.c +9
-0+ 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 ;
Quoted 5 lines
net/nfc/llcp_core.c int nfc_llcp_register_device(struct nfc_dev *ndev) · line 1614
Quoted 2 lines
net/nfc/llcp_core.c +1
-1- 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 tollcp_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.
Quoted 5 lines
net/nfc/llcp_core.c int nfc_llcp_register_device(struct nfc_dev *ndev) · line 1615
Quoted 1 line
net/nfc/llcp_core.c +1
-0+ 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
Quoted 4 lines · Siddh Raman Pant
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 newnfc_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
In reply to Siddh Raman PantQuoted 5 lines · Siddh Raman Pant
Quoted 4 lines · Krzysztof Kozlowski
Quoted 4 lines
net/nfc/llcp_core.c +0
-0@@ -180,6 +183,7 @@ int nfc_llcp_local_put(struct nfc_llcp_local *local)if (local ==NULL )return 0 ;
Quoted 7 lines
Quoted 4 lines
net/nfc/llcp_core.c int nfc_llcp_local_put(struct nfc_llcp_local *local) · line 186
Quoted 1 line
net/nfc/llcp_core.c +1
-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, solocal->devcannot be accessed. Will add a comment.
So the problem is just storing the pointer? That's not really the valid reason.
Quoted 37 lines
Quoted 4 lines
Quoted 4 lines
(quoted diff) +0
-0@@ -959,8 +963,18 @@ static void nfc_llcp_recv_connect(struct nfc_llcp_local *local, }Quoted 8 lines
net/nfc/llcp_core.c static void nfc_llcp_recv_connect(struct nfc_llcp_local *local, · line 968
Quoted 5 lines
net/nfc/llcp_core.c +2
-1new_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.
Quoted 16 lines
Quoted 16 lines
net/nfc/llcp_core.c +9
-0+ 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 ;Quoted 5 lines
net/nfc/llcp_core.c int nfc_llcp_register_device(struct nfc_dev *ndev) · line 1614
Quoted 2 lines
net/nfc/llcp_core.c +1
-1- 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 tollcp_local.
Quoted 3 lines
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.
Quoted 7 lines
Quoted 6 lines
net/nfc/llcp_core.c int nfc_llcp_register_device(struct nfc_dev *ndev) · line 1615
Quoted 2 lines
net/nfc/llcp_core.c +1
-0+ 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.
Quoted 7 lines
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 newnfc_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
Quoted 8 lines · Krzysztof Kozlowski
Quoted 5 lines
Quoted 2 lines
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, solocal->devcannot 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
Quoted 11 lines · Siddh Raman Pant
Quoted 8 lines · Krzysztof Kozlowski
Quoted 5 lines
Quoted 2 lines
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, solocal->devcannot 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 v3 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
RE: [EXT] [PATCH net-next v3 2/2] nfc: Do not send datagram if socket state isn't LLCP_BOUND
Quoted 8 lines
As we know we cannot send the datagram (state can be set to
LLCP_CLOSEDbynfc_llcp_socket_release()), there is no need to proceed further.Thus, bail out early from
llcp_sock_sendmsg().Signed-off-by Siddh Raman Pant <[email protected]>Reviewed-by Krzysztof Kozlowski <[email protected]>---
Quoted 1 line
net/nfc/llcp_sock.c| 5 +++++