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

[Joas Antonio dos Santos <joasantonio108@gmail.com>]

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