[PATCH v2] nfc: nci: fix use of uninitialized memory in CORE_INIT_RSP parsing

[PATCH v2] nfc: nci: fix use of uninitialized memory in CORE_INIT_RSP parsing

[Yun Zhou]

nci_core_init_rsp_packet_v1() and nci_core_init_rsp_packet_v2() parse
the CORE_INIT_RSP packet without validating that the skb contains
enough data. A malformed response (e.g. injected via virtual_ncidev)
can declare a large num_supported_rf_interfaces while providing
insufficient data, causing reads of uninitialized slab memory. This
is later used in nci_init_complete_req(), triggering a KMSAN
uninit-value warning.

Add skb length checks before accessing packet fields:
- Validate the skb has at least 1 byte for the status field.
- Validate the skb can hold the fixed-size header before parsing.
- In v2, bounds-check each variable-length rf_interface entry and its
  extension parameters within the parsing loop.
- In v1, verify the skb is large enough for both the variable-length
  rf_interfaces array and the trailing rsp_2 structure.

Reported-by: syzbot+46ca2592193f2fb3debc@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=46ca2592193f2fb3debc
Fixes: bcd684aace34 ("net/nfc/nci: Support NCI 2.x initial sequence")
Signed-off-by: Yun Zhou <yun.zhou@windriver.com>
---
v2:
 - Validate the skb has at least 1 byte for the status field.
 - Move the fixed-size header check after the status access.
 - Add Closes: link after Reported-by

 net/nfc/nci/rsp.c | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/net/nfc/nci/rsp.c b/net/nfc/nci/rsp.c
index 9eeb862825c5..61fbe31b427e 100644
--- a/net/nfc/nci/rsp.c
+++ b/net/nfc/nci/rsp.c
@@ -50,11 +50,19 @@ static u8 nci_core_init_rsp_packet_v1(struct nci_dev *ndev,
 	const struct nci_core_init_rsp_1 *rsp_1 = (void *)skb->data;
 	const struct nci_core_init_rsp_2 *rsp_2;
 
+	/* Ensure that the status field can be accessed. */
+	if (skb->len < 1)
+		return NCI_STATUS_SYNTAX_ERROR;
+
 	pr_debug("status 0x%x\n", rsp_1->status);
 
 	if (rsp_1->status != NCI_STATUS_OK)
 		return rsp_1->status;
 
+	/* Success response must contain the full fixed-size header */
+	if (skb->len < sizeof(*rsp_1))
+		return NCI_STATUS_SYNTAX_ERROR;
+
 	ndev->nfcc_features = __le32_to_cpu(rsp_1->nfcc_features);
 	ndev->num_supported_rf_interfaces = rsp_1->num_supported_rf_interfaces;
 
@@ -62,6 +70,10 @@ static u8 nci_core_init_rsp_packet_v1(struct nci_dev *ndev,
 		min((int)ndev->num_supported_rf_interfaces,
 		    NCI_MAX_SUPPORTED_RF_INTERFACES);
 
+	if (skb->len < sizeof(*rsp_1) + rsp_1->num_supported_rf_interfaces +
+		       sizeof(*rsp_2))
+		return NCI_STATUS_SYNTAX_ERROR;
+
 	memcpy(ndev->supported_rf_interfaces,
 	       rsp_1->supported_rf_interfaces,
 	       ndev->num_supported_rf_interfaces);
@@ -87,15 +99,26 @@ static u8 nci_core_init_rsp_packet_v2(struct nci_dev *ndev,
 				      const struct sk_buff *skb)
 {
 	const struct nci_core_init_rsp_nci_ver2 *rsp = (void *)skb->data;
-	const u8 *supported_rf_interface = rsp->supported_rf_interfaces;
+	const u8 *supported_rf_interface;
+	const u8 *skb_end = skb->data + skb->len;
 	u8 rf_interface_idx = 0;
 	u8 rf_extension_cnt = 0;
 
+	/* Ensure that the status field can be accessed. */
+	if (skb->len < 1)
+		return NCI_STATUS_SYNTAX_ERROR;
+
 	pr_debug("status %x\n", rsp->status);
 
 	if (rsp->status != NCI_STATUS_OK)
 		return rsp->status;
 
+	/* Success response must contain the full fixed-size header */
+	if (skb->len < sizeof(*rsp))
+		return NCI_STATUS_SYNTAX_ERROR;
+
+	supported_rf_interface = rsp->supported_rf_interfaces;
+
 	ndev->nfcc_features = __le32_to_cpu(rsp->nfcc_features);
 	ndev->num_supported_rf_interfaces = rsp->num_supported_rf_interfaces;
 
@@ -104,13 +127,23 @@ static u8 nci_core_init_rsp_packet_v2(struct nci_dev *ndev,
 		    NCI_MAX_SUPPORTED_RF_INTERFACES);
 
 	while (rf_interface_idx < ndev->num_supported_rf_interfaces) {
-		ndev->supported_rf_interfaces[rf_interface_idx++] = *supported_rf_interface++;
+		/* Each entry: [rf_interface_type (1B)] [ext_count (1B)] [ext...] */
+		if (supported_rf_interface + 2 > skb_end)
+			break;
+		ndev->supported_rf_interfaces[rf_interface_idx] = *supported_rf_interface++;
 
 		/* skip rf extension parameters */
 		rf_extension_cnt = *supported_rf_interface++;
+		if (supported_rf_interface + rf_extension_cnt > skb_end)
+			break;
+
+		/* Only count the entry after full validation */
+		rf_interface_idx++;
 		supported_rf_interface += rf_extension_cnt;
 	}
 
+	ndev->num_supported_rf_interfaces = rf_interface_idx;
+
 	ndev->max_logical_connections = rsp->max_logical_connections;
 	ndev->max_routing_table_size =
 			__le16_to_cpu(rsp->max_routing_table_size);
-- 
2.43.0

Re: [PATCH v2] nfc: nci: fix use of uninitialized memory in CORE_INIT_RSP parsing

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

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://sashiko.dev
---
nfc: nci: fix use of uninitialized memory in CORE_INIT_RSP parsing

This commit adds bounds checking to nci_core_init_rsp_packet_v1() and
nci_core_init_rsp_packet_v2() to prevent reads of uninitialized memory
when parsing malformed CORE_INIT_RSP packets.

Link: https://syzkaller.appspot.com/bug?extid=46ca2592193f2fb3debc

> diff --git a/net/nfc/nci/rsp.c b/net/nfc/nci/rsp.c
> index 9eeb862825c5f..61fbe31b427eb 100644
> --- a/net/nfc/nci/rsp.c
> +++ b/net/nfc/nci/rsp.c
> @@ -50,11 +50,19 @@ static u8 nci_core_init_rsp_packet_v1(struct nci_dev *ndev,
[ ... ]
>  	ndev->nfcc_features = __le32_to_cpu(rsp_1->nfcc_features);
>  	ndev->num_supported_rf_interfaces = rsp_1->num_supported_rf_interfaces;
>  
>  	ndev->num_supported_rf_interfaces =
>  		min((int)ndev->num_supported_rf_interfaces,
>  		    NCI_MAX_SUPPORTED_RF_INTERFACES);
>  
> +	if (skb->len < sizeof(*rsp_1) + rsp_1->num_supported_rf_interfaces +
> +		       sizeof(*rsp_2))
> +		return NCI_STATUS_SYNTAX_ERROR;

Simon says: As noted, the issue flagged below pre-dates this patch.
	    I do not suggest increasing the scope of this patchset for
	    this.  But you may want to look at it in the context of
	    follow-up.

[Severity: High]
This isn't a bug introduced by this patch, but does temporarily updating
ndev->num_supported_rf_interfaces without holding ndev->req_lock create a
TOCTOU race?

The nci_core_init_rsp_packet_v1() function is processed asynchronously in
nci_rx_work(), temporarily setting ndev->num_supported_rf_interfaces to
the unclamped value from the packet before it is clamped to
NCI_MAX_SUPPORTED_RF_INTERFACES.

Concurrently, if a thread executing nci_init_complete_req() iterates over
ndev->supported_rf_interfaces using ndev->num_supported_rf_interfaces as
the limit, could it read past the bounds of the 4-element array?

Simon says: On the other hand, the issue flagged below seems to have
            been introduced by this patch and I'd appreciate it if
	    you could look over it.

Additionally, if the new length validation check returns early with
NCI_STATUS_SYNTAX_ERROR, doesn't it leave ndev->num_supported_rf_interfaces
updated but the array contents uninitialized, causing further state
corruption?

>  	memcpy(ndev->supported_rf_interfaces,
>  	       rsp_1->supported_rf_interfaces,
>  	       ndev->num_supported_rf_interfaces);
[ ... ]
> @@ -87,15 +99,26 @@ static u8 nci_core_init_rsp_packet_v2(struct nci_dev *ndev,
>  				      const struct sk_buff *skb)
>  {
>  	const struct nci_core_init_rsp_nci_ver2 *rsp = (void *)skb->data;
> -	const u8 *supported_rf_interface = rsp->supported_rf_interfaces;
> +	const u8 *supported_rf_interface;
> +	const u8 *skb_end = skb->data + skb->len;

Simon says: I believe the issue described below is a non-issue
            because I think that skbs are always linear in this context.

[Severity: Medium]
Does using skb->len to compute skb_end assume SKBs are strictly linear?

The skb->len field represents the total packet length, including any paged
fragments. If the SKB is nonlinear, skb_end will point past the end of the
linear buffer allocation.

Iterating through *supported_rf_interface++ up to skb_end could result in
out-of-bounds memory reads.

Could we use skb_tail_pointer() or pskb_may_pull() to guarantee contiguous
bytes in the linear region?

[ ... ]

Simon says: Like the first issue, I believe this one is pre-existing,
            should not effect the progress of this patch, but could
	    be looked at in the context of follow-up.

[Severity: High]
This is a pre-existing issue, but does lockless iteration and concurrent
modification of ndev->conn_info_list lead to linked list corruption and
use-after-free?

The ndev->conn_info_list is iterated locklessly by functions like
nci_get_conn_info_by_conn_id() (called from nci_tx_work()) and
nci_get_conn_info_by_dest_type_params() (called from nci_nfcc_loopback()).

Concurrently, nci_rx_work() can process an injected
NCI_OP_CORE_CONN_CLOSE_RSP packet via nci_core_conn_close_rsp_packet(),
which directly calls list_del() and devm_kfree() on the connection object
without acquiring any synchronization locks.

Because tx_work and rx_work operate on independent workqueues, couldn't an
injected packet free the object while it is actively being iterated by
another CPU, resulting in a use-after-free read and linked list corruption?