[net-next,v6,1/2] nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_local
Commit Message
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(-)
Comments
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
@@ -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);