Re: [PATCH net] nfc: st21nfca: complete a pending SE APDU request only once

[Joas Antonio]

Hi Simon,

Thanks, you're right.

The two completion paths are serialised against each other:
st21nfca_apdu_reader_event_received() does timer_delete_sync() and
cancel_work_sync() before it looks at bwi_active, and the timer is only
re-armed from the same HCI rx work (EVT_WTX_REQUEST). But
st21nfca_hci_se_io() runs from the netlink path under device_lock()
only (nfc_se_io() in net/nfc/netlink.c), which neither the rx work nor
the timeout work takes. So:

- bwi_active, cb and cb_context are plain fields written by
  st21nfca_hci_se_io() while the rx work or the timeout work may be
  reading them;
- a second SE_IO issued while one is pending overwrites cb and
  cb_context, so a completion running at the same time can pair one
  request's callback with the other's context, and the first context is
  never completed. That is not new with this patch, but the bwi_active
  check does not cover it either;
- st21nfca_hci_se_io() sets bwi_active only after mod_timer().

For v2 I plan to add a spinlock to se_info that covers bwi_active, cb
and cb_context: st21nfca_hci_se_io() fails with -EBUSY (freeing
cb_context, as on the existing error path) if a request is already
pending, otherwise records it and sets bwi_active before arming the
timer; the timeout work and the EVT_TRANSMIT_DATA handler take the
pending request out under the lock (test and clear bwi_active, copy cb
and cb_context) and call the callback after dropping it, so exactly one
of them completes it.

Does that sound reasonable? I'll rerun the emulator tests on it before
sending.

Thanks,
Joas

Em sáb., 31 de out. de 2026 às 15:25, Simon Horman <horms@xxxxxxxxxx> escreveu:
>
> On Thu, Oct 29, 2026 at 11:40:02PM -0300, Joas Antonio dos Santos wrote:
> > st21nfca_hci_se_io() stores the netlink callback and its context in
> > info->se_info and arms the BWI timer.  The request is then completed
> > either by st21nfca_se_wt_work(), when the timer expires, or by an
> > EVT_TRANSMIT_DATA event on the APDU reader gate.  Neither checks that a
> > request is still pending, and the callback, se_io_cb(), frees its
> > context every time it is called.
> >
> > So when the secure element answers after the timeout, the response
> > calls se_io_cb() again on the context the timeout already freed, and
> > frees it a second time.  The same happens if the controller sends two
> > EVT_TRANSMIT_DATA events for one request.  An EVT_TRANSMIT_DATA, or a
> > timer armed by EVT_WTX_REQUEST, with no request pending calls the
> > callback on the context of an earlier, already completed request, or
> > calls a NULL callback if there never was one.
> >
> > With st21nfca on a virtual phy and an emulated controller answering an
> > NFC_CMD_SE_IO request 4 s after it was sent (the default timeout is
> > 3.2 s):
> >
> >   BUG: KASAN: slab-use-after-free in se_io_cb+0x2ec/0x334
> >   Read of size 4 at addr fff00000c5277da0 by task kworker/3:1/59
> >   Workqueue: events nfc_hci_msg_rx_work
> >   Call trace:
> >    se_io_cb+0x2ec/0x334
> >    st21nfca_apdu_reader_event_received+0x1ec/0x248
> >    st21nfca_hci_event_received+0xb0/0x300
> >    nfc_hci_event_received+0xb8/0x30c
> >    ...
> >   Freed by task 59:
> >    kfree+0x214/0x498
> >    se_io_cb+0x2bc/0x334
> >    st21nfca_se_wt_work+0x14c/0x1d4
> >
> > and for an EVT_TRANSMIT_DATA without a request:
> >
> >   Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> >   pc : 0x0
> >   lr : st21nfca_apdu_reader_event_received+0x1ec/0x248
> >
> > Use bwi_active, which st21nfca_hci_se_io() sets, to tell whether a
> > request is pending, and clear it in whichever of the two paths
> > completes the request first.  The event handler already stops the timer
> > and waits for the timeout work before looking at it.  Also complete the
> > request with the error if sending EVT_SE_END_OF_APDU_TRANSFER fails
> > rather than never calling the callback, which leaked the context.
> >
> > Fixes: 2130fb97fecf ("NFC: st21nfca: Adding support for secure element")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Joas Antonio dos Santos <joasantonio108@xxxxxxxxx>
> > Assisted-by: Yaga Agent HackerSec Claude:claude-opus-5-5
> > ---
> > Tested on arm64 QEMU with KASAN, with a test-only module that registers
> > st21nfca through st21nfca_hci_probe() on a virtual phy (LLC nop) and a
> > userspace controller emulator (DEV_UP, ENABLE_SE, SE_IO over netlink).
> > Without the patch: slab-use-after-free in se_io_cb() for a late and for
> > a duplicated EVT_TRANSMIT_DATA, NULL pointer call for an unsolicited one.
> > With it all three are quiet and a normal SE_IO exchange still works.
> > Not tested on hardware.
> >
> > st-nci has the same pattern; Fan Wu's pending "nfc: st-nci: drain SE
> > timers before NCI teardown" v2 already makes bwi_active an ownership
> > token there, so this only touches st21nfca.
> >
> >  drivers/nfc/st21nfca/se.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
> > index 7154bc1d6..4261ed1d9 100644
> > --- a/drivers/nfc/st21nfca/se.c
> > +++ b/drivers/nfc/st21nfca/se.c
> > @@ -264,6 +264,12 @@ static void st21nfca_se_wt_work(struct work_struct *work)
> >                                               struct st21nfca_hci_info,
> >                                               se_info.timeout_work);
> >
> > +     /* The timer can also be armed by EVT_WTX_REQUEST while no
> > +      * st21nfca_hci_se_io() is pending.
> > +      */
> > +     if (!info->se_info.bwi_active)
> > +             return;
> > +
> >       info->se_info.bwi_active = false;
>
> Hi Joas,
>
> I'm unsure if it effects the correctness of this fix.
> But I am concerned that the use of bwi_active is racy.
>
> >
> >       if (!info->se_info.xch_error) {
> > @@ -383,11 +389,20 @@ int st21nfca_apdu_reader_event_received(struct nfc_hci_dev *hdev,
> >       case ST21NFCA_EVT_TRANSMIT_DATA:
> >               timer_delete_sync(&info->se_info.bwi_timer);
> >               cancel_work_sync(&info->se_info.timeout_work);
> > +             /* Only complete a pending st21nfca_hci_se_io(), and only once:
> > +              * the timeout work may already have completed it, and the
> > +              * callback frees its context.
> > +              */
> > +             if (!info->se_info.bwi_active)
> > +                     break;
> > +
> >               info->se_info.bwi_active = false;
> >               r = nfc_hci_send_event(hdev, ST21NFCA_DEVICE_MGNT_GATE,
> >                               ST21NFCA_EVT_SE_END_OF_APDU_TRANSFER, NULL, 0);
> > -             if (r < 0)
> > +             if (r < 0) {
> > +                     info->se_info.cb(info->se_info.cb_context, NULL, 0, r);
> >                       goto exit;
> > +             }
> >
> >               info->se_info.cb(info->se_info.cb_context,
> >                       skb->data, skb->len, 0);
> > --
> > 2.43.0
> >



-- 
Joas Antonio dos Santos
Cyber Security Specialist and Ethical Hacker
joasantonio108@xxxxxxxxx
LinkedIn