netdev - Re: [PATCH net v5 1/2] NFC: reorder the logic in nfc_{un,}register

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] [day] [month] [year] [list]

Date:   Wed, 17 Nov 2021 20:20:14 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Lin Ma <linma@....edu.cn>
Cc:     netdev@...r.kernel.org, krzysztof.kozlowski@...onical.com,
        davem@...emloft.net
Subject: Re: [PATCH net v5 1/2] NFC: reorder the logic in
 nfc_{un,}register_device

On Tue, 16 Nov 2021 23:26:52 +0800 Lin Ma wrote:
> There is a potential UAF between the unregistration routine and the NFC
> netlink operations.
> 
> The race that cause that UAF can be shown as below:
> 
>  (FREE)                      |  (USE)
> nfcmrvl_nci_unregister_dev   |  nfc_genl_dev_up
>   nci_close_device           |
>   nci_unregister_device      |    nfc_get_device
>     nfc_unregister_device    |    nfc_dev_up
>       rfkill_destory         |
>       device_del             |      rfkill_blocked
>   ...                        |    ...
> 
> The root cause for this race is concluded below:
> 1. The rfkill_blocked (USE) in nfc_dev_up is supposed to be placed after
> the device_is_registered check.
> 2. Since the netlink operations are possible just after the device_add
> in nfc_register_device, the nfc_dev_up() can happen anywhere during the
> rfkill creation process, which leads to data race.
> 
> This patch reorder these actions to permit
> 1. Once device_del is finished, the nfc_dev_up cannot dereference the
> rfkill object.
> 2. The rfkill_register need to be placed after the device_add of nfc_dev
> because the parent device need to be created first. So this patch keeps
> the order but inject device_lock to prevent the data race.

Applied.