netdev - Re: [PATCH v1] net: nfc: Fix use-after-free in nfc_genl_llc_{{get/set}

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <746941b24732655c51dee68ed442bfc14a82e303.camel@redhat.com>
Date: Thu, 22 Jun 2023 09:58:52 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Lin Ma <linma@....edu.cn>, krzysztof.kozlowski@...aro.org, 
 avem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 netdev@...r.kernel.org
Subject: Re: [PATCH v1] net: nfc: Fix use-after-free in
 nfc_genl_llc_{{get/set}_params/sdreq}

On Tue, 2023-06-20 at 10:53 +0800, Lin Ma wrote:
> This commit fixes a use-after-free for object nfc_llcp_local, the root
> cause of this bug is due to the race in nfc_llcp_unregister_device.
> Just like the buggy time window below. Since the nfc_llcp_find_local will
> not increase the reference counter of object nfc_llcp_local, UAF occurs.
> 
> // nfc_genl_llc_get_params   | // nfc_unregister_device
>                              |
> dev = nfc_get_device(idx);   | device_lock(...)
> if (!dev)                    | dev->shutting_down = true;
>     return -ENODEV;          | device_unlock(...);
>                              |
> device_lock(...);            |   // nfc_llcp_unregister_device
>                              |   nfc_llcp_find_local()
> nfc_llcp_find_local(...);    |
>                              |   local_cleanup()
> if (!local) {                |
>     rc = -ENODEV;            |     // nfc_llcp_local_put
>     goto exit;               |     kref_put(.., local_release)
> }                            |
>                              |       // local_release
>                              |       list_del(&local->list)
>   // nfc_genl_send_params    |       kfree()
>   local->dev->idx !!!UAF!!!  |
>                              |
> To avoid this, we can add a check to dev->shutting_down inside the
> device_lock critical section. Therefore, the nfc_genl_llc_get_params will
> surely error return if it grabs the lock after the nfc_unregister_device
> releases the lock.
> 
> This patch applies such check for nfc_genl_llc_{{get/set}_params/sdreq}
> as they all use nfc_llcp_find_local and suffer from the race condition.

It looks like the mentioned race condition could apply to any callers
of nfc_llcp_find_local(), is there any special reason to not add the
new check directly inside nfc_llcp_find_local()?

Thanks!

Paolo