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: [email protected]
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 <[email protected]>
---
net/nfc/llcp_core.c | 24 ++++++++++++++++++++++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
index 1dac28136e6a..9d45ce6dcdca 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,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 +1611,13 @@ 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) {
+ kfree(local);
+ return -ENODEV;
+ }
+
INIT_LIST_HEAD(&local->list);
kref_init(&local->ref);
mutex_init(&local->sdp_lock);
--
2.42.0
>Reported-and-tested-by:
>[email protected]
>Closes: https://urldefense.proofpoint.com/v2/url?u=https-
>3A__syzkaller.appspot.com_bug-3Fextid-
>3Dbbe84a4010eeea00982d&d=DwIDAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=7si3Xn9Ly-
>Se1a655kvEPIYU0nQ9HPeN280sEUv5ROU&m=Y51GaQT3fKSNuGmD-
>9dRkeRq59DJDzTJ7yEh86VaPxCJP7TBLuK6eYKAw-fnG7-
>c&s=NUdyCeltsQ_6_rZPnNvgfyPleIYGMaSIN1sC1zqpV7Y&e=
>Fixes: c7aa12252f51 ("NFC: Take a reference on the LLCP local pointer
>when creating a socket")
>Signed-off-by: Siddh Raman Pant <[email protected]>
>---
Reviewed-by: Suman Ghosh <[email protected]>
> net/nfc/llcp_core.c | 24 ++++++++++++++++++++++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
On 04/12/2023 14:08, 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: [email protected]
> 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 <[email protected]>
> ---
> net/nfc/llcp_core.c | 24 ++++++++++++++++++++++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
> index 1dac28136e6a..9d45ce6dcdca 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);
Mismatched order with get. Unwinding is always in reversed order. Or
maybe other order is here on purpose? Then it needs to be explained.
> return kref_put(&local->ref, local_release);
> }
>
> @@ -959,8 +963,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) {
There is already an cleanup path/label, so extend it. Existing code
needs some improvements in that matter as well.
> + 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 +1611,13 @@ int nfc_llcp_register_device(struct nfc_dev *ndev)
> if (local == NULL)
> return -ENOMEM;
>
> - local->dev = ndev;
> + /* Hold a reference to the device. */
That's obvious. Instead write something not obvious - why you call
nfc_get_device() while not incrementing reference to llcp_local.
> + local->dev = nfc_get_device(ndev->idx);
This looks confusing. If you can access ndev->idx, then ndev reference
was already increased. In such case iterating through all devices to
find it, is unnecessary and confusing.
Best regards,
Krzysztof
On Tue, 05 Dec 2023 22:10:00 +0530, Krzysztof Kozlowski wrote:
> > @@ -180,6 +183,7 @@ int nfc_llcp_local_put(struct nfc_llcp_local *local)
> > if (local == NULL)
> > return 0;
> >
> > + nfc_put_device(local->dev);
>
> Mismatched order with get. Unwinding is always in reversed order. Or
> maybe other order is here on purpose? Then it needs to be explained.
Yes, local_release() will free local, so local->dev cannot be accessed.
Will add a comment.
> > @@ -959,8 +963,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) {
>
> There is already an cleanup path/label, so extend it. Existing code
> needs some improvements in that matter as well.
Sure.
> > + 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 +1611,13 @@ int nfc_llcp_register_device(struct nfc_dev *ndev)
> > if (local == NULL)
> > return -ENOMEM;
> >
> > - local->dev = ndev;
> > + /* Hold a reference to the device. */
>
> That's obvious. Instead write something not obvious - why you call
> nfc_get_device() while not incrementing reference to llcp_local.
Should I move it after kref_init()? Here, I'm bailing out early so we
don't have to do unnecessary init first, and the rest of the function
will never fail.
> > + local->dev = nfc_get_device(ndev->idx);
>
> This looks confusing. If you can access ndev->idx, then ndev reference
> was already increased. In such case iterating through all devices to
> find it, is unnecessary and confusing.
I agree, it was something I thought about as well. There should be a
new function for refcount increment. Maybe the existing one could be
renamed to nfc_get_device_from_idx() and a new nfc_get_device() be
defined.
I didn't want to introduce improvement patches in this UAF series, as
that would be an independent unit of change.
Thanks,
Siddh
On Tue, 05 Dec 2023 22:51:17 +0530, Siddh Raman Pant wrote:
> I agree, it was something I thought about as well. There should be a
> new function for refcount increment. Maybe the existing one could be
> renamed to nfc_get_device_from_idx() and a new nfc_get_device() be
> defined.
Or nfc_find_device() instead of nfc_get_device_from_idx().
Thanks,
Siddh
On 05/12/2023 18:21, Siddh Raman Pant wrote:
> On Tue, 05 Dec 2023 22:10:00 +0530, Krzysztof Kozlowski wrote:
>>> @@ -180,6 +183,7 @@ int nfc_llcp_local_put(struct nfc_llcp_local *local)
>>> if (local == NULL)
>>> return 0;
>>>
>>> + nfc_put_device(local->dev);
>>
>> Mismatched order with get. Unwinding is always in reversed order. Or
>> maybe other order is here on purpose? Then it needs to be explained.
>
> Yes, local_release() will free local, so local->dev cannot be accessed.
> Will add a comment.
So the problem is just storing the pointer? That's not really the valid
reason.
>
>>> @@ -959,8 +963,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) {
>>
>> There is already an cleanup path/label, so extend it. Existing code
>> needs some improvements in that matter as well.
>
> Sure.
>
>>> + 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 +1611,13 @@ int nfc_llcp_register_device(struct nfc_dev *ndev)
>>> if (local == NULL)
>>> return -ENOMEM;
>>>
>>> - local->dev = ndev;
>>> + /* Hold a reference to the device. */
>>
>> That's obvious. Instead write something not obvious - why you call
>> nfc_get_device() while not incrementing reference to llcp_local.
>
> Should I move it after kref_init()? Here, I'm bailing out early so we
> don't have to do unnecessary init first, and the rest of the function
> will never fail.
I meant, comment is obvious.
>
>>> + local->dev = nfc_get_device(ndev->idx);
>>
>> This looks confusing. If you can access ndev->idx, then ndev reference
>> was already increased. In such case iterating through all devices to
>> find it, is unnecessary and confusing.
>
> I agree, it was something I thought about as well. There should be a
> new function for refcount increment. Maybe the existing one could be
> renamed to nfc_get_device_from_idx() and a new nfc_get_device() be
> defined.
>
> I didn't want to introduce improvement patches in this UAF series, as
> that would be an independent unit of change.
Best regards,
Krzysztof
On Tue, 05 Dec 2023 22:57:28 +0530, Krzysztof Kozlowski wrote:
> >> Mismatched order with get. Unwinding is always in reversed order. Or
> >> maybe other order is here on purpose? Then it needs to be explained.
> >
> > Yes, local_release() will free local, so local->dev cannot be accessed.
> > Will add a comment.
>
> So the problem is just storing the pointer? That's not really the valid
> reason.
Oops, my bad. What would be the valid reason? (if any)
Thanks,
Siddh
On 05/12/2023 19:01, Siddh Raman Pant wrote:
> On Tue, 05 Dec 2023 22:57:28 +0530, Krzysztof Kozlowski wrote:
>>>> Mismatched order with get. Unwinding is always in reversed order. Or
>>>> maybe other order is here on purpose? Then it needs to be explained.
>>>
>>> Yes, local_release() will free local, so local->dev cannot be accessed.
>>> Will add a comment.
>>
>> So the problem is just storing the pointer? That's not really the valid
>> reason.
>
> Oops, my bad. What would be the valid reason? (if any)
Just store the local->dev pointer and use correct order.
Best regards,
Krzysztof