| Message ID | 0d812b9aae2f16691d373460b06c5f3e098ed2a6.1702816635.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 17/12/2023 14:11, Siddh Raman Pant wrote: > static struct nfc_llcp_sock *nfc_llcp_sock_get(struct nfc_llcp_local *local, > @@ -959,8 +974,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); Why is this needed? Which part earlier gets the reference? > + nfc_llcp_sock_free(new_sock); This order is still wrong. Unwinding is almost always done in reversed order, for good reasons. Why do you unwind in other order? > + goto fail; > + } > + > + new_sock->dev = local->dev; > new_sock->rw = sock->rw; > new_sock->miux = sock->miux; Best regards, Krzysztof
On Mon, 18 Dec 2023 15:09:00 +0530, Krzysztof Kozlowski wrote: > On 17/12/2023 14:11, Siddh Raman Pant wrote: > > static struct nfc_llcp_sock *nfc_llcp_sock_get(struct nfc_llcp_local *local, > > @@ -959,8 +974,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); > > Why is this needed? Which part earlier gets the reference? Thanks for pointing out. sk_init sets refcount to 1. Actually on a further look, the next line shouldn't be there as nfc_llcp_sock_free() is already called in sk->sk_destruct (== llcp_sock_destruct()), which is called via __sk_destruct(). As sock_put() -> sk_free() -> __sk_destruct() -> sk_prot_free(), so we need to put. TBH really don't know why nfc_llcp_sock_free() is not static. > > + nfc_llcp_sock_free(new_sock); > > This order is still wrong. Unwinding is almost always done in reversed > order, for good reasons. Why do you unwind in other order? Oops, extremely sorry about that :( I reverted back to wrong ordering from an older local commit and didn't check. I'll send the fixed one. Thanks, Siddh
diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c index 1dac28136e6a..fadc8a9ec4df 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,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 +1622,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 | 40 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-)