[PATCH net v2] nfc: digital: fix use-after-free in nfc_digital_unregister_device()

[PATCH net v2] nfc: digital: fix use-after-free in nfc_digital_unregister_device()

[Weiming Shi]

nfc_digital_unregister_device() cancels cmd_work and cmd_complete_work
once each and then frees the command queue.  The two works re-arm each
other: digital_wq_cmd_complete() ends with schedule_work(&ddev->cmd_work),
and digital_wq_cmd() hands a command to the driver whose asynchronous
completion schedules cmd_complete_work.  cancel_work_sync() only waits for
the instance it cancels; it does not stop the work from being queued again.
A work re-armed after its cancel_work_sync() therefore runs concurrently
with the cmd_queue cleanup and dereferences a digital_cmd the cleanup has
already freed.  digital_wq_cmd() widens the window by dropping cmd_lock
before using the command it took from the queue, while the cleanup loop
frees the commands without holding cmd_lock.

It is reproducible with the software NFC simulator (CONFIG_NFC_SIM): start
an NFC-DEP exchange between the two nfcsim devices and unload the module
while it is running.

 BUG: KASAN: slab-use-after-free in digital_wq_cmd (net/nfc/digital_core.c:174)
 Read of size 1 by task kworker/1:5
 Workqueue: events digital_wq_cmd
  digital_wq_cmd (net/nfc/digital_core.c:174)
  process_one_work
  worker_thread
  kthread

 Allocated by task 5124:
  digital_send_cmd (net/nfc/digital_core.c:234)
  digital_in_send_sdd_req
  digital_in_recv_sens_res
  digital_wq_cmd_complete (net/nfc/digital_core.c:134)

 Freed by task 4994:
  kfree
  nfc_digital_unregister_device (net/nfc/digital_core.c:859)
  nfcsim_device_free [nfcsim]
  nfcsim_exit [nfcsim]
  __do_sys_delete_module

Use disable_work_sync() instead of cancel_work_sync() for the two command
works.  disable_work_sync() cancels the work and disables it, so any later
schedule_work() -- whether from the sibling work re-arming it or from the
driver's completion callback -- becomes a no-op.  Once both works are
disabled no work can run, and the cleanup loop frees the queue with no work
able to reach a freed command.

Fixes: 59ee2361c924 ("NFC Digital: Implement driver commands mechanism")
Reported-by: Xiang Mei <xmei5@asu.edu>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
v2: retarget net-next -> net (Daniel Zahka); no code change.

disable_work_sync() is v6.10+, so this is not Cc: stable; a backportable
shutting_down-flag variant can follow for -stable if wanted.

 net/nfc/digital_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/nfc/digital_core.c b/net/nfc/digital_core.c
index 7cb1e6aaae90..6def5132a4a6 100644
--- a/net/nfc/digital_core.c
+++ b/net/nfc/digital_core.c
@@ -843,8 +843,8 @@ void nfc_digital_unregister_device(struct nfc_digital_dev *ddev)
 	mutex_unlock(&ddev->poll_lock);
 
 	cancel_delayed_work_sync(&ddev->poll_work);
-	cancel_work_sync(&ddev->cmd_work);
-	cancel_work_sync(&ddev->cmd_complete_work);
+	disable_work_sync(&ddev->cmd_work);
+	disable_work_sync(&ddev->cmd_complete_work);
 
 	list_for_each_entry_safe(cmd, n, &ddev->cmd_queue, queue) {
 		list_del(&cmd->queue);

base-commit: b2ff91b752b0d85e8815e7f44fd85205c4268094
-- 
2.43.0

Re: [PATCH net v2] nfc: digital: fix use-after-free in nfc_digital_unregister_device()

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260723174450.1015153-2-bestswngs@gmail.com?part=1
---
Subject: nfc: digital: fix use-after-free in nfc_digital_unregister_device()

This patch replaces cancel_work_sync() with disable_work_sync() on
cmd_work and cmd_complete_work in nfc_digital_unregister_device() so
that re-arming schedule_work() calls become no-ops before the cmd_queue
is freed.

> diff --git a/net/nfc/digital_core.c b/net/nfc/digital_core.c
> index 7cb1e6aaae90..6def5132a4a6 100644
> --- a/net/nfc/digital_core.c
> +++ b/net/nfc/digital_core.c
> @@ -843,8 +843,8 @@ void nfc_digital_unregister_device(struct nfc_digital_dev *ddev)
>  	mutex_unlock(&ddev->poll_lock);
>  
>  	cancel_delayed_work_sync(&ddev->poll_work);
> -	cancel_work_sync(&ddev->cmd_work);
> -	cancel_work_sync(&ddev->cmd_complete_work);
> +	disable_work_sync(&ddev->cmd_work);
> +	disable_work_sync(&ddev->cmd_complete_work);

[High]
Does this fully close the use-after-free window, or only the workqueue
re-arm portion of it?

Once digital_wq_cmd() has handed a command to the driver via
ops->in_send_cmd(..., digital_send_cmd_complete, cmd), the driver
retains a raw pointer to that digital_cmd in its own async machinery
(for nfcsim this is dev->recv_work / dev->arg, for real drivers it is
URB completions, timers, etc.).

disable_work_sync() on cmd_work and cmd_complete_work waits only for
those two work items; it does not synchronize the driver's async
completion path.  When that completion later fires it calls back into
digital_send_cmd_complete():

    static void digital_send_cmd_complete(struct nfc_digital_dev *ddev,
                                          void *arg, struct sk_buff *resp)
    {
            struct digital_cmd *cmd = arg;

            cmd->resp = resp;

            schedule_work(&ddev->cmd_complete_work);
    }

The subsequent schedule_work() is correctly a no-op after
disable_work_sync(), but the cmd->resp = resp store happens first, on
a digital_cmd that the cleanup loop in nfc_digital_unregister_device()
may already have kfree()'d.

The same reproducer described in the commit message applies: in
nfcsim_device_free(), nfc_digital_unregister_device() runs before
cancel_work_sync(&dev->recv_work) and before dev->up = false, so a
pending dev->recv_work blocked in wait_event can wake up after the
queue has been freed and call dev->cb(ddev, dev->arg, skb) with
dev->arg pointing at a freed digital_cmd.

Elsewhere in this file the pattern for stopping in-flight commands
before touching the queue calls digital_abort_cmd(ddev) (see
digital_stop_poll(), digital_dep_link_down(), and
digital_deactivate_target()), which asks the driver to abort its
outstanding commands.  Would an equivalent digital_abort_cmd(ddev) in
nfc_digital_unregister_device(), before the list_for_each_entry_safe()
kfree loop, be needed here as well?

The commit message states:

    Once both works are disabled no work can run, and the cleanup
    loop frees the queue with no work able to reach a freed command.

Is that description accurate given that the driver's completion
callback reaches the digital_cmd directly through the arg pointer
rather than through cmd_work or cmd_complete_work?

This residual issue is pre-existing rather than introduced by the
patch, but it seems relevant to the completeness of the fix as
described.