| Message ID | 476cccdcb57645784889fc82f0c7c10ff4c8b8c0.1701530776.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 |
Hi Siddh, >@@ -180,6 +183,7 @@ int nfc_llcp_local_put(struct nfc_llcp_local *local) > if (local == NULL) > return 0; > >+ nfc_put_device(local->dev); [Suman] One question here, if we consider the path, nfc_llcp_mac_is_down() -> nfc_llcp_socket_release() -> nfc_llcp_local_put(), then inside nfc_llcp_socket_release() we are already doing nfc_put_device() if "sk->sk_state == LLCP_CONNECTED", with this change we are doing it again. I guess you need to add some check to avoid that. Let me know if I am missing something. > return kref_put(&local->ref, local_release); } > >@@ -959,8 +963,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; >+ release_sock(&sock->sk); >+ sock_put(&sock->sk); >+ sock_put(&new_sock->sk); [Suman] don't we need to free new_sock? nfc_llcp_sock_free()? >+ 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 +1610,11 @@ >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) >+ return -ENODEV; [Suman] Memory leak here. Need to call kfree(local). >+ > INIT_LIST_HEAD(&local->list); > kref_init(&local->ref); > mutex_init(&local->sdp_lock); >-- >2.42.0 >
On Sun, 03 Dec 2023 22:29:39 +0530, Suman Ghosh wrote: > Hi Siddh, > > >@@ -180,6 +183,7 @@ int nfc_llcp_local_put(struct nfc_llcp_local *local) > > if (local == NULL) > > return 0; > > > >+ nfc_put_device(local->dev); > [Suman] One question here, if we consider the path, nfc_llcp_mac_is_down() -> > nfc_llcp_socket_release() -> nfc_llcp_local_put(), then inside > nfc_llcp_socket_release() we are already doing nfc_put_device() if > "sk->sk_state == LLCP_CONNECTED", with this change we are doing it again. > I guess you need to add some check to avoid that. Let me know if I am > missing something. The socket state is set to LLCP_CONNECTED in just two places: nfc_llcp_recv_connect() and nfc_llcp_recv_cc(). nfc_get_device() is used prior to setting the socket state to LLCP_CONNECTED in nfc_llcp_recv_connect(). After that, it calls nfc_llcp_send_cc(), which I suppose is a connection PDU by some Google-fu (NFC specs is paywalled). In nfc_llcp_recv_cc(), we do not use nfc_get_device(), but since one must send cc (which is done in nfc_llcp_recv_connect()), I think we are good here. This patch change doesn't touch any other refcounting. We increment the refcount whenever we get the local, and decrement when we put it. nfc_llcp_find_local() involves getting it, so all users of that function increment the refcount, which is also the case with nfc_llcp_mac_is_down(). The last nfc_llcp_local_put() then correctly decrements the refcount. If there is indeed a refcount error due to LLCP_CONNECTED, it probably exists without this patch too. > > 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); > [Suman] don't we need to free new_sock? nfc_llcp_sock_free()? > > [...] > > >+ local->dev = nfc_get_device(ndev->idx); > >+ if (!local->dev) > >+ return -ENODEV; > [Suman] Memory leak here. Need to call kfree(local). Yes, you are correct. Very stupid of me. Will send a v3. Thanks, Siddh
>> >> >@@ -180,6 +183,7 @@ int nfc_llcp_local_put(struct nfc_llcp_local >> >*local) >> > if (local == NULL) >> > return 0; >> > >> >+ nfc_put_device(local->dev); >> [Suman] One question here, if we consider the path, >> nfc_llcp_mac_is_down() -> >> nfc_llcp_socket_release() -> nfc_llcp_local_put(), then inside >> nfc_llcp_socket_release() we are already doing nfc_put_device() if >> "sk->sk_state == LLCP_CONNECTED", with this change we are doing it >again. >> I guess you need to add some check to avoid that. Let me know if I am >> missing something. > >The socket state is set to LLCP_CONNECTED in just two places: >nfc_llcp_recv_connect() and nfc_llcp_recv_cc(). > >nfc_get_device() is used prior to setting the socket state to >LLCP_CONNECTED in nfc_llcp_recv_connect(). After that, it calls >nfc_llcp_send_cc(), which I suppose is a connection PDU by some Google- >fu (NFC specs is paywalled). > >In nfc_llcp_recv_cc(), we do not use nfc_get_device(), but since one >must send cc (which is done in nfc_llcp_recv_connect()), I think we are >good here. > >This patch change doesn't touch any other refcounting. We increment the >refcount whenever we get the local, and decrement when we put it. >nfc_llcp_find_local() involves getting it, so all users of that function >increment the refcount, which is also the case with >nfc_llcp_mac_is_down(). The last nfc_llcp_local_put() then correctly >decrements the refcount. > >If there is indeed a refcount error due to LLCP_CONNECTED, it probably >exists without this patch too. [Suman] Thanks for the explanation.
diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c index 1dac28136e6a..a574c653e5d2 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,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; + release_sock(&sock->sk); + sock_put(&sock->sk); + sock_put(&new_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 +1610,11 @@ 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) + return -ENODEV; + INIT_LIST_HEAD(&local->list); kref_init(&local->ref); mutex_init(&local->sdp_lock);
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 | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)