| Message ID | 6a26e3b65817bb31cb11c8dde5b1b420071d944e.1702404519.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 12/12/2023 19: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. > > 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 | 72 +++++++++++++++++++++++++++++---------------- > 1 file changed, 47 insertions(+), 25 deletions(-) > > diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c > index 1dac28136e6a..2f77200a3720 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, > @@ -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. > 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) { Not related. > 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) { Not related. > 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; 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. > > 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; > } > Best regards, Krzysztof
On Wed, 13 Dec 2023 13:10:16 +0530, Krzysztof Kozlowski wrote: > > - 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. So this should not be done? I did it because you told to extend the cleanup label in v3 discussion. Thanks, Siddh
On 14/12/2023 18:23, Siddh Raman Pant wrote: > On Wed, 13 Dec 2023 13:10:16 +0530, Krzysztof Kozlowski wrote: >>> - 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. > > 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
diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c index 1dac28136e6a..2f77200a3720 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, @@ -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);
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 | 72 +++++++++++++++++++++++++++++---------------- 1 file changed, 47 insertions(+), 25 deletions(-)