* [PATCH net-next v7 0/2] nfc: Fix UAF during datagram sending caused by missing refcounting
@ 2023-12-19 17:49 Siddh Raman Pant
2023-12-19 17:49 ` [PATCH net-next v7 1/2] nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_local Siddh Raman Pant
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Siddh Raman Pant @ 2023-12-19 17:49 UTC (permalink / raw)
To: Krzysztof Kozlowski, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel
Changes in v7:
- Stupidly reverted ordering in recv() too, fix that.
- Remove redundant call to nfc_llcp_sock_free().
Changes in v6:
- Revert label introduction from v4, and thus also v5 entirely.
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 | 39 ++++++++++++++++++++++++++++++++++++---
net/nfc/llcp_sock.c | 5 +++++
2 files changed, 41 insertions(+), 3 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH net-next v7 1/2] nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_local
2023-12-19 17:49 [PATCH net-next v7 0/2] nfc: Fix UAF during datagram sending caused by missing refcounting Siddh Raman Pant
@ 2023-12-19 17:49 ` Siddh Raman Pant
2023-12-23 19:04 ` Krzysztof Kozlowski
2023-12-19 17:49 ` [PATCH net-next v7 2/2] nfc: Do not send datagram if socket state isn't LLCP_BOUND Siddh Raman Pant
2023-12-25 7:30 ` [PATCH net-next v7 0/2] nfc: Fix UAF during datagram sending caused by missing refcounting patchwork-bot+netdevbpf
2 siblings, 1 reply; 5+ messages in thread
From: Siddh Raman Pant @ 2023-12-19 17:49 UTC (permalink / raw)
To: Krzysztof Kozlowski, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, syzbot+bbe84a4010eeea00982d, Suman Ghosh
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 | 39 ++++++++++++++++++++++++++++++++++++---
1 file changed, 36 insertions(+), 3 deletions(-)
diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
index 1dac28136e6a..18be13fb9b75 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,
@@ -959,8 +974,17 @@ 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;
+ sock_put(&new_sock->sk);
+ release_sock(&sock->sk);
+ sock_put(&sock->sk);
+ 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 +1621,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.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH net-next v7 2/2] nfc: Do not send datagram if socket state isn't LLCP_BOUND
2023-12-19 17:49 [PATCH net-next v7 0/2] nfc: Fix UAF during datagram sending caused by missing refcounting Siddh Raman Pant
2023-12-19 17:49 ` [PATCH net-next v7 1/2] nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_local Siddh Raman Pant
@ 2023-12-19 17:49 ` Siddh Raman Pant
2023-12-25 7:30 ` [PATCH net-next v7 0/2] nfc: Fix UAF during datagram sending caused by missing refcounting patchwork-bot+netdevbpf
2 siblings, 0 replies; 5+ messages in thread
From: Siddh Raman Pant @ 2023-12-19 17:49 UTC (permalink / raw)
To: Krzysztof Kozlowski, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, 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 +++++
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.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v7 1/2] nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_local
2023-12-19 17:49 ` [PATCH net-next v7 1/2] nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_local Siddh Raman Pant
@ 2023-12-23 19:04 ` Krzysztof Kozlowski
0 siblings, 0 replies; 5+ messages in thread
From: Krzysztof Kozlowski @ 2023-12-23 19:04 UTC (permalink / raw)
To: Siddh Raman Pant, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
Cc: netdev, linux-kernel, syzbot+bbe84a4010eeea00982d, Suman Ghosh
On 19/12/2023 18: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.
>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v7 0/2] nfc: Fix UAF during datagram sending caused by missing refcounting
2023-12-19 17:49 [PATCH net-next v7 0/2] nfc: Fix UAF during datagram sending caused by missing refcounting Siddh Raman Pant
2023-12-19 17:49 ` [PATCH net-next v7 1/2] nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_local Siddh Raman Pant
2023-12-19 17:49 ` [PATCH net-next v7 2/2] nfc: Do not send datagram if socket state isn't LLCP_BOUND Siddh Raman Pant
@ 2023-12-25 7:30 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-12-25 7:30 UTC (permalink / raw)
To: Siddh Raman Pant
Cc: krzysztof.kozlowski, davem, edumazet, kuba, pabeni, netdev, linux-kernel
Hello:
This series was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:
On Tue, 19 Dec 2023 23:19:42 +0530 you wrote:
> Changes in v7:
> - Stupidly reverted ordering in recv() too, fix that.
> - Remove redundant call to nfc_llcp_sock_free().
>
> Changes in v6:
> - Revert label introduction from v4, and thus also v5 entirely.
>
> [...]
Here is the summary with links:
- [net-next,v7,1/2] nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_local
https://git.kernel.org/netdev/net/c/c95f919567d6
- [net-next,v7,2/2] nfc: Do not send datagram if socket state isn't LLCP_BOUND
https://git.kernel.org/netdev/net/c/6ec0d7527c42
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-12-25 7:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-19 17:49 [PATCH net-next v7 0/2] nfc: Fix UAF during datagram sending caused by missing refcounting Siddh Raman Pant
2023-12-19 17:49 ` [PATCH net-next v7 1/2] nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_local Siddh Raman Pant
2023-12-23 19:04 ` Krzysztof Kozlowski
2023-12-19 17:49 ` [PATCH net-next v7 2/2] nfc: Do not send datagram if socket state isn't LLCP_BOUND Siddh Raman Pant
2023-12-25 7:30 ` [PATCH net-next v7 0/2] nfc: Fix UAF during datagram sending caused by missing refcounting patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).