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