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

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

[Yuchao Zhang]

Commit bcd684aace34 ("net/nfc/nci: Support NCI 2.x initial sequence")
added handling of CORE_RESET_NTF in nci_core_reset_ntf_packet(). When
received, it updates ndev->nci_ver, ndev->manufact_id, and
ndev->manufact_specific_info, and calls nci_req_complete(ndev,
NCI_STATUS_OK) to finish the pending reset request.

However, unlike other notification handlers in ntf.c (which validate
ndev->state before completing requests), nci_core_reset_ntf_packet()
does not check whether a core reset request is actually pending.
If an unsolicited or delayed CORE_RESET_NTF arrives (e.g. after a reset
command times out or from a misbehaving NFCC), it unconditionally:
1. Completes whatever request is currently in-flight (such as CORE_INIT,
   RF_DISCOVER, or CONN_CREATE) with NCI_STATUS_OK, leading to kernel
   state desynchronization.
2. Overwrites ndev->nci_ver and manufacturer info. Because
   ndev->nci_ver is used as a selector for subsequent packet formats
   and parsers (e.g., in nci_open_device() and nci_core_init_rsp_packet()),
   unexpectedly modifying it can cause protocol format confusion.

A similar issue exists in nci_core_reset_rsp_packet(): an unexpected or
delayed response packet can prematurely complete an unrelated in-flight
request.

Fix this by ensuring CORE_RESET_NTF and CORE_RESET_RSP are only processed
by the core layer when a reset command is actively awaiting them:
- Set NCI_RESET_PENDING in nci_reset_req() when sending CORE_RESET_CMD.
- In nci_core_reset_rsp_packet(), ignore the response if NCI_RESET_PENDING
  is not set. If set, clear the flag and complete the request on failure
  or for NCI 1.x (checking skb->len >= sizeof(*rsp)).
- In __nci_request(), ensure NCI_RESET_PENDING is cleared upon request
  completion, cancellation, or timeout.
- In nci_core_reset_ntf_packet(), skip power-on notifications (trigger
  0x01, identical in NCI 1.0 and 2.0), then check and clear
  NCI_RESET_PENDING before updating device fields and completing the
  request. The CORE_RESET_CMD trigger value is revision-dependent
  (0x00 in NCI 1.0, 0x02 in NCI 2.0), so both are accepted and gated
  on NCI_RESET_PENDING alone. If unexpected, log a warning and return
  0 so driver-specific handlers (such as fdp firmware patch handling)
  still receive the notification.

Fixes: bcd684aace34 ("net/nfc/nci: Support NCI 2.x initial sequence")
Cc: stable@vger.kernel.org
Signed-off-by: Yuchao Zhang <ndaugoing@gmail.com>
---
v2:
 - Do not abort the notification pipeline on unexpected CORE_RESET_NTF (return 0
   instead of -EINVAL), preserving driver-level hooks (e.g. fdp firmware
   patching) per Simon Horman.
 - Skip power-on notifications (trigger 0x01, identical in NCI 1.0 and 2.0)
   while gating both NCI 1.0 (0x00) and NCI 2.0 (0x02) command-triggered resets
   on NCI_RESET_PENDING alone.
 - Check NCI_RESET_PENDING in nci_core_reset_rsp_packet() to avoid completing
   unrelated requests on unexpected responses.
 - Explicitly check skb->len >= sizeof(*rsp) in nci_core_reset_rsp_packet()
   for NCI 1.x handling.

 include/net/nfc/nci.h      |  5 +++++
 include/net/nfc/nci_core.h |  1 +
 net/nfc/nci/core.c         |  3 +++
 net/nfc/nci/ntf.c          | 17 ++++++++++++++++-
 net/nfc/nci/rsp.c          | 24 ++++++++++++++++++------
 5 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/include/net/nfc/nci.h b/include/net/nfc/nci.h
index 09efcaed7c3f..2ac45adba088 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
+
 /* NCI Static RF connection ID */
 #define NCI_STATIC_RF_CONN_ID					0x00
 
diff --git a/include/net/nfc/nci_core.h b/include/net/nfc/nci_core.h
index 664d5058e66e..504ea91ddf27 100644
--- a/include/net/nfc/nci_core.h
+++ b/include/net/nfc/nci_core.h
@@ -31,6 +31,7 @@ enum nci_flag {
 	NCI_DATA_EXCHANGE,
 	NCI_DATA_EXCHANGE_TO,
 	NCI_UNREG,
+	NCI_RESET_PENDING,
 };
 
 /* NCI device states */
diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
index 5f46c4b5720f..5a90591cd9af 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 f5c9a8ab7ec1..57f99bf46a22 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;
 
-	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;
+	}
 
 	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 b0ab4f5acbce..3a2491497e4b 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;
+
+	if (rsp->status != NCI_STATUS_OK) {
+		clear_bit(NCI_RESET_PENDING, &ndev->flags);
+		nci_req_complete(ndev, rsp->status);
+		return;
+	}
+
 	/* 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);
 	}
 }
-- 
2.53.0

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · 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] The NCI_RESET_PENDING gate is evaluated in workqueue context
  (nci_rx_work() -> nci_rsp_packet()/nci_ntf_packet(),…
- [Low] The patch adds contradictory reset-trigger semantics in two
  artifacts of the same commit: include/net/nfc/nci.h:143-146 defines…

--

nfc: nci: ignore unexpected CORE_RESET_NTF and CORE_RESET_RSP

This adds an NCI_RESET_PENDING flag which is set when CORE_RESET_CMD is
sent and cleared when the request finishes, and gates the core layer
handling of CORE_RESET_RSP and CORE_RESET_NTF on that flag.  The intent is
that an unsolicited or delayed reset packet no longer completes an
unrelated in-flight request nor overwrites ndev->nci_ver and the
manufacturer fields.

A couple of questions 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: Low]
Do these new definitions agree with the comment added in
nci_core_reset_ntf_packet() in the same patch?

Here 0x00 is named NCI_RESET_TRIGGER_ERROR and 0x02 is named
NCI_RESET_TRIGGER_RESET_CMD, but the new comment in ntf.c says:

	 * revision-dependent (0x00 in NCI 1.0, 0x02 in NCI 2.0), so both
	 * are let through to the reset-pending gate below instead.

The in-tree selftest tools/testing/selftests/nci/nci_dev.c uses 0x02 as
the first payload byte of its CORE_RESET_NTF, which matches
NCI_RESET_TRIGGER_RESET_CMD but not the 0x00 claim.

Also, only NCI_RESET_TRIGGER_POWER_ON is used anywhere;
NCI_RESET_TRIGGER_ERROR and NCI_RESET_TRIGGER_RESET_CMD have no
references under net/nfc or drivers/nfc.  Should the unused ones be
dropped, or should the code actually test for
NCI_RESET_TRIGGER_RESET_CMD?

Following from that, if a notification with reset_trigger == 0x00 arrives
while NCI_RESET_PENDING is set, the handler treats it as a successful
command-triggered reset:

	ndev->nci_ver = ntf->nci_ver;
	...
	nci_req_complete(ndev, NCI_STATUS_OK);

Is reporting NCI_STATUS_OK correct for a trigger the header names ERROR?

[ ... ]

> 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);
>  }

[Severity: Medium]
Is the flag enough to keep a late reset packet from completing a
different request?

NCI_RESET_PENDING is set and cleared here in process context under
req_lock, while the gate is evaluated from the rx workqueue
nci_rx_work() -> nci_rsp_packet()/nci_ntf_packet(), which never takes
req_lock.  So the gate test, the flag clear and nci_req_complete() are
three separate steps with respect to the request lifetime:

rx_wq
nci_core_reset_rsp_packet()
	if (!test_bit(NCI_RESET_PENDING, &ndev->flags))
		return;		/* passes, then gets preempted */

process context
__nci_request()
	rc = -ETIMEDOUT;
	ndev->req_status = ndev->req_result = 0;
	clear_bit(NCI_RESET_PENDING, &ndev->flags);

nci_open_device() then fails and drops req_lock.  Its failure path purges
cmd_q/rx_q/tx_q and calls ops->close(), but does not flush ndev->rx_wq,
so the handler above can still be mid-execution while a retry starts a
new request and sets req_status = NCI_REQ_PEND.  When the handler
resumes:

	clear_bit(NCI_RESET_PENDING, &ndev->flags);
	nci_req_complete(ndev, rsp->status);

and nci_req_complete() only checks req_status == NCI_REQ_PEND, with no
opcode or generation matching, so the new request (CORE_INIT, for
instance) is completed with NCI_STATUS_OK without any of its fields
being parsed.

The same window exists in nci_core_reset_ntf_packet() between the
successful test_and_clear_bit() and nci_req_complete(), so switching the
response handler to test_and_clear_bit() would not close it.  Would
evaluating the gate and calling nci_req_complete() under req_lock, or
matching the completion to the pending opcode, be a better fit than the
flag?

> 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;
>  
> -	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;
> +	}
>  
>  	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;
> +
> +	if (rsp->status != NCI_STATUS_OK) {
> +		clear_bit(NCI_RESET_PENDING, &ndev->flags);
> +		nci_req_complete(ndev, rsp->status);
> +		return;
> +	}
> +
>  	/* 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/20260922102144.39689-1-ndaugoing%40gmail.com

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

[netdev-bot+sashiko]

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

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

[Yuchao Zhang]

Hi David,

Thanks a lot for the pointer and explanation! I apologize for the
threading confusion — I mistakenly chained the v2 patch under the
previous review message using --in-reply-to.

I'll re-send the v2 patch as a clean, standalone thread right away so
that patchwork can track it properly, and I'll definitely adopt b4 for
future submissions.

Thanks again for keeping me on the right track!

Best regards,
Yuchao

David Heidelberg <david@ixit.cz> 于2026年9月22日周二 18:05写道：
>
> On 22/09/2026 10:07, Yuchao Zhang wrote:
> > Commit bcd684aace34 ("net/nfc/nci: Support NCI 2.x initial sequence")
> > added handling of CORE_RESET_NTF in nci_core_reset_ntf_packet(). When
> > received, it updates ndev->nci_ver, ndev->manufact_id, and
> > ndev->manufact_specific_info, and calls nci_req_complete(ndev,
> > NCI_STATUS_OK) to finish the pending reset request.
> >
> > However, unlike other notification handlers in ntf.c (which validate
> > ndev->state before completing requests), nci_core_reset_ntf_packet()
> > does not check whether a core reset request is actually pending.
> > If an unsolicited or delayed CORE_RESET_NTF arrives (e.g. after a reset
> > command times out or from a misbehaving NFCC), it unconditionally:
> > 1. Completes whatever request is currently in-flight (such as CORE_INIT,
> >     RF_DISCOVER, or CONN_CREATE) with NCI_STATUS_OK, leading to kernel
> >     state desynchronization.
> > 2. Overwrites ndev->nci_ver and manufacturer info. Because
> >     ndev->nci_ver is used as a selector for subsequent packet formats
> >     and parsers (e.g., in nci_open_device() and nci_core_init_rsp_packet()),
> >     unexpectedly modifying it can cause protocol format confusion.
> >
> > A similar issue exists in nci_core_reset_rsp_packet(): an unexpected or
> > delayed response packet can prematurely complete an unrelated in-flight
> > request.
> >
> > Fix this by ensuring CORE_RESET_NTF and CORE_RESET_RSP are only processed
> > by the core layer when a reset command is actively awaiting them:
> > - Set NCI_RESET_PENDING in nci_reset_req() when sending CORE_RESET_CMD.
> > - In nci_core_reset_rsp_packet(), ignore the response if NCI_RESET_PENDING
> >    is not set. If set, clear the flag and complete the request on failure
> >    or for NCI 1.x (checking skb->len >= sizeof(*rsp)).
> > - In __nci_request(), ensure NCI_RESET_PENDING is cleared upon request
> >    completion, cancellation, or timeout.
> > - In nci_core_reset_ntf_packet(), skip power-on notifications (trigger
> >    0x01, identical in NCI 1.0 and 2.0), then check and clear
> >    NCI_RESET_PENDING before updating device fields and completing the
> >    request. The CORE_RESET_CMD trigger value is revision-dependent
> >    (0x00 in NCI 1.0, 0x02 in NCI 2.0), so both are accepted and gated
> >    on NCI_RESET_PENDING alone. If unexpected, log a warning and return
> >    0 so driver-specific handlers (such as fdp firmware patch handling)
> >    still receive the notification.
> >
> > Fixes: bcd684aace34 ("net/nfc/nci: Support NCI 2.x initial sequence")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Yuchao Zhang <ndaugoing@gmail.com>
> > ---
> > v2:
> >   - Do not abort the notification pipeline on unexpected CORE_RESET_NTF (return 0
> >     instead of -EINVAL), preserving driver-level hooks (e.g. fdp firmware
> >     patching) per Simon Horman.
> >   - Skip power-on notifications (trigger 0x01, identical in NCI 1.0 and 2.0)
> >     while gating both NCI 1.0 (0x00) and NCI 2.0 (0x02) command-triggered resets
> >     on NCI_RESET_PENDING alone.
> >   - Check NCI_RESET_PENDING in nci_core_reset_rsp_packet() to avoid completing
> >     unrelated requests on unexpected responses.
> >   - Explicitly check skb->len >= sizeof(*rsp) in nci_core_reset_rsp_packet()
> >     for NCI 1.x handling.
> Hello Yuchao,
>
> please never send follow-ups as part of the thread. Always send them as a
> separate message (otherwise they may get lost and we already have patchwork wich
> can track the submissions.
>
> Even better, try to use tools such as b4 to submit patches (easier for you, sent
> in expected format for us).
>
> Thanks
> David

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

[David Heidelberg]

On 22/09/2026 10:07, Yuchao Zhang wrote:
> Commit bcd684aace34 ("net/nfc/nci: Support NCI 2.x initial sequence")
> added handling of CORE_RESET_NTF in nci_core_reset_ntf_packet(). When
> received, it updates ndev->nci_ver, ndev->manufact_id, and
> ndev->manufact_specific_info, and calls nci_req_complete(ndev,
> NCI_STATUS_OK) to finish the pending reset request.
> 
> However, unlike other notification handlers in ntf.c (which validate
> ndev->state before completing requests), nci_core_reset_ntf_packet()
> does not check whether a core reset request is actually pending.
> If an unsolicited or delayed CORE_RESET_NTF arrives (e.g. after a reset
> command times out or from a misbehaving NFCC), it unconditionally:
> 1. Completes whatever request is currently in-flight (such as CORE_INIT,
>     RF_DISCOVER, or CONN_CREATE) with NCI_STATUS_OK, leading to kernel
>     state desynchronization.
> 2. Overwrites ndev->nci_ver and manufacturer info. Because
>     ndev->nci_ver is used as a selector for subsequent packet formats
>     and parsers (e.g., in nci_open_device() and nci_core_init_rsp_packet()),
>     unexpectedly modifying it can cause protocol format confusion.
> 
> A similar issue exists in nci_core_reset_rsp_packet(): an unexpected or
> delayed response packet can prematurely complete an unrelated in-flight
> request.
> 
> Fix this by ensuring CORE_RESET_NTF and CORE_RESET_RSP are only processed
> by the core layer when a reset command is actively awaiting them:
> - Set NCI_RESET_PENDING in nci_reset_req() when sending CORE_RESET_CMD.
> - In nci_core_reset_rsp_packet(), ignore the response if NCI_RESET_PENDING
>    is not set. If set, clear the flag and complete the request on failure
>    or for NCI 1.x (checking skb->len >= sizeof(*rsp)).
> - In __nci_request(), ensure NCI_RESET_PENDING is cleared upon request
>    completion, cancellation, or timeout.
> - In nci_core_reset_ntf_packet(), skip power-on notifications (trigger
>    0x01, identical in NCI 1.0 and 2.0), then check and clear
>    NCI_RESET_PENDING before updating device fields and completing the
>    request. The CORE_RESET_CMD trigger value is revision-dependent
>    (0x00 in NCI 1.0, 0x02 in NCI 2.0), so both are accepted and gated
>    on NCI_RESET_PENDING alone. If unexpected, log a warning and return
>    0 so driver-specific handlers (such as fdp firmware patch handling)
>    still receive the notification.
> 
> Fixes: bcd684aace34 ("net/nfc/nci: Support NCI 2.x initial sequence")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yuchao Zhang <ndaugoing@gmail.com>
> ---
> v2:
>   - Do not abort the notification pipeline on unexpected CORE_RESET_NTF (return 0
>     instead of -EINVAL), preserving driver-level hooks (e.g. fdp firmware
>     patching) per Simon Horman.
>   - Skip power-on notifications (trigger 0x01, identical in NCI 1.0 and 2.0)
>     while gating both NCI 1.0 (0x00) and NCI 2.0 (0x02) command-triggered resets
>     on NCI_RESET_PENDING alone.
>   - Check NCI_RESET_PENDING in nci_core_reset_rsp_packet() to avoid completing
>     unrelated requests on unexpected responses.
>   - Explicitly check skb->len >= sizeof(*rsp) in nci_core_reset_rsp_packet()
>     for NCI 1.x handling.
Hello Yuchao,

please never send follow-ups as part of the thread. Always send them as a 
separate message (otherwise they may get lost and we already have patchwork wich 
can track the submissions.

Even better, try to use tools such as b4 to submit patches (easier for you, sent 
in expected format for us).

Thanks
David

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

[Yuchao Zhang]

Commit bcd684aace34 ("net/nfc/nci: Support NCI 2.x initial sequence")
added handling of CORE_RESET_NTF in nci_core_reset_ntf_packet(). When
received, it updates ndev->nci_ver, ndev->manufact_id, and
ndev->manufact_specific_info, and calls nci_req_complete(ndev,
NCI_STATUS_OK) to finish the pending reset request.

However, unlike other notification handlers in ntf.c (which validate
ndev->state before completing requests), nci_core_reset_ntf_packet()
does not check whether a core reset request is actually pending.
If an unsolicited or delayed CORE_RESET_NTF arrives (e.g. after a reset
command times out or from a misbehaving NFCC), it unconditionally:
1. Completes whatever request is currently in-flight (such as CORE_INIT,
   RF_DISCOVER, or CONN_CREATE) with NCI_STATUS_OK, leading to kernel
   state desynchronization.
2. Overwrites ndev->nci_ver and manufacturer info. Because
   ndev->nci_ver is used as a selector for subsequent packet formats
   and parsers (e.g., in nci_open_device() and nci_core_init_rsp_packet()),
   unexpectedly modifying it can cause protocol format confusion.

A similar issue exists in nci_core_reset_rsp_packet(): an unexpected or
delayed response packet can prematurely complete an unrelated in-flight
request.

Fix this by ensuring CORE_RESET_NTF and CORE_RESET_RSP are only processed
by the core layer when a reset command is actively awaiting them:
- Set NCI_RESET_PENDING in nci_reset_req() when sending CORE_RESET_CMD.
- In nci_core_reset_rsp_packet(), ignore the response if NCI_RESET_PENDING
  is not set. If set, clear the flag and complete the request on failure
  or for NCI 1.x (checking skb->len >= sizeof(*rsp)).
- In __nci_request(), ensure NCI_RESET_PENDING is cleared upon request
  completion, cancellation, or timeout.
- In nci_core_reset_ntf_packet(), skip power-on notifications (trigger
  0x01, identical in NCI 1.0 and 2.0), then check and clear
  NCI_RESET_PENDING before updating device fields and completing the
  request. The CORE_RESET_CMD trigger value is revision-dependent
  (0x00 in NCI 1.0, 0x02 in NCI 2.0), so both are accepted and gated
  on NCI_RESET_PENDING alone. If unexpected, log a warning and return
  0 so driver-specific handlers (such as fdp firmware patch handling)
  still receive the notification.

Fixes: bcd684aace34 ("net/nfc/nci: Support NCI 2.x initial sequence")
Cc: stable@vger.kernel.org
Signed-off-by: Yuchao Zhang <ndaugoing@gmail.com>
---
v2:
 - Do not abort the notification pipeline on unexpected CORE_RESET_NTF (return 0
   instead of -EINVAL), preserving driver-level hooks (e.g. fdp firmware
   patching) per Simon Horman.
 - Skip power-on notifications (trigger 0x01, identical in NCI 1.0 and 2.0)
   while gating both NCI 1.0 (0x00) and NCI 2.0 (0x02) command-triggered resets
   on NCI_RESET_PENDING alone.
 - Check NCI_RESET_PENDING in nci_core_reset_rsp_packet() to avoid completing
   unrelated requests on unexpected responses.
 - Explicitly check skb->len >= sizeof(*rsp) in nci_core_reset_rsp_packet()
   for NCI 1.x handling.

 include/net/nfc/nci.h      |  5 +++++
 include/net/nfc/nci_core.h |  1 +
 net/nfc/nci/core.c         |  3 +++
 net/nfc/nci/ntf.c          | 17 ++++++++++++++++-
 net/nfc/nci/rsp.c          | 24 ++++++++++++++++++------
 5 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/include/net/nfc/nci.h b/include/net/nfc/nci.h
index 09efcaed7c3f..2ac45adba088 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
+
 /* NCI Static RF connection ID */
 #define NCI_STATIC_RF_CONN_ID					0x00
 
diff --git a/include/net/nfc/nci_core.h b/include/net/nfc/nci_core.h
index 664d5058e66e..504ea91ddf27 100644
--- a/include/net/nfc/nci_core.h
+++ b/include/net/nfc/nci_core.h
@@ -31,6 +31,7 @@ enum nci_flag {
 	NCI_DATA_EXCHANGE,
 	NCI_DATA_EXCHANGE_TO,
 	NCI_UNREG,
+	NCI_RESET_PENDING,
 };
 
 /* NCI device states */
diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
index 5f46c4b5720f..5a90591cd9af 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 f5c9a8ab7ec1..57f99bf46a22 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;
 
-	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;
+	}
 
 	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 b0ab4f5acbce..3a2491497e4b 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;
+
+	if (rsp->status != NCI_STATUS_OK) {
+		clear_bit(NCI_RESET_PENDING, &ndev->flags);
+		nci_req_complete(ndev, rsp->status);
+		return;
+	}
+
 	/* 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);
 	}
 }
-- 
2.53.0