[PATCH RESEND v2 0/2] nfc: st21nfca: fix SE APDU request race condition

[PATCH RESEND v2 0/2] nfc: st21nfca: fix SE APDU request race condition

[Joas Antonio dos Santos via B4 Relay]

Fix a race between the timeout work handler and the EVT_TRANSMIT_DATA
event handler that can lead to a double invocation of the SE APDU
callback, resulting in use-after-free or double-free of the callback
context.

Split into two patches:
  1/2: Add a spinlock to serialize access to bwi_active, cb, and
       cb_context across the three concurrent paths.
  2/2: Use the lock to guard against double completion and reject
       overlapping requests.

Changes since v1:
  - Use spinlock instead of plain bwi_active flag checks (Simon Horman)
  - Split into 2-patch series: locking infrastructure + completion fix
  - Move bwi_active = true before mod_timer()
  - Copy cb/cb_context under lock, invoke after releasing
  - Reject overlapping se_io requests with -EBUSY
  - Drop Assisted-by trailer

---
Joas Antonio dos Santos (2):
      nfc: st21nfca: add spinlock to serialize SE APDU request state
      nfc: st21nfca: prevent double completion of SE APDU request

 drivers/nfc/st21nfca/se.c       | 36 ++++++++++++++++++++++++++++++++----
 drivers/nfc/st21nfca/st21nfca.h |  2 ++
 2 files changed, 34 insertions(+), 4 deletions(-)
---
base-commit: 602042bf29f6efde39cfb5fdd9289bf4854bc0c5
change-id: 20261108-st21nfca-v2-f4c5f861e416

Best regards,
-- 
Joas Antonio dos Santos <joasantonio108@gmail.com>

[PATCH RESEND v2 1/2] nfc: st21nfca: add spinlock to serialize SE APDU request state

[Joas Antonio dos Santos via B4 Relay]

From: Joas Antonio dos Santos <joasantonio108@gmail.com>

The bwi_active flag, callback pointer (cb), and callback context
(cb_context) in st21nfca_se_info are accessed concurrently from
multiple contexts without synchronization:

 - st21nfca_hci_se_io() sets them from the netlink/ioctl path
 - st21nfca_se_wt_work() reads/clears them from the timeout workqueue
 - st21nfca_apdu_reader_event_received() reads/clears them from the
   HCI rx path

Add a spinlock (se_lock) to serialize access to these fields.  The
callback pointer and context are copied under the lock and invoked
after releasing it, since both the callback and nfc_hci_send_event()
may sleep.

Also move the bwi_active = true assignment before mod_timer() so that
the flag is always set before the timeout handler could observe it.

Found by LLM-assisted manual code review.

Fixes: 2130fb97fecf ("NFC: st21nfca: Adding support for secure element")
Cc: Simon Horman <horms@kernel.org>
Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
---
 drivers/nfc/st21nfca/se.c       | 24 ++++++++++++++++++++----
 drivers/nfc/st21nfca/st21nfca.h |  2 ++
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
index 7154bc1d644a..03a30f797ff8 100644
--- a/drivers/nfc/st21nfca/se.c
+++ b/drivers/nfc/st21nfca/se.c
@@ -227,11 +227,13 @@ int st21nfca_hci_se_io(struct nfc_hci_dev *hdev, u32 se_idx,
 
 	switch (se_idx) {
 	case ST21NFCA_ESE_HOST_ID:
+		spin_lock(&info->se_info.se_lock);
 		info->se_info.cb = cb;
 		info->se_info.cb_context = cb_context;
+		info->se_info.bwi_active = true;
+		spin_unlock(&info->se_info.se_lock);
 		mod_timer(&info->se_info.bwi_timer, jiffies +
 			  msecs_to_jiffies(info->se_info.wt_timeout));
-		info->se_info.bwi_active = true;
 		return nfc_hci_send_event(hdev, ST21NFCA_APDU_READER_GATE,
 					ST21NFCA_EVT_TRANSMIT_DATA,
 					apdu, apdu_length);
@@ -260,11 +262,17 @@ static void st21nfca_se_wt_work(struct work_struct *work)
 	 */
 	/* hardware reset managed through VCC_UICC_OUT power supply */
 	u8 param = 0x01;
+	se_io_cb_t cb;
+	void *cb_context;
 	struct st21nfca_hci_info *info = container_of(work,
 						struct st21nfca_hci_info,
 						se_info.timeout_work);
 
+	spin_lock(&info->se_info.se_lock);
 	info->se_info.bwi_active = false;
+	cb = info->se_info.cb;
+	cb_context = info->se_info.cb_context;
+	spin_unlock(&info->se_info.se_lock);
 
 	if (!info->se_info.xch_error) {
 		info->se_info.xch_error = true;
@@ -275,7 +283,7 @@ static void st21nfca_se_wt_work(struct work_struct *work)
 		nfc_hci_send_event(info->hdev, ST21NFCA_DEVICE_MGNT_GATE,
 				ST21NFCA_EVT_SE_HARD_RESET, &param, 1);
 	}
-	info->se_info.cb(info->se_info.cb_context, NULL, 0, -ETIME);
+	cb(cb_context, NULL, 0, -ETIME);
 }
 
 static void st21nfca_se_wt_timeout(struct timer_list *t)
@@ -375,6 +383,8 @@ int st21nfca_apdu_reader_event_received(struct nfc_hci_dev *hdev,
 					u8 event, struct sk_buff *skb)
 {
 	int r = 0;
+	se_io_cb_t cb;
+	void *cb_context;
 	struct st21nfca_hci_info *info = nfc_hci_get_clientdata(hdev);
 
 	pr_debug("apdu reader gate event: %x\n", event);
@@ -383,14 +393,19 @@ 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);
+
+		spin_lock(&info->se_info.se_lock);
 		info->se_info.bwi_active = false;
+		cb = info->se_info.cb;
+		cb_context = info->se_info.cb_context;
+		spin_unlock(&info->se_info.se_lock);
+
 		r = nfc_hci_send_event(hdev, ST21NFCA_DEVICE_MGNT_GATE,
 				ST21NFCA_EVT_SE_END_OF_APDU_TRANSFER, NULL, 0);
 		if (r < 0)
 			goto exit;
 
-		info->se_info.cb(info->se_info.cb_context,
-			skb->data, skb->len, 0);
+		cb(cb_context, skb->data, skb->len, 0);
 		break;
 	case ST21NFCA_EVT_WTX_REQUEST:
 		mod_timer(&info->se_info.bwi_timer, jiffies +
@@ -416,6 +431,7 @@ void st21nfca_se_init(struct nfc_hci_dev *hdev)
 	/* initialize timers */
 	timer_setup(&info->se_info.bwi_timer, st21nfca_se_wt_timeout, 0);
 	info->se_info.bwi_active = false;
+	spin_lock_init(&info->se_info.se_lock);
 
 	timer_setup(&info->se_info.se_active_timer,
 		    st21nfca_se_activation_timeout, 0);
diff --git a/drivers/nfc/st21nfca/st21nfca.h b/drivers/nfc/st21nfca/st21nfca.h
index ae6771cc9894..abcef1ee48f2 100644
--- a/drivers/nfc/st21nfca/st21nfca.h
+++ b/drivers/nfc/st21nfca/st21nfca.h
@@ -6,6 +6,7 @@
 #ifndef __LOCAL_ST21NFCA_H_
 #define __LOCAL_ST21NFCA_H_
 
+#include <linux/spinlock.h>
 #include <net/nfc/hci.h>
 #include <linux/skbuff.h>
 #include <linux/workqueue.h>
@@ -131,6 +132,7 @@ struct st21nfca_se_info {
 	struct timer_list bwi_timer;
 	int wt_timeout; /* in msecs */
 	bool bwi_active;
+	spinlock_t se_lock;
 
 	struct timer_list se_active_timer;
 	bool se_active;

-- 
2.43.0

Re: [PATCH RESEND v2 1/2] nfc: st21nfca: add spinlock to serialize SE APDU request state

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 (2) · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] At the end of the series, st21nfca_hci_se_io() leaks the
  netlink se_io_ctx every time it rejects an overlapping request.
- [Low] The commit message says se_lock was added "to serialize access to
  these fields" (bwi_active, cb, cb_context).

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but teardown can
  leave bwi_timer armed when the struct st21nfca_hci_info is freed.
- [High] This problem wasn't introduced by this patch, but the sibling
  driver drivers/nfc/st-nci/se.c has the same SE APDU completion bug…

--

nfc: st21nfca: add spinlock to serialize SE APDU request state

This patch adds se_lock to struct st21nfca_se_info. The lock protects
bwi_active, cb and cb_context in st21nfca_hci_se_io(),
st21nfca_se_wt_work() and st21nfca_apdu_reader_event_received(), and
the callback is copied under the lock and called after it is dropped.
It also sets bwi_active before mod_timer() in st21nfca_hci_se_io().

> diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
> index 7154bc1d644ae..03a30f797ff8f 100644
> --- a/drivers/nfc/st21nfca/se.c
> +++ b/drivers/nfc/st21nfca/se.c
> @@ -227,11 +227,13 @@ int st21nfca_hci_se_io(struct nfc_hci_dev *hdev, u32 se_idx,
>  
>  	switch (se_idx) {
>  	case ST21NFCA_ESE_HOST_ID:
> +		spin_lock(&info->se_info.se_lock);
> +		info->se_info.cb = cb;
> +		info->se_info.cb_context = cb_context;
> +		info->se_info.bwi_active = true;
> +		spin_unlock(&info->se_info.se_lock);

[Severity: Medium]
This comment is about the follow-up commit in this series, "nfc:
st21nfca: prevent double completion of SE APDU request". That commit
adds a bwi_active check here:

    spin_lock(&info->se_info.se_lock);
    if (info->se_info.bwi_active) {
        spin_unlock(&info->se_info.se_lock);
        return -EBUSY;
    }

Does that -EBUSY path leak cb_context?

Once dev->ops->se_io() has been called, the driver owns cb_context.
nfc_se_io() in net/nfc/netlink.c only frees it when one of its own
pre-checks fails. When the driver returns an error, it does this:

    rc = dev->ops->se_io(dev, se_idx, apdu,
                         apdu_length, cb, cb_context);

    device_unlock(&dev->dev);
    return rc;

nfc_genl_se_io() doesn't free it either. The default branch of
st21nfca_hci_se_io() calls kfree(cb_context) for exactly this reason,
as its comment explains.

So each NFC_CMD_SE_IO that gets -EBUSY while an APDU is in flight would
leak one struct se_io_ctx. The window lasts at least wt_timeout (3200
ms by default). EVT_WTX_REQUEST can keep extending it. A failed
nfc_hci_send_event() can too, since it leaves bwi_active set until the
timer fires.

Before the series, a second request overwrote cb and cb_context and
leaked the first context. The follow-up fixes the double completion,
but the leak now happens on the rejected request instead.

Should the -EBUSY path call kfree(cb_context), like the default branch?

>  		mod_timer(&info->se_info.bwi_timer, jiffies +
>  			  msecs_to_jiffies(info->se_info.wt_timeout));
> -		info->se_info.bwi_active = true;
>  		return nfc_hci_send_event(hdev, ST21NFCA_APDU_READER_GATE,
>  					ST21NFCA_EVT_TRANSMIT_DATA,
>  					apdu, apdu_length);

[ ... ]

> @@ -383,14 +393,19 @@ 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);
> +
> +		spin_lock(&info->se_info.se_lock);
>  		info->se_info.bwi_active = false;
> +		cb = info->se_info.cb;
> +		cb_context = info->se_info.cb_context;
> +		spin_unlock(&info->se_info.se_lock);
> +
>  		r = nfc_hci_send_event(hdev, ST21NFCA_DEVICE_MGNT_GATE,
>  				ST21NFCA_EVT_SE_END_OF_APDU_TRANSFER, NULL, 0);
>  		if (r < 0)
>  			goto exit;
>  
> -		info->se_info.cb(info->se_info.cb_context,
> -			skb->data, skb->len, 0);
> +		cb(cb_context, skb->data, skb->len, 0);

[Severity: High]
This isn't a bug introduced by this patch, but the sibling driver
drivers/nfc/st-nci/se.c looks like it has the same double completion
problem, and the series doesn't touch it.

st_nci_se_wt_timeout() always calls the stored callback:

    info->se_info.bwi_active = false;
    ...
    info->se_info.cb(info->se_info.cb_context, NULL, 0, -ETIME);

st_nci_hci_apdu_reader_event_received() calls it for
ST_NCI_EVT_TRANSMIT_DATA without checking bwi_active:

    timer_delete_sync(&info->se_info.bwi_timer);
    info->se_info.bwi_active = false;
    info->se_info.cb(info->se_info.cb_context,
                     skb->data, skb->len, 0);

se_io_cb() in net/nfc/netlink.c frees its context on every call. If
the SE answers after the BWI timeout has already fired, se_io_cb()
could run twice on the same context:

nfc_genl_se_io()->nfc_se_io()->nci_se_io()->st_nci_se_io()
    arms bwi_timer
st_nci_se_wt_timeout()->se_io_cb()
    kfree(ctx)
st_nci_hci_apdu_reader_event_received()->se_io_cb()
    reads ctx->dev_idx, ctx->se_idx, then kfree(ctx) again

Would that be a use-after-free followed by a double kfree()? A
duplicate EVT_TRANSMIT_DATA would do the same thing. So would a WTX
request that re-arms the timer after an exchange has finished.
st_nci_se_io() also overwrites cb and cb_context without checking for
an outstanding request.

st_nci_se_wt_timeout() is a timer callback and runs in softirq
context. se_io_cb() allocates with nlmsg_new(NLMSG_DEFAULT_SIZE,
GFP_KERNEL). Can that sleep in atomic context?

Should st-nci get an equivalent fix?

>  		break;
>  	case ST21NFCA_EVT_WTX_REQUEST:
>  		mod_timer(&info->se_info.bwi_timer, jiffies +

[Severity: High]
This is a pre-existing issue, but can bwi_timer still be pending when
st21nfca_hci_remove() frees info?

    st21nfca_se_deinit(hdev);
    nfc_hci_unregister_device(hdev);
    nfc_hci_free_device(hdev);
    kfree(info);

st21nfca_se_deinit() only deletes the timer when an unlocked read of
bwi_active returns true, and it never shuts the timer down:

    if (info->se_info.bwi_active)
        timer_delete_sync(&info->se_info.bwi_timer);

There seem to be three ways the timer can stay armed:

- The HCI rx work is only cancelled later, in
  nfc_hci_unregister_device(). A queued ST21NFCA_EVT_WTX_REQUEST can
  reach the mod_timer() above after st21nfca_se_deinit() has run.

- A WTX request handled while bwi_active was false (for example just
  after a timeout) arms the timer, and st21nfca_se_deinit() then skips
  timer_delete_sync().

- The nfc_dev is still registered while st21nfca_se_deinit() runs, so
  NFC_CMD_SE_IO can reach st21nfca_hci_se_io(). It can set bwi_active
  and call mod_timer() after st21nfca_se_deinit() has read the flag,
  because the remove path doesn't take the device_lock that nfc_se_io()
  holds.

In each case, st21nfca_se_wt_timeout() would run on the freed info
when the timer expires. It would then call
schedule_work(&info->se_info.timeout_work) on freed memory.

st21nfca_se_deinit() also clears bwi_active without calling the
pending cb, so an outstanding cb_context would leak.

The new se_lock doesn't change any of this. Would it work to stop the
rx work and se_io first, and then unconditionally call
timer_shutdown_sync() on bwi_timer and cancel timeout_work?

[ ... ]

> diff --git a/drivers/nfc/st21nfca/st21nfca.h b/drivers/nfc/st21nfca/st21nfca.h
> index ae6771cc9894a..abcef1ee48f25 100644
> --- a/drivers/nfc/st21nfca/st21nfca.h
> +++ b/drivers/nfc/st21nfca/st21nfca.h

[ ... ]

> @@ -131,6 +132,7 @@ struct st21nfca_se_info {
>  	struct timer_list bwi_timer;
>  	int wt_timeout; /* in msecs */
>  	bool bwi_active;
> +	spinlock_t se_lock;

[Severity: Low]
The commit message says se_lock is added "to serialize access to these
fields". However, st21nfca_se_deinit() still reads and writes
bwi_active without it:

    if (info->se_info.bwi_active)
        timer_delete_sync(&info->se_info.bwi_timer);
    ...
    info->se_info.bwi_active = false;

st21nfca_se_deinit() runs before nfc_hci_unregister_device(), so the
HCI rx work and netlink se_io can still run at the same time. Should
these accesses take se_lock as well?

Taking the lock around this read alone wouldn't fix the teardown
problem above, because the timer can still be armed after the read.

>  
>  	struct timer_list se_active_timer;
>  	bool se_active;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110-st21nfca-v2-v2-0-cf9102b314cb%40gmail.com

[PATCH RESEND v2 2/2] nfc: st21nfca: prevent double completion of SE APDU request

[Joas Antonio dos Santos via B4 Relay]

From: Joas Antonio dos Santos <joasantonio108@gmail.com>

Use the spinlock added in the previous commit to prevent completing an
SE APDU request more than once.  Without this, the timeout work handler
(st21nfca_se_wt_work) and the EVT_TRANSMIT_DATA event handler can race
and both invoke the callback, leading to a use-after-free or double-free
of the callback context.

Check bwi_active under the lock in both completion paths and bail out if
the request was already completed.  Also reject a new SE APDU request in
st21nfca_hci_se_io() while one is already in progress by returning
-EBUSY.

Found by LLM-assisted manual code review.

Fixes: 2130fb97fecf ("NFC: st21nfca: Adding support for secure element")
Cc: Simon Horman <horms@kernel.org>
Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
---
 drivers/nfc/st21nfca/se.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
index 03a30f797ff8..3be77420e956 100644
--- a/drivers/nfc/st21nfca/se.c
+++ b/drivers/nfc/st21nfca/se.c
@@ -228,6 +228,10 @@ int st21nfca_hci_se_io(struct nfc_hci_dev *hdev, u32 se_idx,
 	switch (se_idx) {
 	case ST21NFCA_ESE_HOST_ID:
 		spin_lock(&info->se_info.se_lock);
+		if (info->se_info.bwi_active) {
+			spin_unlock(&info->se_info.se_lock);
+			return -EBUSY;
+		}
 		info->se_info.cb = cb;
 		info->se_info.cb_context = cb_context;
 		info->se_info.bwi_active = true;
@@ -269,6 +273,10 @@ static void st21nfca_se_wt_work(struct work_struct *work)
 						se_info.timeout_work);
 
 	spin_lock(&info->se_info.se_lock);
+	if (!info->se_info.bwi_active) {
+		spin_unlock(&info->se_info.se_lock);
+		return;
+	}
 	info->se_info.bwi_active = false;
 	cb = info->se_info.cb;
 	cb_context = info->se_info.cb_context;
@@ -395,6 +403,10 @@ int st21nfca_apdu_reader_event_received(struct nfc_hci_dev *hdev,
 		cancel_work_sync(&info->se_info.timeout_work);
 
 		spin_lock(&info->se_info.se_lock);
+		if (!info->se_info.bwi_active) {
+			spin_unlock(&info->se_info.se_lock);
+			break;
+		}
 		info->se_info.bwi_active = false;
 		cb = info->se_info.cb;
 		cb_context = info->se_info.cb_context;

-- 
2.43.0

Re: [PATCH RESEND v2 2/2] nfc: st21nfca: prevent double completion of SE APDU request

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 (2) · Medium: 2 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new -EBUSY early return in st21nfca_hci_se_io()
  (drivers/nfc/st21nfca/se.c:231-234) leaks cb_context.
- [Medium] nfc: st21nfca: race on bwi_active and bwi_timer across SE IO
  and RX paths
- [Low] st21nfca_hci_se_io() sets bwi_active = true (line 237) and arms
  bwi_timer (line 239) before calling nfc_hci_send_event() (line 241).
- [Low] In st21nfca_apdu_reader_event_received(), when
  ST21NFCA_EVT_TRANSMIT_DATA arrives with bwi_active == false (a…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the sibling ST
  driver drivers/nfc/st-nci/se.c still has the same double-completion…
- [High] This problem wasn't introduced by this patch, but
  st21nfca_se_deinit() (drivers/nfc/st21nfca/se.c:462-474) reads…

--

nfc: st21nfca: prevent double completion of SE APDU request

This commit checks bwi_active under se_lock in both the timeout work
handler and the EVT_TRANSMIT_DATA handler, so that an SE APDU request is
completed only once. It also makes st21nfca_hci_se_io() return -EBUSY
when a request is already in progress.

> diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
> index 03a30f797ff8f..3be77420e9569 100644
> --- a/drivers/nfc/st21nfca/se.c
> +++ b/drivers/nfc/st21nfca/se.c
> @@ -228,6 +228,10 @@ int st21nfca_hci_se_io(struct nfc_hci_dev *hdev, u32 se_idx,
>  	switch (se_idx) {
>  	case ST21NFCA_ESE_HOST_ID:
>  		spin_lock(&info->se_info.se_lock);
> +		if (info->se_info.bwi_active) {
> +			spin_unlock(&info->se_info.se_lock);
> +			return -EBUSY;
> +		}

[Severity: Medium]
Does this early return leak cb_context?

After nfc_se_io() in net/nfc/netlink.c calls the driver op, it returns rc
without freeing cb_context, whatever rc is:

net/nfc/netlink.c:nfc_se_io() {
    ...
	rc = dev->ops->se_io(dev, se_idx, apdu,
			apdu_length, cb, cb_context);

	device_unlock(&dev->dev);
	return rc;
}

The default branch of this same function frees the context before it
returns an error:

	default:
		/* Need to free cb_context here as at the moment we can't
		 * clearly indicate to the caller if the callback function
		 * would be called (and free it) or not. In both cases a
		 * negative value may be returned to the caller.
		 */
		kfree(cb_context);
		return -ENODEV;

The new -EBUSY path does not call cb and does not free cb_context. A
second NFC_CMD_SE_IO for the eSE, sent while an earlier APDU is still
outstanding, would then leak the struct se_io_ctx that nfc_genl_se_io()
allocated.

Should this path also call kfree(cb_context) before returning -EBUSY, the
same way the -ENODEV branch does?

>  		info->se_info.cb = cb;
>  		info->se_info.cb_context = cb_context;
>  		info->se_info.bwi_active = true;

[Severity: Low]
What happens to bwi_active if the send that follows fails? Right after
this hunk, st21nfca_hci_se_io() continues with:

		spin_unlock(&info->se_info.se_lock);
		mod_timer(&info->se_info.bwi_timer, jiffies +
			  msecs_to_jiffies(info->se_info.wt_timeout));
		return nfc_hci_send_event(hdev, ST21NFCA_APDU_READER_GATE,
					ST21NFCA_EVT_TRANSMIT_DATA,
					apdu, apdu_length);

nfc_hci_send_event() can fail without queueing anything to the
controller. It returns -EADDRNOTAVAIL for an invalid pipe, and
nfc_hci_hcp_message_tx() returns -ENOMEM or -ESHUTDOWN. In those cases
bwi_active stays true and the timer stays armed.

With the new -EBUSY check, every later eSE APDU request would be rejected
until the BWI timer expires. That takes 3.2 s by default and up to about
6553 s for an ATR-provided BWI of 15. Each rejected request would also
leak its ctx, as described above.

When the timer fires, st21nfca_se_wt_work() sees bwi_active == true. It
then sends EVT_SE_SOFT_RESET, or EVT_SE_HARD_RESET if xch_error is set,
to an SE that never received a command.

The missing rollback predates this patch, but before it a retry
overwrote the stale state. The -EBUSY blocking is new.

Could the failure path clear bwi_active under se_lock and call
timer_delete_sync() on the timer? It would also need to decide
explicitly who frees cb_context in that case.

> @@ -269,6 +273,10 @@ static void st21nfca_se_wt_work(struct work_struct *work)
>  						se_info.timeout_work);
>  
>  	spin_lock(&info->se_info.se_lock);
> +	if (!info->se_info.bwi_active) {
> +		spin_unlock(&info->se_info.se_lock);
> +		return;
> +	}

[Severity: High]
This is a pre-existing issue, but this check shows that bwi_timer can be
armed while bwi_active is false. Does st21nfca_se_deinit() handle that
case?

void st21nfca_se_deinit(struct nfc_hci_dev *hdev)
{
	...
	if (info->se_info.bwi_active)
		timer_delete_sync(&info->se_info.bwi_timer);
	...
	cancel_work_sync(&info->se_info.timeout_work);
	info->se_info.bwi_active = false;
	...
}

There are two ways to end up with an armed bwi_timer and bwi_active ==
false:

ST21NFCA_EVT_WTX_REQUEST in st21nfca_apdu_reader_event_received() calls
mod_timer() without checking bwi_active. A WTX that arrives after the
request has completed therefore re-arms the timer.

The previous patch in this series ("nfc: st21nfca: add spinlock to
serialize SE APDU request state") also opened a window.
st21nfca_hci_se_io() now sets bwi_active under se_lock but calls
mod_timer() only after dropping the lock. A late EVT_TRANSMIT_DATA can
complete the request inside that window. se_io then arms a timer for a
request that has already finished.

At teardown, st21nfca_se_deinit() reads bwi_active without se_lock, sees
false, and skips timer_delete_sync(). The cancel_work_sync() call does not
help if the timer fires after it. st21nfca_hci_remove() then frees info:

	st21nfca_se_deinit(hdev);
	nfc_hci_unregister_device(hdev);
	nfc_hci_free_device(hdev);
	kfree(info);

The pending timer would then run st21nfca_se_wt_timeout(), which calls
schedule_work(&info->se_info.timeout_work) on freed memory.

st21nfca_se_deinit() also runs before nfc_hci_unregister_device(), so rx
processing could still re-arm the timer after deinit. Deinit also clears
bwi_active without the lock, and it drops an active request's cb_context
without calling cb.

Would it be safer for st21nfca_se_deinit() to call timer_shutdown_sync()
unconditionally, then cancel_work_sync(), and to handle bwi_active and
cb_context under se_lock?

[ ... ]

> @@ -395,6 +403,10 @@ int st21nfca_apdu_reader_event_received(struct nfc_hci_dev *hdev,
>  		cancel_work_sync(&info->se_info.timeout_work);
>  
>  		spin_lock(&info->se_info.se_lock);
> +		if (!info->se_info.bwi_active) {
> +			spin_unlock(&info->se_info.se_lock);
> +			break;
> +		}

[Severity: Medium]
Can a late response to a timed out request complete the next request
instead?

st21nfca_apdu_reader_event_received() runs from hdev->msg_rx_work
without device_lock(). st21nfca_hci_se_io() is called from
nfc_se_io() with device_lock() held. That leaves se_lock as the only
thing ordering the two paths.

The rx path calls timer_delete_sync() and cancel_work_sync() before it
takes se_lock. The new bwi_active check then only tells it that some
request is active, not which one:

1. bwi_timer for request 1 fires. st21nfca_se_wt_work() clears
   bwi_active under se_lock and drops the lock. It then sends the
   soft reset and calls cb(ctx1, NULL, 0, -ETIME).

2. A late EVT_TRANSMIT_DATA for request 1 is processed on
   msg_rx_work. timer_delete_sync() finds nothing pending, and
   cancel_work_sync() waits for step 1 to finish.

3. Userspace gets -ETIME and issues request 2.
   st21nfca_hci_se_io() sees bwi_active == false and installs
   cb2/ctx2. It sets bwi_active, arms bwi_timer and sends APDU 2.

4. The rx handler takes se_lock, sees bwi_active == true and clears
   it. It then calls cb(ctx2, skb->data, skb->len, 0) with request
   1's response.

Request 2 is then reported as successful with the wrong APDU response.
Its bwi_timer stays armed while bwi_active is false, so the timeout
work bails out on the new check. When the real answer to APDU 2
arrives, the new check in the rx path drops it.

Step 3 can also land after cancel_work_sync() returns and before
spin_lock(). The -EBUSY check does not close the race, because
bwi_active is already false at that point.

The leftover armed timer with bwi_active == false is the same state
that st21nfca_se_deinit() does not handle, as described above. This
is therefore another way to reach the timer use-after-free on remove.

This interleaving is not new. However, the commit message says a
request is now completed only once. Because the check runs after the
sync cancels, it can still claim a request that was submitted after
the response arrived.

Is there any way to tell which request an EVT_TRANSMIT_DATA belongs
to? If not, could st21nfca_hci_se_io() keep returning -EBUSY until
the timeout recovery in st21nfca_se_wt_work() has finished? Could the
rx path also decide under se_lock whether there is a request to
complete, before it touches bwi_timer?

[Severity: Low]
Is it intended that this break also skips the end-of-transfer event to
the CLF? After the request state is claimed, the handler goes on to:

		r = nfc_hci_send_event(hdev, ST21NFCA_DEVICE_MGNT_GATE,
				ST21NFCA_EVT_SE_END_OF_APDU_TRANSFER, NULL, 0);
		if (r < 0)
			goto exit;

		cb(cb_context, skb->data, skb->len, 0);

Before this patch, a response that arrived after the BWI timeout still
caused ST21NFCA_EVT_SE_END_OF_APDU_TRANSFER to be sent. Now a late or
unsolicited EVT_TRANSMIT_DATA skips both the stale callback and that
event.

Depending on CLF firmware behaviour, this might leave the CLF/SWP link in
APDU reader state. It is not clear whether the soft or hard reset sent
earlier by the timeout path already clears that state.

Would it be safer to skip only the callback here and still send
END_OF_APDU_TRANSFER?

>  		info->se_info.bwi_active = false;
>  		cb = info->se_info.cb;
>  		cb_context = info->se_info.cb_context;

[Severity: High]
This isn't a bug introduced by this patch, but the sibling driver
drivers/nfc/st-nci/se.c appears to have the same double completion
problem with no guard at all. Should it be fixed as well?

drivers/nfc/st-nci/se.c:st_nci_hci_apdu_reader_event_received() {
    ...
	case ST_NCI_EVT_TRANSMIT_DATA:
		timer_delete_sync(&info->se_info.bwi_timer);
		info->se_info.bwi_active = false;
		info->se_info.cb(info->se_info.cb_context,
				 skb->data, skb->len, 0);
    ...
}

drivers/nfc/st-nci/se.c:st_nci_se_wt_timeout() {
    ...
	info->se_info.bwi_active = false;
    ...
	info->se_info.cb(info->se_info.cb_context, NULL, 0, -ETIME);
}

If the BWI timer fires first, se_io_cb() frees the ctx. A later
ST_NCI_EVT_TRANSMIT_DATA then passes the freed ctx to se_io_cb() a second
time, which reads ctx->dev_idx and calls kfree(ctx) again.

ST_NCI_EVT_WTX_REQUEST also re-arms the timer unconditionally. A WTX that
arrives after a completed request would make the timeout complete the
dead request again.

st_nci_se_io() has no in-flight check and takes no lock. An unsolicited
TRANSMIT_DATA that arrives before any se_io would call a NULL cb.

In st-nci the timeout completion also runs directly in timer (softirq)
context, where se_io_cb() calls nlmsg_new() with GFP_KERNEL.

The commit message does not say whether st-nci was considered.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110-st21nfca-v2-v2-0-cf9102b314cb%40gmail.com

Re: [PATCH RESEND v2 0/2] nfc: st21nfca: fix SE APDU request race condition

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.