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

Daniel Zahka <[email protected]> =E4=BA=8E2026=E5=B9=B47=E6=9C=8822=E6=
=97=A5=E5=91=A8=E4=B8=89 02:07=E5=86=99=E9=81=93=EF=BC=9A
>
>
>
> On 7/21/26 12:36 PM, Weiming Shi wrote:
> > 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_wor=
k),
> > 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 ag=
ain.
> > A work re-armed after its cancel_work_sync() therefore runs concurrentl=
y
> > with the cmd_queue cleanup and dereferences a digital_cmd the cleanup h=
as
> > 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): st=
art
> > an NFC-DEP exchange between the two nfcsim devices and unload the modul=
e
> > while it is running.
> >
> >   BUG: KASAN: slab-use-after-free in digital_wq_cmd (net/nfc/digital_co=
re.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 comma=
nd
> > works.  disable_work_sync() cancels the work and disables it, so any la=
ter
> > schedule_work() -- whether from the sibling work re-arming it or from t=
he
> > 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"=
)
>
> Fix for this commit should target the net tree instead of net-next.
>
> > Reported-by: Xiang Mei <[email protected]>
> > Assisted-by: Claude:claude-opus-4-8
> > Signed-off-by: Weiming Shi <[email protected]>
> > ---
> >   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_digit=
al_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: d4932951a19a5f1ec93200260b85e1a4c080ff77
>

Thanks, will resend as v2 against net.