[PATCH net 0/5] nfc: fix HCI over NCI and NFCC loopback response handling

[PATCH net 0/5] nfc: fix HCI over NCI and NFCC loopback response handling

[Joas Antonio dos Santos]

Patch 1 fixes a regression: since commit 8265a626cc14 ("nfc: nci: fix
double completion race in nci_data_exchange_complete") every data packet
received on an NCI logical connection other than the static RF one is
dropped, because only nci_transceive() sets NCI_DATA_EXCHANGE.  That
breaks HCI over NCI (st-nci) and nci_nfcc_loopback() entirely: every
command times out.  The commit is in mainline and in the 5.10+ stable
trees.

Patches 2 and 3 fix the handling of the stored response in those two
paths.  conn_info->rx_skb is never cleared after the response has been
handed to the caller and freed, and a request can be completed without
a new response (an HCP event or command received on the pipe, or an
unrelated NCI response).  The next command then reads, modifies and
returns the freed skb, or dereferences NULL.

Patches 4 and 5 bound the controller supplied pipe ids used as an index
into the pipes[] array in the st21nfca and st-nci load_session
handlers, as pointed out by sashiko.

Testing:

- Patches 1-3 were tested on arm64 QEMU with KASAN, using virtual_ncidev
  and a userspace NFCC emulator, plus a test-only ioctl in
  virtual_ncidev (not part of this series) that drives
  nci_core_conn_create(), nci_hci_dev_session_init(),
  nci_hci_get_param() and nci_nfcc_loopback() the way st-nci does.
  - Without patch 1, the response to the first ANY_OPEN_PIPE is dropped
    and nci_hci_dev_session_init() returns -ETIMEDOUT; with it, session
    init and HCP commands complete.
  - Without patches 2/3: slab-use-after-free in nci_hci_get_param()
    (HCP event instead of a response), null-ptr-deref in
    nci_hci_get_param() (no response stored yet), and a
    slab-use-after-free on the skb returned by the second
    nci_nfcc_loopback().  With them all three return -EPROTO, normal
    responses are still returned, and KASAN is quiet.
  - tools/testing/selftests/nci passes (8/8) with the series applied.
- Patches 4 and 5 are compile tested only (W=1); I have no ST hardware.

Sanghyun Park's pending "nfc: nci: Fix conn_info use-after-free" v5
touches the same functions in hci.c (it adds a NULL check for rx_skb,
but does not clear it); whichever goes in second will need a rebase.


Joas Antonio dos Santos (5):
  nfc: nci: don't drop data received on HCI and loopback connections
  nfc: nci: hci: consume the HCP response exactly once
  nfc: nci: consume the loopback response exactly once
  nfc: st21nfca: validate pipe ids in st21nfca_hci_load_session()
  nfc: st-nci: validate pipe ids in st_nci_hci_load_session()

 drivers/nfc/st-nci/se.c     |  8 ++++
 drivers/nfc/st21nfca/core.c |  8 ++++
 net/nfc/nci/core.c          | 18 ++++++--
 net/nfc/nci/data.c          |  9 +++-
 net/nfc/nci/hci.c           | 83 ++++++++++++++++++++++---------------
 5 files changed, 87 insertions(+), 39 deletions(-)

-- 
2.43.0

[PATCH net 1/5] nfc: nci: don't drop data received on HCI and loopback connections

[Joas Antonio dos Santos]

Commit 8265a626cc14 ("nfc: nci: fix double completion race in
nci_data_exchange_complete") made nci_data_exchange_complete() drop the
received skb unless it can clear NCI_DATA_EXCHANGE.  That bit is only
set by nci_transceive(), for data exchanges with a remote target on the
static RF connection.

The other users of NCI logical connections do not set it: HCI over NCI
(nci_hci_send_cmd() and friends, used by st-nci) and
nci_nfcc_loopback() send with nci_send_data() and rely on their
data_exchange_cb being called for every data packet received on their
connection.  Since that commit every such packet is silently freed, so
each HCI command and each loopback request times out after
NCI_DATA_TIMEOUT.  On st-nci, secure element discovery fails when the
device is brought up ("SE discovery failed") and the HCI and loopback
based vendor commands no longer work.

Seen with virtual_ncidev and an emulated NFCC: the response to the
first ANY_OPEN_PIPE command is dropped and nci_hci_dev_session_init()
returns -ETIMEDOUT.

Only claim NCI_DATA_EXCHANGE for data on the static RF connection, as
before that commit data on the other connections is always handed to
their callback.

Fixes: 8265a626cc14 ("nfc: nci: fix double completion race in nci_data_exchange_complete")
Cc: stable@vger.kernel.org
Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
Assisted-by: Yaga Agent HackerSec Claude:claude-opus-5-5
---
 net/nfc/nci/data.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/nfc/nci/data.c b/net/nfc/nci/data.c
index 4253edea5..76ec891a0 100644
--- a/net/nfc/nci/data.c
+++ b/net/nfc/nci/data.c
@@ -46,8 +46,13 @@ void nci_data_exchange_complete(struct nci_dev *ndev, struct sk_buff *skb,
 	timer_delete_sync(&ndev->data_timer);
 	clear_bit(NCI_DATA_EXCHANGE_TO, &ndev->flags);
 
-	/* Claim completion atomically -- both close and rx_work may race here */
-	if (!test_and_clear_bit(NCI_DATA_EXCHANGE, &ndev->flags)) {
+	/* Claim completion atomically -- both close and rx_work may race here.
+	 * NCI_DATA_EXCHANGE only tracks nci_transceive() on the static RF
+	 * connection; data received on other logical connections (HCI over
+	 * NCI, NFCC loopback) is always handed to their callback.
+	 */
+	if (conn_id == NCI_STATIC_RF_CONN_ID &&
+	    !test_and_clear_bit(NCI_DATA_EXCHANGE, &ndev->flags)) {
 		kfree_skb(skb);
 		return;
 	}
-- 
2.43.0

[PATCH net 2/5] nfc: nci: hci: consume the HCP response exactly once

[Joas Antonio dos Santos]

nci_hci_resp_received() stores the HCP response in conn_info->rx_skb
and completes the pending request.  nci_hci_send_cmd(),
nci_hci_set_param() and nci_hci_get_param() then parse conn_info->rx_skb
and hand it to the caller, which frees it, but the pointer is never
cleared.  nci_hci_open_pipe() does not look at the response at all, so
that skb is left in conn_info.

A request is not only completed by a response, though:
nci_hci_hcp_message_rx() also calls nci_req_complete(ndev,
NCI_STATUS_OK) for an HCP command or event received on the connection,
and so does any NCI response with a zero status.  When that happens
while a command is pending, the code above reads, skb_pull()s and
returns whatever conn_info->rx_skb points to: the skb of the previous
response, which the previous caller already freed, or NULL if no
response was stored yet.  The skb is then freed again by the new
caller, e.g. st_nci_hci_load_session() frees every skb it gets from
nci_hci_send_cmd().

With an emulated NFCC on virtual_ncidev answering an
ANY_GET_PARAMETER command with an HCP event instead of a response:

  BUG: KASAN: slab-use-after-free in nci_hci_get_param+0x314/0x34c
  Read of size 8 at addr fff00000c0b31248 by task nfcc_emu/109
  ...
  Freed by task 109:
   kfree_skbmem+0x140/0x1a4
   sk_skb_reason_drop+0xc4/0x398
   ...
  The buggy address belongs to the object at fff00000c0b31180
   which belongs to the cache skbuff_head_cache of size 224

(the free being the previous caller's kfree_skb() of the response it
got), and when no response was stored yet:

  KASAN: null-ptr-deref in range [0x00000000000000c8-0x00000000000000cf]
  pc : nci_hci_get_param+0x1a0/0x34c

Add a helper that detaches the stored response from conn_info, parses
it, and either hands it to the caller or frees it, and return -EPROTO
when the request was completed without a response.  Use it for
nci_hci_open_pipe() as well, like nfc_hci_open_pipe() in the HCI core
does, so that its response is not left behind.

Fixes: 11f54f228643 ("NFC: nci: Add HCI over NCI protocol support")
Cc: stable@vger.kernel.org
Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
Assisted-by: Yaga Agent HackerSec Claude:claude-opus-5-5
---
 net/nfc/nci/hci.c | 83 ++++++++++++++++++++++++++++-------------------
 1 file changed, 49 insertions(+), 34 deletions(-)

diff --git a/net/nfc/nci/hci.c b/net/nfc/nci/hci.c
index c03e8a0bd..bdfe9eee6 100644
--- a/net/nfc/nci/hci.c
+++ b/net/nfc/nci/hci.c
@@ -107,6 +107,38 @@ static int nci_hci_result_to_errno(u8 result)
 	}
 }
 
+/* Take the HCP response stored by nci_hci_resp_received().
+ *
+ * The response skb is detached from conn_info so that it is consumed
+ * exactly once.  A request can also be completed without a response
+ * being stored (an HCP command or event received while the request is
+ * pending, or an unrelated NCI response), so a missing response is an
+ * error rather than a reason to reuse whatever skb was stored last.
+ */
+static int nci_hci_take_resp(struct nci_conn_info *conn_info,
+			     struct sk_buff **skb)
+{
+	const struct nci_hcp_message *message;
+	struct sk_buff *rx_skb;
+	int r;
+
+	rx_skb = conn_info->rx_skb;
+	conn_info->rx_skb = NULL;
+	if (!rx_skb)
+		return -EPROTO;
+
+	message = (struct nci_hcp_message *)rx_skb->data;
+	r = nci_hci_result_to_errno(NCI_HCP_MSG_GET_CMD(message->header));
+	skb_pull(rx_skb, NCI_HCI_HCP_MESSAGE_HEADER_LEN);
+
+	if (!r && skb)
+		*skb = rx_skb;
+	else
+		kfree_skb(rx_skb);
+
+	return r;
+}
+
 /* HCI core */
 static void nci_hci_reset_pipes(struct nci_hci_dev *hdev)
 {
@@ -222,8 +254,7 @@ int nci_hci_send_cmd(struct nci_dev *ndev, u8 gate, u8 cmd,
 		     const u8 *param, size_t param_len,
 		     struct sk_buff **skb)
 {
-	const struct nci_hcp_message *message;
-	const struct nci_conn_info *conn_info;
+	struct nci_conn_info *conn_info;
 	struct nci_data data;
 	int r;
 	u8 pipe = ndev->hci_dev->gate2pipe[gate];
@@ -243,15 +274,8 @@ int nci_hci_send_cmd(struct nci_dev *ndev, u8 gate, u8 cmd,
 
 	r = nci_request(ndev, nci_hci_send_data_req, &data,
 			msecs_to_jiffies(NCI_DATA_TIMEOUT));
-	if (r == NCI_STATUS_OK) {
-		message = (struct nci_hcp_message *)conn_info->rx_skb->data;
-		r = nci_hci_result_to_errno(
-			NCI_HCP_MSG_GET_CMD(message->header));
-		skb_pull(conn_info->rx_skb, NCI_HCI_HCP_MESSAGE_HEADER_LEN);
-
-		if (!r && skb)
-			*skb = conn_info->rx_skb;
-	}
+	if (r == NCI_STATUS_OK)
+		r = nci_hci_take_resp(conn_info, skb);
 
 	return r;
 }
@@ -508,7 +532,8 @@ void nci_hci_data_received_cb(void *context,
 int nci_hci_open_pipe(struct nci_dev *ndev, u8 pipe)
 {
 	struct nci_data data;
-	const struct nci_conn_info *conn_info;
+	struct nci_conn_info *conn_info;
+	int r;
 
 	conn_info = ndev->hci_dev->conn_info;
 	if (!conn_info)
@@ -521,8 +546,12 @@ int nci_hci_open_pipe(struct nci_dev *ndev, u8 pipe)
 	data.data = NULL;
 	data.data_len = 0;
 
-	return nci_request(ndev, nci_hci_send_data_req, &data,
-			   msecs_to_jiffies(NCI_DATA_TIMEOUT));
+	r = nci_request(ndev, nci_hci_send_data_req, &data,
+			msecs_to_jiffies(NCI_DATA_TIMEOUT));
+	if (r == NCI_STATUS_OK)
+		r = nci_hci_take_resp(conn_info, NULL);
+
+	return r;
 }
 EXPORT_SYMBOL(nci_hci_open_pipe);
 
@@ -566,8 +595,7 @@ static int nci_hci_delete_pipe(struct nci_dev *ndev, u8 pipe)
 int nci_hci_set_param(struct nci_dev *ndev, u8 gate, u8 idx,
 		      const u8 *param, size_t param_len)
 {
-	const struct nci_hcp_message *message;
-	const struct nci_conn_info *conn_info;
+	struct nci_conn_info *conn_info;
 	struct nci_data data;
 	int r;
 	u8 *tmp;
@@ -598,12 +626,8 @@ int nci_hci_set_param(struct nci_dev *ndev, u8 gate, u8 idx,
 
 	r = nci_request(ndev, nci_hci_send_data_req, &data,
 			msecs_to_jiffies(NCI_DATA_TIMEOUT));
-	if (r == NCI_STATUS_OK) {
-		message = (struct nci_hcp_message *)conn_info->rx_skb->data;
-		r = nci_hci_result_to_errno(
-			NCI_HCP_MSG_GET_CMD(message->header));
-		skb_pull(conn_info->rx_skb, NCI_HCI_HCP_MESSAGE_HEADER_LEN);
-	}
+	if (r == NCI_STATUS_OK)
+		r = nci_hci_take_resp(conn_info, NULL);
 
 	kfree(tmp);
 	return r;
@@ -613,8 +637,7 @@ EXPORT_SYMBOL(nci_hci_set_param);
 int nci_hci_get_param(struct nci_dev *ndev, u8 gate, u8 idx,
 		      struct sk_buff **skb)
 {
-	const struct nci_hcp_message *message;
-	const struct nci_conn_info *conn_info;
+	struct nci_conn_info *conn_info;
 	struct nci_data data;
 	int r;
 	u8 pipe = ndev->hci_dev->gate2pipe[gate];
@@ -637,16 +660,8 @@ int nci_hci_get_param(struct nci_dev *ndev, u8 gate, u8 idx,
 
 	r = nci_request(ndev, nci_hci_send_data_req, &data,
 			msecs_to_jiffies(NCI_DATA_TIMEOUT));
-
-	if (r == NCI_STATUS_OK) {
-		message = (struct nci_hcp_message *)conn_info->rx_skb->data;
-		r = nci_hci_result_to_errno(
-			NCI_HCP_MSG_GET_CMD(message->header));
-		skb_pull(conn_info->rx_skb, NCI_HCI_HCP_MESSAGE_HEADER_LEN);
-
-		if (!r && skb)
-			*skb = conn_info->rx_skb;
-	}
+	if (r == NCI_STATUS_OK)
+		r = nci_hci_take_resp(conn_info, skb);
 
 	return r;
 }
-- 
2.43.0

[PATCH net 3/5] nfc: nci: consume the loopback response exactly once

[Joas Antonio dos Santos]

nci_nfcc_loopback_cb() stores the received data packet in
conn_info->rx_skb and completes the request, and nci_nfcc_loopback()
returns conn_info->rx_skb to the caller, which frees it.  The pointer
is never cleared.

Any NCI response with a zero status also completes the request, as
nci_req_complete() only checks NCI_REQ_PEND.  If the NFCC sends one
instead of the data packet, nci_nfcc_loopback() returns the skb of the
previous loopback, which was already freed, or NULL.  st_nci_loopback()
then copies skb->data into its vendor command reply and frees the skb
again.

With an emulated NFCC on virtual_ncidev answering the second loopback
with an unsolicited CORE_SET_CONFIG_RSP, nci_nfcc_loopback() returns 0
and the skb of the first loopback, and KASAN reports a
slab-use-after-free as soon as the caller reads it.

Detach the skb from conn_info when consuming it, free it when the
caller does not want it, and return -EPROTO when the request was
completed without a data packet.

Fixes: 1c53855f6be2 ("nfc: nci: Add nci_nfcc_loopback to the nci core")
Cc: stable@vger.kernel.org
Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
Assisted-by: Yaga Agent HackerSec Claude:claude-opus-5-5
---
 net/nfc/nci/core.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
index 73e3a9647..8e2a755ce 100644
--- a/net/nfc/nci/core.c
+++ b/net/nfc/nci/core.c
@@ -428,7 +428,7 @@ int nci_nfcc_loopback(struct nci_dev *ndev, const void *data, size_t data_len,
 	int r;
 	struct nci_loopback_data loopback_data;
 	struct nci_conn_info *conn_info;
-	struct sk_buff *skb;
+	struct sk_buff *skb, *rx_skb;
 	int conn_id = nci_get_conn_info_by_dest_type_params(ndev,
 					NCI_DESTINATION_NFCC_LOOPBACK, NULL);
 
@@ -464,8 +464,20 @@ int nci_nfcc_loopback(struct nci_dev *ndev, const void *data, size_t data_len,
 	ndev->cur_conn_id = conn_id;
 	r = nci_request(ndev, nci_send_data_req, &loopback_data,
 			msecs_to_jiffies(NCI_DATA_TIMEOUT));
-	if (r == NCI_STATUS_OK && resp)
-		*resp = conn_info->rx_skb;
+	if (r == NCI_STATUS_OK) {
+		/* Consume the stored data packet exactly once, see
+		 * nci_nfcc_loopback_cb().
+		 */
+		rx_skb = conn_info->rx_skb;
+		conn_info->rx_skb = NULL;
+		if (!rx_skb)
+			return -EPROTO;
+
+		if (resp)
+			*resp = rx_skb;
+		else
+			kfree_skb(rx_skb);
+	}
 
 	return r;
 }
-- 
2.43.0

[PATCH net 4/5] nfc: st21nfca: validate pipe ids in st21nfca_hci_load_session()

[Joas Antonio dos Santos]

st21nfca_hci_load_session() gets the list of open pipes from the
controller and, for each pipe id in it, asks for the pipe information
and records it in hdev->pipes[].  The pipe id is a byte from the
controller and is used as an index into hdev->pipes[], which has
NFC_HCI_MAX_PIPES (128) entries, without any check.  For a pipe id of
128 or more that the controller reports as open on one of the known
gates, the gate id and the controller supplied host id are written past
the end of the array, over the rest of struct nfc_hci_dev (including
the async_cb function pointer) and, for larger ids, past the end of the
object.

The pipe information response is not length checked either, so a
short response is read past its end.

Skip pipe ids that do not fit in hdev->pipes[] and responses that are
shorter than struct st21nfca_pipe_info.

This was pointed out as a pre-existing issue by the sashiko review of an
nfc_hci_create_pipe() fix.

Link: https://lore.kernel.org/all/178952042575.22033.9676833200303972344@kernel.org/
Fixes: 2130fb97fecf ("NFC: st21nfca: Adding support for secure element")
Cc: stable@vger.kernel.org
Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
Assisted-by: Yaga Agent HackerSec Claude:claude-opus-5-5
---
 drivers/nfc/st21nfca/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/nfc/st21nfca/core.c b/drivers/nfc/st21nfca/core.c
index b5c1ca3ac..88856c26c 100644
--- a/drivers/nfc/st21nfca/core.c
+++ b/drivers/nfc/st21nfca/core.c
@@ -147,12 +147,20 @@ static int st21nfca_hci_load_session(struct nfc_hci_dev *hdev)
 	/* Complete the existing gate_pipe table */
 	for (i = 0; i < skb_pipe_list->len; i++) {
 		pipe_info[2] = skb_pipe_list->data[i];
+		if (pipe_info[2] >= NFC_HCI_MAX_PIPES)
+			continue;
+
 		r = nfc_hci_send_cmd(hdev, ST21NFCA_DEVICE_MGNT_GATE,
 					ST21NFCA_DM_GETINFO, pipe_info,
 					sizeof(pipe_info), &skb_pipe_info);
 		if (r)
 			continue;
 
+		if (skb_pipe_info->len < sizeof(*info)) {
+			kfree_skb(skb_pipe_info);
+			continue;
+		}
+
 		/*
 		 * Match pipe ID and gate ID
 		 * Output format from ST21NFC_DM_GETINFO is:
-- 
2.43.0

[PATCH net 5/5] nfc: st-nci: validate pipe ids in st_nci_hci_load_session()

[Joas Antonio dos Santos]

st_nci_hci_load_session() gets the list of open pipes from the
controller and, for each pipe id in it, asks for the pipe information
and records it in ndev->hci_dev->pipes[].  The pipe id is a byte from
the controller and is used as an index into pipes[], which has
NCI_HCI_MAX_PIPES (128) entries, without any check.  For a pipe id of
128 or more that the controller reports as open on one of the known
gates, the gate id and the controller supplied host id are written past
the end of the array, into gate2pipe[].

The pipe information response is not length checked either, so a
short response is read past its end.

Skip pipe ids that do not fit in pipes[] and responses that are
shorter than struct st_nci_pipe_info.

This was pointed out as a pre-existing issue by the sashiko review of an
nfc_hci_create_pipe() fix.

Link: https://lore.kernel.org/all/178952042575.22033.9676833200303972344@kernel.org/
Fixes: 8ae01f796771 ("NFC: st21nfcb: Add support for secure element")
Cc: stable@vger.kernel.org
Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
Assisted-by: Yaga Agent HackerSec Claude:claude-opus-5-5
---
 drivers/nfc/st-nci/se.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/nfc/st-nci/se.c b/drivers/nfc/st-nci/se.c
index 607ec768e..909fb1493 100644
--- a/drivers/nfc/st-nci/se.c
+++ b/drivers/nfc/st-nci/se.c
@@ -191,6 +191,9 @@ int st_nci_hci_load_session(struct nci_dev *ndev)
 	/* Complete the existing gate_pipe table */
 	for (i = 0; i < skb_pipe_list->len; i++) {
 		pipe_info[2] = skb_pipe_list->data[i];
+		if (pipe_info[2] >= NCI_HCI_MAX_PIPES)
+			continue;
+
 		r = nci_hci_send_cmd(ndev, ST_NCI_DEVICE_MGNT_GATE,
 					ST_NCI_DM_GETINFO, pipe_info,
 					sizeof(pipe_info), &skb_pipe_info);
@@ -198,6 +201,11 @@ int st_nci_hci_load_session(struct nci_dev *ndev)
 		if (r)
 			continue;
 
+		if (skb_pipe_info->len < sizeof(*dm_pipe_info)) {
+			kfree_skb(skb_pipe_info);
+			continue;
+		}
+
 		/*
 		 * Match pipe ID and gate ID
 		 * Output format from ST21NFC_DM_GETINFO is:
-- 
2.43.0

Re: [PATCH net 1/5] nfc: nci: don't drop data received on HCI and loopback connections

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (2) · Medium: 2 (3) · 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).

- [Medium] Three completion paths pass ndev->cur_conn_id where they
  should pass the RF connection id, and after this patch that can leave…
- [Medium] After this patch, completions on non-RF connections still tear
  down the RF exchange's timeout state but no longer release…

Pre-existing issues:
- [High] HCI work can use the device after it is freed during teardown.
- [High] At the target commit, this patch lets NFCC data on the HCI
  connection reach nci_hci_data_received_cb() and nci_hci_msg_rx_work()…
- [Medium] nci_nfcc_loopback_cb() (net/nfc/nci/core.c:409-423) ignores
  err, stores the skb (possibly NULL) in conn_info->rx_skb, and…
- [Medium] This patch makes the conn_info->rx_skb lifecycle reachable
  again.
- [Medium] For an HCP RESPONSE, nci_hci_hcp_message_rx() completes the
  request twice.

--

nfc: nci: don't drop data received on HCI and loopback connections

This patch limits the NCI_DATA_EXCHANGE claim in
nci_data_exchange_complete() to data on the static RF connection. Data
on HCI over NCI and NFCC loopback connections goes to their
data_exchange_cb again, instead of being freed. That undoes the
regression from 8265a626cc14 where every HCI and loopback request timed
out.

> diff --git a/net/nfc/nci/data.c b/net/nfc/nci/data.c
> index 4253edea5c8db..76ec891a02b09 100644
> --- a/net/nfc/nci/data.c
> +++ b/net/nfc/nci/data.c
> @@ -46,8 +46,13 @@ void nci_data_exchange_complete(struct nci_dev *ndev, struct sk_buff *skb,
>  	timer_delete_sync(&ndev->data_timer);
>  	clear_bit(NCI_DATA_EXCHANGE_TO, &ndev->flags);
>  
> -	/* Claim completion atomically -- both close and rx_work may race here */
> -	if (!test_and_clear_bit(NCI_DATA_EXCHANGE, &ndev->flags)) {
> +	/* Claim completion atomically -- both close and rx_work may race here.
> +	 * NCI_DATA_EXCHANGE only tracks nci_transceive() on the static RF
> +	 * connection; data received on other logical connections (HCI over
> +	 * NCI, NFCC loopback) is always handed to their callback.
> +	 */
> +	if (conn_id == NCI_STATIC_RF_CONN_ID &&
> +	    !test_and_clear_bit(NCI_DATA_EXCHANGE, &ndev->flags)) {

[Severity: Medium]
Can this leave NCI_DATA_EXCHANGE set with nothing left to clear it?

The callers gated on NCI_DATA_EXCHANGE pass ndev->cur_conn_id, not the
RF connection id. In nci_rx_work():

	if (test_bit(NCI_DATA_EXCHANGE_TO, &ndev->flags)) {
		/* complete the data exchange transaction, if exists */
		if (test_bit(NCI_DATA_EXCHANGE, &ndev->flags))
			nci_data_exchange_complete(ndev, NULL,
						   ndev->cur_conn_id,
						   -ETIMEDOUT);

Both completions in nci_close_device() do the same thing with -ENODEV.

Only nci_transceive() sets NCI_DATA_EXCHANGE, so a set bit means an RF
exchange on conn 0 is pending. But every nci_send_data() overwrites
cur_conn_id without a lock. That includes HCI sends through
nci_hci_send_data() and nci_nfcc_loopback(), and nci_transceive()
doesn't take req_lock.

Say an RF exchange from rawsock or LLCP is pending. An HCI or loopback
send then sets cur_conn_id to a non-RF connection and re-arms data_timer
in nci_tx_work(). If the timer fires:

nci_rx_work()
  nci_data_exchange_complete(ndev, NULL, ndev->cur_conn_id, -ETIMEDOUT)
    timer_delete_sync(&ndev->data_timer);
    clear_bit(NCI_DATA_EXCHANGE_TO, &ndev->flags);
    conn_id != NCI_STATIC_RF_CONN_ID, so test_and_clear_bit() is skipped
    HCI or loopback cb(cb_context, NULL, -ETIMEDOUT)

The RF callback never runs. NCI_DATA_EXCHANGE stays set and no timer is
left, so every later nci_transceive() returns -EBUSY. Only an
RF_DEACTIVATE_NTF or a close with NCI_UP set clears it. Before this
patch, test_and_clear_bit() released the bit whatever the conn_id was.

The same mismatch lets a close-time completion and an rx_work delivery
on the same non-RF connection both reach the callback. The atomic claim
from 8265a626cc14 no longer covers them, so nci_nfcc_loopback_cb() could
overwrite a just-stored rx_skb with NULL.

One part of this is older than this patch. On close the RF callback
never runs, which leaks the rawsock sock_hold(), but the old code also
called the cur_conn_id callback.

Should the paths gated on NCI_DATA_EXCHANGE pass NCI_STATIC_RF_CONN_ID
instead, as nci_rf_deactivate_ntf_packet() already does?

[Severity: Medium]
With the claim now limited to conn 0, a completion on a non-RF
connection still runs this on the device-wide data_timer:

	timer_delete_sync(&ndev->data_timer);
	clear_bit(NCI_DATA_EXCHANGE_TO, &ndev->flags);

but it no longer releases NCI_DATA_EXCHANGE. Does this cancel the only
timeout of a pending RF nci_transceive()?

nci_tx_work() re-arms data_timer for frames on any connection. While an
RF exchange is pending, any HCI or loopback data on its own connection
deletes the RF exchange's timer. That includes unsolicited HCP events
from the NFCC or SE.

If data_timer has already fired and rx_work handles such a packet in
its dequeue loop, NCI_DATA_EXCHANGE_TO is cleared before the check at
the end of nci_rx_work(). The timeout is lost.

nci_core_conn_intf_error_ntf_packet() has the same shape:

	if (test_bit(NCI_DATA_EXCHANGE, &ndev->flags))
		nci_data_exchange_complete(ndev, NULL, ntf->conn_id, -EIO);

If the NFCC sends a non-RF ntf->conn_id, this deletes the RF timer and
leaves the bit set.

If the RF response is then lost, NCI_DATA_EXCHANGE stays set. Every
later nci_transceive() returns -EBUSY until an RF deactivation. Before
this patch, the non-RF completion's test_and_clear_bit() released the
bit.

The new comment says NCI_DATA_EXCHANGE only tracks the RF connection,
but the timer and TO state are still shared. Should the timer and TO
handling also be limited to conn_id == NCI_STATIC_RF_CONN_ID, or be
made per connection?

>  		kfree_skb(skb);
>  		return;
>  	}

[Severity: Medium]
This isn't a bug introduced by this patch, but now that non-RF data is
always forwarded again, can nci_nfcc_loopback_cb() report success with
a NULL skb?

	conn_info->rx_skb = skb;

	nci_req_complete(ndev, NCI_STATUS_OK);

The callback ignores err. A NULL skb can come from the rx_work timeout,
from CORE_INTERFACE_ERROR_NTF, or from the skb_cow_head() failure path
in nci_add_rx_data_frag() (err = -ENOMEM). In each case the request
completes with OK, and nci_nfcc_loopback() returns 0 with
*resp == NULL. st_nci_loopback() then dereferences it:

	r = nci_nfcc_loopback(ndev, data, data_len, &skb);
	if (r < 0)
		return r;

	msg = nfc_vendor_cmd_alloc_reply_skb(dev, ST_NCI_VENDOR_OUI,
					     LOOPBACK, skb->len);

With this patch, the -ENOMEM case is reachable without an RF exchange.

A later patch in this series, "nfc: nci: consume the loopback response
exactly once", fixes the NULL dereference: nci_nfcc_loopback() detaches
rx_skb and returns -EPROTO when it is NULL. One minor gap remains after
that: a non-NULL skb that arrives together with an error is still
reported as success.

[Severity: Medium]
This is a pre-existing issue, but this patch makes the
conn_info->rx_skb lifecycle reachable again. At this commit rx_skb is
never consumed in these cases, so those skbs leak:

  nci_hci_set_param()
  nci_hci_send_cmd(), when skb is NULL or the HCI result is non-zero
  nci_hci_open_pipe()

Also, any skb that is handed out leaves conn_info->rx_skb dangling.

Later patches in this series fix most of that. "nfc: nci: hci: consume
the HCP response exactly once" adds nci_hci_take_resp(), and "nfc: nci:
consume the loopback response exactly once" does the same for loopback.

One gap remains after those. nci_hci_resp_received() and
nci_nfcc_loopback_cb() both still do:

	conn_info->rx_skb = skb;

without freeing an unconsumed previous skb. Does each late reply after
nci_request() has timed out, or each extra unsolicited HCP RESPONSE from
the NFCC, leak one skb? Nothing frees rx_skb when the connection or
device is torn down either.

[Severity: High]
This isn't a bug introduced by this patch, but HCI delivery was
effectively unreachable after 8265a626cc14, and this patch turns it
back on. Is msg_rx_work ever synchronized with device teardown?

nci_hci_data_received_cb() queues non-RESPONSE messages on the system
workqueue:

		skb_queue_tail(&ndev->hci_dev->msg_rx_queue, hcp_skb);
		schedule_work(&ndev->hci_dev->msg_rx_work);

nci_unregister_device() only flushes and destroys the NCI cmd, rx and
tx workqueues. nci_free_device() calls nci_hci_deallocate():

void nci_hci_deallocate(struct nci_dev *ndev)
{
	kfree(ndev->hci_dev);
}

and then frees ndev. Nothing in net/nfc/nci calls cancel_work_sync() or
flush_work() on msg_rx_work. msg_rx_queue and rx_hcp_frags are never
purged.

In the NCI_UP branch, nci_close_device() keeps the transport open
during the reset request. HCI packets received then still go through
rx_work and schedule msg_rx_work. That work can sleep: for example,
st_nci_hci_connectivity_event_received() calls
devm_kzalloc(GFP_KERNEL) and nfc_se_transaction().

Suppose st_nci_remove()->nci_unregister_device()->nci_free_device()
runs meanwhile. Can nci_hci_msg_rx_work() then touch the freed hdev and
ndev, or call nci_hci_send_data()->queue_work() on the destroyed tx_wq?
Any queued fragments and messages would also leak.

[Severity: Medium]
This isn't a bug introduced by this patch, but now that HCI responses
reach their callback again, each HCP RESPONSE completes the request
twice:

nci_hci_hcp_message_rx()
  nci_hci_resp_received()
    nci_req_complete(ndev, NCI_STATUS_OK);
  nci_req_complete(ndev, NCI_STATUS_OK);

nci_req_complete() only checks req_status == NCI_REQ_PEND. Suppose
rx_work is preempted after the first complete(). The waiter can return,
drop req_lock and start the next nci_request(), which sets
NCI_REQ_PEND. Could the second call then complete that new request
early with OK?

req_lock serializes submitters but doesn't exclude the receive worker.
In the final series state, the next HCI request would get -EPROTO from
nci_hci_take_resp(). A plain NCI command would instead return success
before its RSP arrives, and its real response would later be stored and
never consumed.

At this commit, the race can also cause a use-after-free of a stale
rx_skb. "nfc: nci: hci: consume the HCP response exactly once" fixes
that part.

[Severity: High]
This is a pre-existing issue, but at this commit NFCC data on the HCI
connection reaches nci_hci_data_received_cb() and nci_hci_msg_rx_work()
again. nci_hci_hcp_message_rx() ends with:

	nci_req_complete(ndev, NCI_STATUS_OK);

It does this for COMMAND, EVENT and unknown types as well, and
conn_info->rx_skb is never reset to NULL.

Suppose an unsolicited HCP EVENT or COMMAND arrives while one of these
is waiting:

  nci_hci_send_cmd()
  nci_hci_set_param()
  nci_hci_get_param()
  nci_nfcc_loopback()

The request then completes with OK. Can the waiter skb_pull() a NULL or
already-freed conn_info->rx_skb and hand it out again? That would lead
to a double free in callers such as st_nci_hci_load_session() and
st_nci_loopback().

Later patches in this series fix this. "nfc: nci: hci: consume the HCP
response exactly once" adds nci_hci_take_resp(), which detaches rx_skb
and returns -EPROTO when it is NULL. "nfc: nci: consume the loopback
response exactly once" does the same for nci_nfcc_loopback().

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

Re: [PATCH net 2/5] nfc: nci: hci: consume the HCP response exactly once

[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] nci_hci_take_resp() is supposed to return -EPROTO "when the
  request was completed without a response" and not "reuse whatever skb…
- [Low] nci_hci_take_resp() detaches the response with a plain load
  followed by a plain store (`rx_skb = conn_info->rx_skb;…

--

nfc: nci: hci: consume the HCP response exactly once

This patch adds nci_hci_take_resp(). The helper detaches the HCP response
stored in conn_info->rx_skb, parses it, and either hands it to the caller
or frees it. It returns -EPROTO when a request completes without a stored
response. nci_hci_send_cmd(), nci_hci_set_param(), nci_hci_get_param() and
now nci_hci_open_pipe() all use it, which closes the use-after-free and
NULL dereference shown in the commit message.

> diff --git a/net/nfc/nci/hci.c b/net/nfc/nci/hci.c
> index c03e8a0bd3bd6..bdfe9eee6358c 100644
> --- a/net/nfc/nci/hci.c
> +++ b/net/nfc/nci/hci.c
> @@ -107,6 +107,38 @@ static int nci_hci_result_to_errno(u8 result)
>  	}
>  }
>  
> +/* Take the HCP response stored by nci_hci_resp_received().
> + *
> + * The response skb is detached from conn_info so that it is consumed
> + * exactly once.  A request can also be completed without a response
> + * being stored (an HCP command or event received while the request is
> + * pending, or an unrelated NCI response), so a missing response is an
> + * error rather than a reason to reuse whatever skb was stored last.
> + */
> +static int nci_hci_take_resp(struct nci_conn_info *conn_info,
> +			     struct sk_buff **skb)
> +{
> +	const struct nci_hcp_message *message;
> +	struct sk_buff *rx_skb;
> +	int r;
> +
> +	rx_skb = conn_info->rx_skb;
> +	conn_info->rx_skb = NULL;

[Severity: Low]
Can this read-and-clear race with nci_hci_resp_received()? Every caller
runs nci_hci_take_resp() after nci_request() has dropped ndev->req_lock.
The producer runs from rx_wq and shares no lock with this code.

Suppose an HCI command is completed early by an HCP event or an OK-status
NCI notification, and its real response comes in during this window:

caller                                rx_wq
nci_hci_take_resp()
  rx_skb = conn_info->rx_skb; /* NULL */
                                      nci_hci_resp_received()
                                        conn_info->rx_skb = skb;
  conn_info->rx_skb = NULL;

The skb stored by rx_wq would be overwritten and leaked, and the call
would return -EPROTO. These are also plain, unannotated concurrent
accesses that KCSAN would report.

Two consumers detaching the same skb doesn't look reachable in-tree. The
nfc generic netlink family doesn't set parallel_ops, so genl_mutex
serializes the callers.

Would xchg(), or detaching under a lock shared with the producer, avoid
the lost store?

> +	if (!rx_skb)
> +		return -EPROTO;

[Severity: Medium]
Can a request still get an earlier request's response here? The comment
above says a missing response is an error rather than a reason to reuse
whatever skb was stored last. However, the only check is for NULL.

Several paths call nci_req_complete(ndev, NCI_STATUS_OK) whenever any
request is pending:

  nci_hci_hcp_message_rx() for HCP events and commands (run later from
  msg_rx_work)
  nci_rf_deactivate_ntf_packet(), nci_nfcee_discover_ntf_packet() and
  nci_core_reset_ntf_packet() in ntf.c
  the zero-status NCI response handlers in rsp.c

If one of these completes HCI command R1 early, this correctly returns
-EPROTO. When R1's real response arrives afterwards,
nci_hci_resp_received() still stores it unconditionally:

net/nfc/nci/hci.c:nci_hci_resp_received() {
	...
	conn_info->rx_skb = skb;

exit:
	nci_req_complete(ndev, NCI_STATUS_OK);
}

That function doesn't check whether a request is pending or which pipe
the response is for. It also doesn't free an skb that is already stored.
The same thing happens when R1 hits NCI_DATA_TIMEOUT in __nci_request()
and the NFCC answers late.

None of nci_hci_send_cmd(), nci_hci_get_param(), nci_hci_set_param() or
nci_hci_open_pipe() clears rx_skb before calling nci_request().

For the next command R2, there seem to be two outcomes.

If R2's own response arrives, it overwrites the orphan and the orphaned
skb leaks. Could an NFCC that keeps sending late or unsolicited responses
leak skbs without bound this way?

If R2 is also completed early, nci_hci_take_resp() parses R1's HCP result
code and hands R1's payload to R2's caller. R2's own response then becomes
the next orphan, so the mismatch carries forward. Callers that would get
the wrong payload include:

  nci_hci_create_pipe(), which reads resp->pipe at offset 4 with no
  length check, so a payload-less ANY_OPEN_PIPE response would be read
  past its data
  nci_hci_dev_session_init() (session identity)
  st_nci_hci_load_session() (pipe list)
  st_nci_control_se() (host list)
  st_nci_se_get_atr() (ATR)

The commit message says the helper returns -EPROTO "when the request was
completed without a response". In this sequence the request succeeds with
the previous request's response.

Would it make sense to drop any stale rx_skb before sending, for example
in the req callback under req_lock? Separately, nci_hci_resp_received()
could store a response only while a request is pending, freeing any
previous skb.

> +
> +	message = (struct nci_hcp_message *)rx_skb->data;
> +	r = nci_hci_result_to_errno(NCI_HCP_MSG_GET_CMD(message->header));
> +	skb_pull(rx_skb, NCI_HCI_HCP_MESSAGE_HEADER_LEN);
> +
> +	if (!r && skb)
> +		*skb = rx_skb;
> +	else
> +		kfree_skb(rx_skb);
> +
> +	return r;
> +}

[ ... ]

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

Re: [PATCH net 3/5] nfc: nci: consume the loopback response exactly once

[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_nfcc_loopback() (net/nfc/nci/core.c) can still use or write
  freed memory.
- [Medium] The patch consumes conn_info->rx_skb only when nci_request()
  returns NCI_STATUS_OK.
- [Medium] The read-then-clear the patch adds to nci_nfcc_loopback()
  (core.c:471-472, `rx_skb = conn_info->rx_skb; conn_info->rx_skb =…

--

nfc: nci: consume the loopback response exactly once

This patch changes nci_nfcc_loopback() so that, after a successful
nci_request(), it detaches conn_info->rx_skb before using it. The skb is
freed if the caller passed no resp pointer, and -EPROTO is returned if the
request completed without a data packet. The goal is to fix a
use-after-free and double free that happen when a stray zero-status NCI
response completes a loopback request.

> diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
> index 73e3a96470ace..8e2a755ce2555 100644
> --- a/net/nfc/nci/core.c
> +++ b/net/nfc/nci/core.c

[ ... ]

> @@ -464,8 +464,20 @@ int nci_nfcc_loopback(struct nci_dev *ndev, const void *data, size_t data_len,
>  	ndev->cur_conn_id = conn_id;
>  	r = nci_request(ndev, nci_send_data_req, &loopback_data,
>  			msecs_to_jiffies(NCI_DATA_TIMEOUT));
> -	if (r == NCI_STATUS_OK && resp)
> -		*resp = conn_info->rx_skb;
> +	if (r == NCI_STATUS_OK) {

[Severity: Medium]
Can a stale rx_skb from an earlier loopback still be returned here?

The rx_skb is only consumed when nci_request() returns NCI_STATUS_OK.
Nothing clears or frees conn_info->rx_skb before the new request is sent.
The same is true on the -ETIMEDOUT, -ERESTARTSYS and non-zero status
paths.

data_exchange_cb is never cleared. nci_data_exchange_complete() checks
NCI_DATA_EXCHANGE only for the static RF connection:

net/nfc/nci/data.c:nci_data_exchange_complete() {
    ...
	if (conn_id == NCI_STATIC_RF_CONN_ID &&
	    !test_and_clear_bit(NCI_DATA_EXCHANGE, &ndev->flags)) {
    ...
}

Suppose loopback #1 times out and its data packet arrives late.
nci_nfcc_loopback_cb() still stores the packet:

	conn_info->rx_skb = skb;

Suppose loopback #2 is then completed by a stray zero-status response,
like the CORE_SET_CONFIG_RSP in the commit message. Wouldn't this code
return the skb from loopback #1 as the reply to loopback #2?
st_nci_loopback() would then copy that skb's data to userspace.

If the real data for loopback #2 arrives instead, nci_nfcc_loopback_cb()
overwrites rx_skb without freeing it. Does that leak the late skb?

nci_core_conn_close_rsp_packet() and nci_unregister_device() also drop
conn_info without freeing any skb still stored in rx_skb.

st21nfca_hci_loopback(), by comparison, clears its rx_skb before it sends.
Could conn_info->rx_skb be freed and cleared before nci_request() here,
and on the error paths too?

> +		/* Consume the stored data packet exactly once, see
> +		 * nci_nfcc_loopback_cb().
> +		 */
> +		rx_skb = conn_info->rx_skb;
> +		conn_info->rx_skb = NULL;

[Severity: High]
Is conn_info still valid at this point?

conn_info is looked up before the blocking nci_request() call, and
ndev->cur_conn_id is set to the loopback conn_id. While the request is
pending, the NFCC can send an unsolicited CORE_CONN_CLOSE_RSP with
status 0. nci_rsp_packet() picks the handler by opcode only, and the
handler does:

net/nfc/nci/rsp.c:nci_core_conn_close_rsp_packet() {
    ...
	if (status == NCI_STATUS_OK) {
		conn_info = nci_get_conn_info_by_conn_id(ndev,
							 ndev->cur_conn_id);
		if (conn_info) {
			list_del(&conn_info->list);
			...
			devm_kfree(&ndev->nfc_dev->dev, conn_info->dest_params);
			devm_kfree(&ndev->nfc_dev->dev, conn_info);
		}
	}
	nci_req_complete(ndev, status);
}

That completes the loopback request with NCI_STATUS_OK. The code above then
reads rx_skb from freed memory, and with this patch it also writes NULL
into that memory.

If the slab object has been reused, could the new NULL store corrupt the
new owner? If the read returns a non-NULL value, st_nci_loopback() would
read skb->len and skb->data and then call kfree_skb() on it. That is the
same skb use-after-free and double free the commit message describes.

Would it be safer to look up conn_info again after nci_request() returns,
or to check that the completing response belongs to this request?

[Severity: Medium]
Can this read-then-clear race with nci_nfcc_loopback_cb()?

These are two plain accesses, and they share no lock with the code that
stores rx_skb. nci_nfcc_loopback_cb() runs from rx_work and doesn't take
req_lock:

	conn_info->rx_skb = skb;

	nci_req_complete(ndev, NCI_STATUS_OK);

In the case this patch targets, a stray zero-status response completes
the request first. rx_work can then handle the real loopback data packet
while the woken caller is in this code:

caller                                rx_work
rx_skb = conn_info->rx_skb; /* NULL */
                                      conn_info->rx_skb = skb;
conn_info->rx_skb = NULL;
return -EPROTO;

Does that leak the skb stored by the callback? This is also an unlocked
race on a plain pointer, which KCSAN would report.

> +		if (!rx_skb)
> +			return -EPROTO;

[ ... ]

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