Re: [PATCH v2] nfc: nci: ignore unexpected CORE_RESET_NTF and CORE_RESET_RSP

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [High] nci_core_reset_ntf_packet() still rejects any CORE_RESET_NTF
  shorter than sizeof(struct nci_core_reset_ntf) (9 bytes) with -EINVAL…
- [Medium] Contradictory trigger semantics introduced by the same patch,
  plus dead constants.
- [Medium] The new NCI_RESET_PENDING gate is a single boolean token, and
  in nci_core_reset_rsp_packet() it is consumed non-atomically (plain…

--

nfc: nci: ignore unexpected CORE_RESET_NTF and CORE_RESET_RSP

This adds a new NCI_RESET_PENDING flag that is set when CORE_RESET_CMD is
sent and checked in the CORE_RESET_RSP and CORE_RESET_NTF handlers, so
that unsolicited or delayed reset packets no longer complete an unrelated
in-flight request or overwrite ndev->nci_ver and the manufacturer info.
A few questions on the new gating logic below.

> diff --git a/include/net/nfc/nci.h b/include/net/nfc/nci.h
> index 09efcaed7c3f6..2ac45adba0886 100644
> --- a/include/net/nfc/nci.h
> +++ b/include/net/nfc/nci.h
> @@ -140,6 +140,11 @@
>  #define NCI_RESET_TYPE_KEEP_CONFIG				0x00
>  #define NCI_RESET_TYPE_RESET_CONFIG				0x01
>  
> +/* NCI Reset Triggers */
> +#define NCI_RESET_TRIGGER_ERROR					0x00
> +#define NCI_RESET_TRIGGER_POWER_ON				0x01
> +#define NCI_RESET_TRIGGER_RESET_CMD				0x02
> +

[Severity: Medium]
Are these two descriptions of trigger 0x00 consistent?  Here 0x00 is named
NCI_RESET_TRIGGER_ERROR, while the comment added to
nci_core_reset_ntf_packet() in the same patch says 0x00 is the
CORE_RESET_CMD trigger for NCI 1.0:

	 * revision-dependent (0x00 in NCI 1.0, 0x02 in NCI 2.0), so both

Only one of those can hold.

Also, NCI_RESET_TRIGGER_ERROR and NCI_RESET_TRIGGER_RESET_CMD are not
referenced anywhere; only NCI_RESET_TRIGGER_POWER_ON has a single use in
nci_core_reset_ntf_packet().  Would it be clearer to either use the
constants in the trigger check or drop the unused ones?

>  /* NCI Static RF connection ID */
>  #define NCI_STATIC_RF_CONN_ID					0x00
>  

[ ... ]

> diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
> index 5f46c4b5720f6..5a90591cd9af7 100644
> --- a/net/nfc/nci/core.c
> +++ b/net/nfc/nci/core.c
> @@ -134,6 +134,7 @@ static int __nci_request(struct nci_dev *ndev,
>  	}
>  
>  	ndev->req_status = ndev->req_result = 0;
> +	clear_bit(NCI_RESET_PENDING, &ndev->flags);
>  
>  	return rc;
>  }
> @@ -163,6 +164,8 @@ static void nci_reset_req(struct nci_dev *ndev, const void *opt)
>  {
>  	struct nci_core_reset_cmd cmd;
>  
> +	set_bit(NCI_RESET_PENDING, &ndev->flags);
> +
>  	cmd.reset_type = NCI_RESET_TYPE_RESET_CONFIG;
>  	nci_send_cmd(ndev, NCI_OP_CORE_RESET_CMD, 1, &cmd);
>  }
> diff --git a/net/nfc/nci/ntf.c b/net/nfc/nci/ntf.c
> index f5c9a8ab7ec10..57f99bf46a221 100644
> --- a/net/nfc/nci/ntf.c
> +++ b/net/nfc/nci/ntf.c
> @@ -36,7 +36,22 @@ static int nci_core_reset_ntf_packet(struct nci_dev *ndev,
>  	if (skb->len < sizeof(struct nci_core_reset_ntf))
>  		return -EINVAL;
       ^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a length check introduced by this patch (it came in with
9c328f54741bd5), but does it undermine the commit message's stated
premise?

The changelog says:

    If unexpected, log a warning and return 0 so driver-specific handlers
    (such as fdp firmware patch handling) still receive the notification.

struct nci_core_reset_ntf is 9 bytes, so any shorter notification returns
-EINVAL here, and nci_ntf_packet() then takes:

	case NCI_OP_CORE_RESET_NTF:
		if (nci_core_reset_ntf_packet(ndev, skb))
			goto end;

which skips nci_core_ntf_packet() entirely, the only dispatcher for the
per-driver .ntf callbacks.  Shorter notifications look common: an NCI 1.x
CORE_RESET_NTF carries 2 payload octets, and drivers/nfc/nfcmrvl/fw_dnld.c
documents such a frame:

drivers/nfc/nfcmrvl/fw_dnld.c:
static const u8 nci_pattern_core_reset_ntf[] = {
	0x60, 0x00, 0x02, 0xA0, 0x01
};

A conformant NCI 2.0 notification with manufacturer_specific_len == 0 is
5 octets and is rejected as well.

For fdp, the skipped hook is the only thing that wakes the firmware setup
path:

drivers/nfc/fdp/fdp.c:fdp_nci_core_reset_ntf_packet() {
	info->setup_reset_ntf = 1;
	wake_up(&info->setup_wq);
}

and fdp_nci_patch_ram()/fdp_nci_patch_otp() block with no timeout:

drivers/nfc/fdp/fdp.c:fdp_nci_patch_ram() {
	...
	wait_event_interruptible(info->setup_wq, info->setup_reset_ntf);
}

So if such a controller emits a short CORE_RESET_NTF after applying the
patch, does the setup path sleep indefinitely (only a signal breaks it)?
Would capping the field reads at the received length be preferable to
rejecting short notifications outright?

> -	ntf = (struct nci_core_reset_ntf *)skb->data;
> +	ntf = (const struct nci_core_reset_ntf *)skb->data;
> +
> +	/*
> +	 * A power-on notification must not update core protocol state nor
> +	 * complete a pending request.  The CORE_RESET_CMD trigger value is
> +	 * revision-dependent (0x00 in NCI 1.0, 0x02 in NCI 2.0), so both
> +	 * are let through to the reset-pending gate below instead.
> +	 * Return 0 so driver-specific notification hooks can still run.
> +	 */
> +	if (ntf->reset_trigger == NCI_RESET_TRIGGER_POWER_ON)
> +		return 0;
> +
> +	if (!test_and_clear_bit(NCI_RESET_PENDING, &ndev->flags)) {
> +		pr_warn_ratelimited("unexpected CORE_RESET_NTF\n");
> +		return 0;
> +	}

[Severity: Medium]
Only trigger 0x01 is filtered here.  Every other value reaches the pending
gate, including 0x00 (which the new header names
NCI_RESET_TRIGGER_ERROR), the RFU range 0x03-0x9F and the proprietary
range 0xA0-0xFF, and real hardware does use the proprietary range
(drivers/nfc/nfcmrvl/fw_dnld.c uses trigger 0xA0).

When a reset is pending, is it intended that an error or proprietary
trigger notification stamps ndev->nci_ver, ndev->manufact_id and
ndev->manufact_specific_info and completes the request with
NCI_STATUS_OK?  The notification content comes from the controller, and
/dev/virtual_nci (virtual_ncidev_write() -> nci_recv_frame()) lets
userspace supply any trigger value.

>  
>  	ndev->nci_ver = ntf->nci_ver;
>  	pr_debug("nci_ver 0x%x, config_status 0x%x\n",
> diff --git a/net/nfc/nci/rsp.c b/net/nfc/nci/rsp.c
> index b0ab4f5acbce8..3a2491497e4b3 100644
> --- a/net/nfc/nci/rsp.c
> +++ b/net/nfc/nci/rsp.c
> @@ -32,14 +32,26 @@ static void nci_core_reset_rsp_packet(struct nci_dev *ndev,
>  
>  	pr_debug("status 0x%x\n", rsp->status);
>  
> +	/*
> +	 * If no reset request is pending, ignore unexpected responses to avoid
> +	 * prematurely completing an unrelated request.
> +	 */
> +	if (!test_bit(NCI_RESET_PENDING, &ndev->flags))
> +		return;
> +

[Severity: Medium]
Should this be test_and_clear_bit() for consistency with the notification
handler?  As written the flag is tested here and only cleared further down,
so the window in between is open to the request thread:

nci_rx_work() -> nci_rsp_packet() -> nci_core_reset_rsp_packet()
	test_bit(NCI_RESET_PENDING) -> true   /* stale response */

request thread, timing out in __nci_request():
	ndev->req_status = ndev->req_result = 0;
	clear_bit(NCI_RESET_PENDING, &ndev->flags);

next reset, nci_reset_req():
	set_bit(NCI_RESET_PENDING, &ndev->flags);

back in nci_core_reset_rsp_packet():
	clear_bit(NCI_RESET_PENDING, &ndev->flags);
	nci_req_complete(ndev, rsp->status);

Can the stale response then complete the new request?

More generally, a single boolean carries no information about which reset
a packet belongs to.  A delayed or duplicated CORE_RESET_RSP/CORE_RESET_NTF
from an earlier reset passes the gate of a later one, for example the
nci_core_reset() issued from fdp_nci_post_setup() after the reset done in
nci_open_device().  That completes the later request with stale status and
nci_ver while the NFCC is still resetting, and the genuine response is
then dropped as unexpected.

Would a per-request generation or sequence number be a better match for
what the gate needs to express?

>  	/* Handle NCI 1.x ver */
> -	if (skb->len != 1) {
> -		if (rsp->status == NCI_STATUS_OK) {
> -			ndev->nci_ver = rsp->nci_ver;
> -			pr_debug("nci_ver 0x%x, config_status 0x%x\n",
> -				 rsp->nci_ver, rsp->config_status);
> -		}
> +	if (skb->len >= sizeof(*rsp)) {
> +		ndev->nci_ver = rsp->nci_ver;
> +		pr_debug("nci_ver 0x%x, config_status 0x%x\n",
> +			 rsp->nci_ver, rsp->config_status);
>  
> +		clear_bit(NCI_RESET_PENDING, &ndev->flags);
>  		nci_req_complete(ndev, rsp->status);
>  	}
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922080750.35929-1-ndaugoing%40gmail.com