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

[netdev-bot+sashiko@kernel.org]

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