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

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

[zjamg]

From: Yuchao Zhang <ndaugoing@gmail.com>

Hello,

This patch addresses an issue in the NCI core stack where an unexpected
or unsolicited CORE_RESET_NTF packet can prematurely complete unrelated
in-flight requests with NCI_STATUS_OK and corrupt protocol version state.

Problem Overview:
=================
Commit bcd684aace34 ("net/nfc/nci: Support NCI 2.x initial sequence")
added nci_core_reset_ntf_packet() to handle NCI 2.x CORE_RESET
notifications. When received, it updates ndev->nci_ver and manufacturer
information, and calls nci_req_complete(ndev, NCI_STATUS_OK).

Unlike other notification handlers in ntf.c (which validate ndev->state
before acting), nci_core_reset_ntf_packet() does not verify whether a
core reset request is actually pending.

If an unsolicited or delayed CORE_RESET_NTF is received:
1. If another request is currently in-flight (such as CORE_INIT,
   RF_DISCOVER, or CONN_CREATE), it prematurely completes that request
   with NCI_STATUS_OK, leading to state desynchronization.
2. Even when no request is in-flight, it unconditionally overwrites
   ndev->nci_ver and manufacturer info. Because ndev->nci_ver acts as
   a parser and packet format selector (e.g., in nci_open_device() and
   nci_core_init_rsp_packet()), unexpectedly modifying it can cause
   protocol format confusion.

Solution:
=========
Introduce an NCI_RESET_PENDING flag in enum nci_flag to ensure
CORE_RESET_NTF is only accepted while a reset command is actively
awaiting it.

Testing:
========
Verified with module compilation and checkpatch.pl (0 errors, 0 warnings).
Empirically confirmed that unsolicited CORE_RESET_NTF packets are safely
rejected with a warning while legitimate reset sequences continue to
complete normally.

Thanks,
Yuchao Zhang

Yuchao Zhang (1):
  nfc: nci: ignore unexpected CORE_RESET_NTF

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

-- 
2.53.0

[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?