A deadlock can occur between nfc_unregister_device() and rfkill_fop_write()
due to lock ordering inversion between device_lock and rfkill_global_mutex.

The problematic lock order is:

Thread A (rfkill_fop_write):
  rfkill_fop_write()
    mutex_lock(&rfkill_global_mutex)
      rfkill_set_block()
        nfc_rfkill_set_block()
          nfc_dev_down()
            device_lock(&dev->dev)    <- waits for device_lock

Thread B (nfc_unregister_device):
  nfc_unregister_device()
    device_lock(&dev->dev)
      rfkill_unregister()
        mutex_lock(&rfkill_global_mutex)  <- waits for rfkill_global_mutex

This creates a classic ABBA deadlock scenario.

Fix this by moving rfkill_unregister() and rfkill_destroy() outside the
device_lock critical section. To ensure safety, set shutting_down flag
first and store rfkill pointer in a local variable before releasing the
lock. The shutting_down flag ensures that nfc_dev_down() and nfc_dev_up()
will bail out early if called during device unregistration.

Reported-by: syzbot+4ef89409a235d804c6c2@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=4ef89409a235d804c6c2
Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
---
 net/nfc/core.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/nfc/core.c b/net/nfc/core.c
index ae1c842f9c64..201d2b95432b 100644
--- a/net/nfc/core.c
+++ b/net/nfc/core.c
@@ -1154,7 +1154,7 @@ EXPORT_SYMBOL(nfc_register_device);
 void nfc_unregister_device(struct nfc_dev *dev)
 {
 	int rc;
-
+	struct rfkill *rfk = NULL;
 	pr_debug("dev_name=%s\n", dev_name(&dev->dev));
 
 	rc = nfc_genl_device_removed(dev);
@@ -1164,13 +1164,17 @@ void nfc_unregister_device(struct nfc_dev *dev)
 
 	device_lock(&dev->dev);
 	if (dev->rfkill) {
-		rfkill_unregister(dev->rfkill);
-		rfkill_destroy(dev->rfkill);
+		rfk = dev->rfkill;
 		dev->rfkill = NULL;
 	}
 	dev->shutting_down = true;
 	device_unlock(&dev->dev);
 
+	if (rfk) {
+		rfkill_unregister(rfk);
+		rfkill_destroy(rfk);
+	}
+
 	if (dev->ops->check_presence) {
 		timer_delete_sync(&dev->check_pres_timer);
 		cancel_work_sync(&dev->check_pres_work);
-- 
2.43.0

On 17/12/2025 06:49, Deepanshu Kartikey wrote:
> A deadlock can occur between nfc_unregister_device() and rfkill_fop_write()
> due to lock ordering inversion between device_lock and rfkill_global_mutex.
> 
> The problematic lock order is:
> 
> Thread A (rfkill_fop_write):
>   rfkill_fop_write()
>     mutex_lock(&rfkill_global_mutex)
>       rfkill_set_block()
>         nfc_rfkill_set_block()
>           nfc_dev_down()
>             device_lock(&dev->dev)    <- waits for device_lock
> 
> Thread B (nfc_unregister_device):
>   nfc_unregister_device()
>     device_lock(&dev->dev)
>       rfkill_unregister()
>         mutex_lock(&rfkill_global_mutex)  <- waits for rfkill_global_mutex
> 
> This creates a classic ABBA deadlock scenario.
> 
> Fix this by moving rfkill_unregister() and rfkill_destroy() outside the
> device_lock critical section. To ensure safety, set shutting_down flag
> first and store rfkill pointer in a local variable before releasing the
> lock. The shutting_down flag ensures that nfc_dev_down() and nfc_dev_up()
> will bail out early if called during device unregistration.
> 
> Reported-by: syzbot+4ef89409a235d804c6c2@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=4ef89409a235d804c6c2

You need Fixes and CC-stable tags.

So this was introduced by previous fix from Lin Ma... All these random
drive-bys by various people based on syzbot are just patching buggy code
with more buggy code. :/ No one cares too look more than just the syzbot
report.

And you as well. If you fix ABBA here, you should look and fix in other
places as well. There is exactly same order of locks in
nfc_register_device(). That's the register path which should not be a
problem, maybe except false LOCKDEP positives, but you should explain
that in commit msg why leaving ABBA there is safe.




> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
> ---
>  net/nfc/core.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/net/nfc/core.c b/net/nfc/core.c
> index ae1c842f9c64..201d2b95432b 100644
> --- a/net/nfc/core.c
> +++ b/net/nfc/core.c
> @@ -1154,7 +1154,7 @@ EXPORT_SYMBOL(nfc_register_device);
>  void nfc_unregister_device(struct nfc_dev *dev)
>  {
>  	int rc;
> -

Do not remove blank line.

> +	struct rfkill *rfk = NULL;
>  	pr_debug("dev_name=%s\n", dev_name(&dev->dev));
>  
>  	rc = nfc_genl_device_removed(dev);
> @@ -1164,13 +1164,17 @@ void nfc_unregister_device(struct nfc_dev *dev)
>  
>  	device_lock(&dev->dev);
>  	if (dev->rfkill) {
> -		rfkill_unregister(dev->rfkill);
> -		rfkill_destroy(dev->rfkill);
> +		rfk = dev->rfkill;
>  		dev->rfkill = NULL;
>  	}
>  	dev->shutting_down = true;
>  	device_unlock(&dev->dev);

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?

Best regards,
Krzysztof

On Wed, Dec 17, 2025 at 1:01 PM Krzysztof Kozlowski <krzk@kernel.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

On 17/12/2025 09:11, Deepanshu Kartikey wrote:
> 
> 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.

Indeed, that's correct. Please mention this briefly in commit msg. The
same as other ABBA remark in register path.


Best regards,
Krzysztof