| Message ID | 4233248c0ca219693c6e6476aa6e59c799241ac8.1702118242.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 |
On Sat, 2023-12-09 at 16:34 +0530, 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") > Signed-off-by: Siddh Raman Pant <code@siddh.me> > Reviewed-by: Suman Ghosh <sumang@marvell.com> > --- > net/nfc/llcp_core.c | 55 ++++++++++++++++++++++++++++++++++----------- > 1 file changed, 42 insertions(+), 13 deletions(-) > > diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c > index 1dac28136e6a..0ae89ab42aaa 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, > @@ -930,9 +945,7 @@ static void nfc_llcp_recv_connect(struct nfc_llcp_local *local, > > if (sk_acceptq_is_full(parent)) { > reason = LLCP_DM_REJ; 'reason' is set to 'LLCP_DM_REJ' every time you jump to the 'fail_put_sock' or 'fail_free_new_sock' labels, you can as well move the assignment after 'fail_put_sock:' Cheers, Paolo
On Tue, 12 Dec 2023 18:31:57 +0530, Paolo Abeni wrote: > On Sat, 2023-12-09 at 16:34 +0530, Siddh Raman Pant wrote: > > static struct nfc_llcp_sock *nfc_llcp_sock_get(struct nfc_llcp_local *local, > > @@ -930,9 +945,7 @@ static void nfc_llcp_recv_connect(struct nfc_llcp_local *local, > > > > if (sk_acceptq_is_full(parent)) { > > reason = LLCP_DM_REJ; > > 'reason' is set to 'LLCP_DM_REJ' every time you jump to the > 'fail_put_sock' or 'fail_free_new_sock' labels, you can as well move > the assignment after 'fail_put_sock:' Sure, I'll send a patch with that. Thanks, Siddh
diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c index 1dac28136e6a..0ae89ab42aaa 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, @@ -930,9 +945,7 @@ static void nfc_llcp_recv_connect(struct nfc_llcp_local *local, if (sk_acceptq_is_full(parent)) { reason = LLCP_DM_REJ; - release_sock(&sock->sk); - sock_put(&sock->sk); - goto fail; + goto fail_put_sock; } if (sock->ssap == LLCP_SDP_UNBOUND) { @@ -942,9 +955,7 @@ static void nfc_llcp_recv_connect(struct nfc_llcp_local *local, if (ssap == LLCP_SAP_MAX) { reason = LLCP_DM_REJ; - release_sock(&sock->sk); - sock_put(&sock->sk); - goto fail; + goto fail_put_sock; } sock->ssap = ssap; @@ -953,14 +964,18 @@ static void nfc_llcp_recv_connect(struct nfc_llcp_local *local, 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; + 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) { + reason = LLCP_DM_REJ; + 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 +1019,13 @@ 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: + release_sock(&sock->sk); + sock_put(&sock->sk); fail: - /* Send DM */ nfc_llcp_send_dm(local, dsap, ssap, reason); } @@ -1597,7 +1617,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);
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> Reviewed-by: Suman Ghosh <sumang@marvell.com> --- net/nfc/llcp_core.c | 55 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 42 insertions(+), 13 deletions(-)