netdev - Re: [PATCH net v4] nfc: ... device_is_registered() is data race-able

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: <f51aa1.41ae.180705614b5.Coremail.linma@zju.edu.cn>
Date:   Thu, 28 Apr 2022 21:22:11 +0800 (GMT+08:00)
From:   "Lin Ma" <linma@....edu.cn>
To:     "Greg KH" <gregkh@...uxfoundation.org>
Cc:     "Duoming Zhou" <duoming@....edu.cn>,
        krzysztof.kozlowski@...aro.org, pabeni@...hat.com,
        linux-kernel@...r.kernel.org, davem@...emloft.net,
        alexander.deucher@....com, akpm@...ux-foundation.org,
        broonie@...nel.org, netdev@...r.kernel.org,
        "Jakub Kicinski" <kuba@...nel.org>
Subject: Re: [PATCH net v4] nfc: ... device_is_registered() is data
 race-able

Hello there,

> 
> Yes, that looks better, 

Cool, thanks again for giving comments. :)

> but what is the root problem here that you are
> trying to solve?  Why does NFC need this when no other subsystem does?
> 

Well, in fact, me and Duoming are keep finding concurrency bugs that happen 
between the device cleanup/detach routine and other undergoing routines.

That is to say, when a device, no matter real or virtual, is detached from 
the machine, the kernel awake cleanup routine to reclaim the resource. 
In current case, the cleanup routine will call nfc_unregister_device().

Other routines, mainly from user-space system calls, need to be careful of 
the cleanup event. In another word, the kernel need to synchronize these 
routines to avoid race bugs.

In our practice, we find that many subsystems are prone to this type of bug.

For example, in bluetooth we fix

BT subsystem
* e2cb6b891ad2 ("bluetooth: eliminate the potential race condition when removing
the HCI controller")
* fa78d2d1d64f ("Bluetooth: fix data races in smp_unregister(), smp_del_chan()")
..

AX25 subsystem
* 1ade48d0c27d ("ax25: NPD bug when detaching AX25 device")
..

we currently focus on the net relevant subsystems and we now is auditing the NFC 
code.

In another word, all subsystems need to take care of the synchronization issues.
But seems that the solutions are varied between different subsystem. 

Empirically speaking, most of them use specific flags + specific locks to prevent
the race. 

In such cases, if the cleanup routine first hold the lock, the other routines will
wait on the locks. Since the cleanup routine write the specific flag, the other
routine, after check the specific flag, will be aware of the cleanup stuff and just
abort their tasks.
If the other routines first hold the lock, the cleanup routine just wait them to 
finish.

NFC here is special because it uses device_is_registered. I thought the author may
believe this macro is race free. However, it is not. So we need to replace this check
to make sure the netlink functions will 100 percent be aware of the cleanup routine
and abort the task if they grab the device_lock lately. Otherwise, the nelink routine
will call sub-layer code and possilby dereference resources that already freed.

For example, one of my recent fix 3e3b5dfcd16a ("NFC: reorder the logic in 
nfc_{un,}register_device") takes the suggestion from maintainer as he thought the 
device_is_registered is enough. And for now we find out this device_is_registered
is not enough.

If you are wondering if other subsystems use the combination of device_is_registered
as check to avoid the race, I have to say I don't know. If the answer is yes, that code
is possibly also prone to this type of concurrent bug.

> thansk,
> 
> greg k-h

Thanks again.

Lin Ma