linux-kernel - Re: [PATCH] net: nfc: fix deadlock between nfc_unregister_device and rfkill_fop

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: <CADhLXY4pBLt8vLfo8JeZMgfNYD7f=F+zGWTim5HmPyM-7j9THg@mail.gmail.com>
Date: Wed, 17 Dec 2025 13:41:18 +0530
From: Deepanshu Kartikey <kartikey406@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, horms@...nel.org, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, 
	syzbot+4ef89409a235d804c6c2@...kaller.appspotmail.com
Subject: Re: [PATCH] net: nfc: fix deadlock between nfc_unregister_device and rfkill_fop_write

On Wed, Dec 17, 2025 at 1:01 PM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> 1. So your code allows concurrent thread nfc_rfkill_set_block() to be
> called at this spot
> 2. Original thread of unregistering will shortly later call
> device_del(), which goes through lock+kill+unlock,
> 3. Then the concurrent thread proceeds to device_lock() and all other
> things with freed device.
>
> You just replaced one issue with another issue, right?
>

Hi Krzysztof,

Thanks for the review.

Regarding the UAF concern:

The callback nfc_rfkill_set_block() is invoked from rfkill_fop_write()
which holds rfkill_global_mutex for the entire operation:

rfkill_fop_write():
    mutex_lock(&rfkill_global_mutex);
    list_for_each_entry(rfkill, &rfkill_list, node) {
        rfkill_set_block(rfkill, ev.soft);
    }
    mutex_unlock(&rfkill_global_mutex);

rfkill_set_block() calls ops->set_block() (i.e., nfc_rfkill_set_block)
without releasing rfkill_global_mutex.

Since rfkill_unregister() also acquires rfkill_global_mutex:

void rfkill_unregister(struct rfkill *rfkill)
{
    ...
    mutex_lock(&rfkill_global_mutex);
    rfkill_send_events(rfkill, RFKILL_OP_DEL);
    list_del_init(&rfkill->node);
    ...
    mutex_unlock(&rfkill_global_mutex);
}

The unregister path cannot proceed past rfkill_unregister() until any
ongoing callback completes. Since device_del() is called after
rfkill_unregister() returns, no UAF should be possible.

Additionally, if nfc_dev_down() runs after we set shutting_down = true,
it will see the flag and bail out early with -ENODEV without accessing
device internals.

Regarding nfc_register_device(): The same lock ordering exists there
(device_lock -> rfkill_global_mutex via rfkill_register), but during
registration the device is not yet visible to other subsystems, so no
concurrent rfkill operations can occur. The ABBA pattern there should
not cause an actual deadlock.

I will send a v2 addressing:
- Adding Fixes and Cc: stable tags
- Keeping the blank line after variable declaration

Thanks,
Deepanshu