[PATCH 1/1] nfc: nci: ignore unexpected CORE_RESET_NTF

[PATCH 1/1] nfc: nci: ignore unexpected CORE_RESET_NTF

[zjamg]

From: Yuchao Zhang <ndaugoing@gmail.com>

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.

Introduce an NCI_RESET_PENDING flag to ensure CORE_RESET_NTF is only
processed when a reset command is actively awaiting it:
- Set NCI_RESET_PENDING in nci_reset_req() when sending CORE_RESET_CMD.
- In nci_core_reset_rsp_packet(), if the reset failed or if handling
  NCI 1.x (where no notification is expected), clear NCI_RESET_PENDING
  and complete the request immediately.
- In __nci_request(), ensure NCI_RESET_PENDING is cleared upon request
  completion, cancellation, or timeout.
- In nci_core_reset_ntf_packet(), check and clear NCI_RESET_PENDING
  before updating device fields and completing the request. If the flag
  is not set, log a warning and drop the packet.

Fixes: bcd684aace34 ("net/nfc/nci: Support NCI 2.x initial sequence")
Cc: stable@vger.kernel.org
Signed-off-by: Yuchao Zhang <ndaugoing@gmail.com>
---
 include/net/nfc/nci_core.h |  1 +
 net/nfc/nci/core.c         |  3 +++
 net/nfc/nci/ntf.c          |  5 +++++
 net/nfc/nci/rsp.c          | 15 ++++++++++-----
 4 files changed, 19 insertions(+), 5 deletions(-)

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..69bc9047f057 100644
--- a/net/nfc/nci/ntf.c
+++ b/net/nfc/nci/ntf.c
@@ -36,6 +36,11 @@ static int nci_core_reset_ntf_packet(struct nci_dev *ndev,
 	if (skb->len < sizeof(struct nci_core_reset_ntf))
 		return -EINVAL;
 
+	if (!test_and_clear_bit(NCI_RESET_PENDING, &ndev->flags)) {
+		pr_warn_ratelimited("unexpected CORE_RESET_NTF\n");
+		return -EINVAL;
+	}
+
 	ntf = (struct nci_core_reset_ntf *)skb->data;
 
 	ndev->nci_ver = ntf->nci_ver;
diff --git a/net/nfc/nci/rsp.c b/net/nfc/nci/rsp.c
index b0ab4f5acbce..a4bfe14372a8 100644
--- a/net/nfc/nci/rsp.c
+++ b/net/nfc/nci/rsp.c
@@ -32,14 +32,19 @@ static void nci_core_reset_rsp_packet(struct nci_dev *ndev,
 
 	pr_debug("status 0x%x\n", rsp->status);
 
+	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);
-		}
+		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 1/1] nfc: nci: ignore unexpected CORE_RESET_NTF

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918013337.82214-2-ndaugoing@gmail.com?part=1
---
nfc: nci: ignore unexpected CORE_RESET_NTF

This adds an NCI_RESET_PENDING flag that is set when CORE_RESET_CMD is sent
and cleared on request completion, so that CORE_RESET_NTF is only handled
while a reset request is outstanding. Unexpected notifications are dropped
with a ratelimited warning.

A few questions about the new gate below.

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

[Severity: Medium]
Since the flag carries no generation or sequence identity, can a
CORE_RESET_NTF belonging to an abandoned reset be matched against a later
reset attempt?

If reset attempt #1 times out here, the flag is cleared, but the
notification may already be sitting in nci_rx_work. The failure path in
nci_open_device() purges the queue but does not drain the worker:

	/* Init failed, cleanup */
	skb_queue_purge(&ndev->cmd_q);
	skb_queue_purge(&ndev->rx_q);
	skb_queue_purge(&ndev->tx_q);

	ndev->ops->close(ndev);

Only nci_close_device() flushes ndev->rx_wq, and it does that after dropping
req_lock; the exported nci_core_reset() path purges nothing.

If userspace then retries NFC_CMD_DEV_UP (or a driver calls
nci_core_reset()), nci_reset_req() sets the same bit again and the stale
frame is accepted by test_and_clear_bit() in nci_core_reset_ntf_packet(),
overwriting ndev->nci_ver / manufact_* from the old frame and completing
reset #2 with NCI_STATUS_OK before its own CORE_RESET_RSP has been seen.
The later real response can then complete the following CORE_INIT request.

> @@ -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 a single bit enough synchronisation between the two contexts here?
nci_reset_req()/__nci_request() run in process context under
ndev->req_lock, while nci_core_reset_ntf_packet() runs from nci_rx_work on
ndev->rx_wq, and the two share no lock.

The check and the use are not atomic. After test_and_clear_bit() succeeds in
nci_core_reset_ntf_packet(), the worker can be preempted before it reaches:

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

If the reset it belonged to times out in that window, __nci_request() zeroes
req_status and returns -ETIMEDOUT, and a new request can be armed
(req_status = NCI_REQ_PEND). nci_req_complete() only looks at the global
status:

	if (ndev->req_status == NCI_REQ_PEND) {
		ndev->req_result = result;
		ndev->req_status = NCI_REQ_DONE;
		complete(&ndev->req_completion);
	}

so the resumed worker completes the new request as successful, and writes
ndev->nci_ver while nci_open_device() may be reading it:

	if (ndev->nci_ver & NCI_VER_2_MASK)
		opt = &nci_init_v2_cmd;

Would tying the notification to a request generation, or serialising the
flag test with request start/teardown, close this?

> diff --git a/net/nfc/nci/ntf.c b/net/nfc/nci/ntf.c
> index f5c9a8ab7ec10..b7fa8980804da 100644
> --- a/net/nfc/nci/ntf.c
> +++ b/net/nfc/nci/ntf.c
> @@ -36,6 +36,11 @@ static int nci_core_reset_ntf_packet(struct nci_dev *ndev,
>  	if (skb->len < sizeof(struct nci_core_reset_ntf))
>  		return -EINVAL;
>  
> +	if (!test_and_clear_bit(NCI_RESET_PENDING, &ndev->flags)) {
> +		pr_warn_ratelimited("unexpected CORE_RESET_NTF\n");
> +		return -EINVAL;
> +	}
> +

[Severity: High]
Does returning -EINVAL here break the fdp firmware update handshake?

In nci_ntf_packet() a non-zero return skips the driver dispatch:

	case NCI_OP_CORE_RESET_NTF:
		if (nci_core_reset_ntf_packet(ndev, skb))
			goto end;
		break;
	...
	nci_core_ntf_packet(ndev, ntf_opcode, skb);
end:
	kfree_skb(skb);

nci_core_ntf_packet() is the only dispatcher for driver core_ops handlers,
and drivers/nfc/fdp registers one for exactly this notification:

	static const struct nci_driver_ops fdp_core_ops[] = {
		...
		{
			.opcode = NCI_OP_CORE_RESET_NTF,
			.ntf = fdp_nci_core_reset_ntf_packet,
		},
	};

That hook is what wakes the patch path:

	static int fdp_nci_core_reset_ntf_packet(struct nci_dev *ndev,
						  struct sk_buff *skb)
	{
		struct fdp_nci_info *info = nci_get_drvdata(ndev);

		info->setup_reset_ntf = 1;
		wake_up(&info->setup_wq);

The notification it waits for is unsolicited, as the comment in
fdp_nci_send_patch() describes:

	 * command with NCI_PATCH_TYPE_EOT parameter. The device will send a
	 * NFCC_PATCH_NTF packet and a NCI_OP_CORE_RESET_NTF packet.

At that point the last request was nci_prop_cmd(NCI_OP_PROP_PATCH_OID),
whose completion also clears NCI_RESET_PENDING in __nci_request(), so the
flag is clear and the notification is dropped. Would
fdp_nci_patch_otp()/fdp_nci_patch_ram() then block forever in

	wait_event_interruptible(info->setup_wq, info->setup_reset_ntf);

with no timeout? That wait runs inside ndev->ops->setup(), which
nci_open_device() calls while holding ndev->req_lock, so a stalled task
would also block nci_close_device()/nci_unregister_device() and keep the
request_firmware() buffers pinned because fdp_nci_release_firmware() is
never reached.

Would it be enough to skip only the core-level bookkeeping (drop the state
update and break) instead of aborting the whole notification pipeline?

[Severity: Medium]
Should the decision use ntf->reset_trigger rather than a host-side flag?

The notification carries the discriminator itself:

	struct nci_core_reset_ntf {
		u8	reset_trigger;
		u8	config_status;
		...

but the accept/reject decision is taken before the packet is even parsed,
and reset_trigger is never read in the handler.

That cuts both ways. A genuine NFCC-initiated reset arriving outside a reset
request is discarded with only a warning, even though the controller has
returned to its initial state (RF discovery stopped, logical connections and
credits destroyed, routing table cleared), and the driver hook does not run
either.

Conversely, a spontaneous notification (reset_trigger not indicating
CORE_RESET_CMD) that happens to land inside the reset window is still
accepted, still overwrites ndev->nci_ver / manufact_id /
manufact_specific_info, and still calls:

	nci_req_complete(ndev, NCI_STATUS_OK);

Is the intended bug class actually eliminated, or only narrowed by timing?

> diff --git a/net/nfc/nci/rsp.c b/net/nfc/nci/rsp.c
> index b0ab4f5acbce8..a4bfe14372a81 100644
> --- a/net/nfc/nci/rsp.c
> +++ b/net/nfc/nci/rsp.c
> @@ -32,14 +32,19 @@ static void nci_core_reset_rsp_packet(struct nci_dev *ndev,
>  
>  	pr_debug("status 0x%x\n", rsp->status);
>  
> +	if (rsp->status != NCI_STATUS_OK) {
> +		clear_bit(NCI_RESET_PENDING, &ndev->flags);
> +		nci_req_complete(ndev, rsp->status);
> +		return;
> +	}
> +

[Severity: Medium]
Should the response handler test NCI_RESET_PENDING too, rather than only
clearing it?

nci_rsp_packet() dispatches purely by opcode with no correlation to the
outstanding command:

	switch (rsp_opcode) {
	case NCI_OP_CORE_RESET_RSP:
		nci_core_reset_rsp_packet(ndev, skb);
		break;

and nci_req_complete() only checks the global status, so a stray or delayed
CORE_RESET_RSP still completes whatever unrelated request is in flight
(CORE_INIT, RF_DISCOVER, CONN_CREATE) - the desynchronisation the commit
message sets out to remove.

This new block also widens that surface. Before the patch a 1-byte (NCI 2.x)
response completed nothing at all; now the status test runs before any length
discrimination, so a single 3-byte control frame (header plus a non-OK
status, accepted by nci_valid_size() since the declared payload is non-zero)
completes an unrelated request with a controller-chosen status.

There is also a mirror case: because the flag is cleared here without being
validated, a stray CORE_RESET_RSP during a genuine NCI 2.x reset disarms the
new guard, and the real CORE_RESET_NTF is then rejected as unexpected, so
the reset stalls until NCI_RESET_TIMEOUT.

>  	/* 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);
> -		}
> +		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);
>  	}
>  }

[Severity: Low]
This is a pre-existing issue and not introduced by this patch, but does
"skb->len != 1" read rsp->config_status past the received payload?

config_status is the third byte:

	struct nci_core_reset_rsp {
		__u8	status;
		__u8	nci_ver;
		__u8	config_status;
	} __packed;

and nci_valid_size() only requires the declared payload to be present and
non-zero:

	if (skb->len < hdr_size ||
	    skb->len < hdr_size + nci_plen(skb->data)) {
		return false;
	}

	if (!nci_plen(skb->data)) {

so a CORE_RESET_RSP with plen == 2 and status == NCI_STATUS_OK reaches this
block and the pr_debug() reads one byte beyond the payload. The patch keeps
the same condition (status == OK and len != 1), so this is unchanged
behaviour; would an explicit length check for the 3-byte form be worth
adding while this function is being touched?

Re: [PATCH 1/1] nfc: nci: ignore unexpected CORE_RESET_NTF

[Yuchao Zhang]

Hi Simon,

Thank you (and the sashiko reviewer) for the thorough review.
The points raised are completely valid and insightful.

1. Regarding FDP firmware update breakage (Severity: High):
You are absolutely right. In nci_ntf_packet(), returning -EINVAL from
nci_core_reset_ntf_packet() aborts the dispatch pipeline (goto end), which
prevents nci_core_ntf_packet() from running driver-specific handlers. In
drivers/nfc/fdp/fdp.c, the firmware patching path relies on this spontaneous
CORE_RESET_NTF to wake up info->setup_wq; dropping the packet causes it to
block indefinitely.

As suggested, in v2 we drop only the core-level bookkeeping (skipping the
ndev->nci_ver update and nci_req_complete) and return 0 so that driver-registered
ops continue to receive the notification.

2. Regarding ntf->reset_trigger (Severity: Medium):
The reset_trigger field is revision-dependent: in NCI 1.0, CORE_RESET_CMD
is reported as 0x00 (0x01 is powered-on), while NCI 2.0 defines 0x00 as
an unrecoverable error and reports CORE_RESET_CMD as 0x02. Hardcoding
either value would break the other generation of devices, so v2 gates
the core bookkeeping on NCI_RESET_PENDING alone and only skips power-on
notifications (trigger 0x01, which means the same in both revisions).
Power-on notifications never overlap a pending reset in the protocol
flow, and both revision's command-triggered values reach the flag gate.

3. Regarding nci_core_reset_rsp_packet() (Severity: Medium):
Agreed. nci_core_reset_rsp_packet() now checks whether NCI_RESET_PENDING
is set before calling nci_req_complete() on non-OK or NCI 1.x responses to avoid
prematurely completing an unrelated request.

4. Regarding NCI 1.x response length check (Severity: Low):
In v2, we explicitly check skb->len >= sizeof(*rsp) to ensure config_status is
not read out-of-bounds in pr_debug().

5. Regarding request serialization / stale notifications (Severity: Medium):
All requests are serialized under ndev->req_lock, and __nci_request() unconditionally
clears NCI_RESET_PENDING before dropping req_lock. Filtering out power-on notifications
(trigger 0x01) ensures spontaneous controller power-ups are ignored, while checking and
clearing the pending flag ensures each command matches at most one notification.

6. Regarding request generation (Severity: Medium on both questions):
The NCI protocol gives notifications no request correlation, so a stale
CORE_RESET_NTF/RSP that is already inside nci_rx_work when a reset times
out and userspace retries cannot be distinguished from the new attempt's
own frame: the flag gate and the trigger check narrow the window, but a
generation identity would require protocol support that does not exist.
The failure path purges queued frames; only an in-flight worker frame can
straddle, and the data it carries originates from the same controller.
If you would prefer, I can add a per-request generation check for the RSP
path where an identity exists, as a follow-up.

v2 patch has been sent as reply to this thread.

Thanks,
Yuchao

[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

[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

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

[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