[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

[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