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

[Simon Horman]

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
>