[PATCH] nfc: nci: fix OOB heap read in nci_core_init_rsp_packet_v1()

[PATCH] nfc: nci: fix OOB heap read in nci_core_init_rsp_packet_v1()

[Lekë Hapçiu]

From: Lekë Hapçiu <framemain@outlook.com>

nci_core_init_rsp_packet_v1() uses the raw chip-supplied
num_supported_rf_interfaces byte to compute the rsp_2 pointer, but
the preceding min() already stores the capped value in
ndev->num_supported_rf_interfaces.  When a hostile chip returns
num_supported_rf_interfaces > 4 the memcpy is safe (capped) but rsp_2
lands past the end of the skb, and the fields copied out of it corrupt
nci_dev with data from adjacent kernel heap.

Use the already-capped ndev->num_supported_rf_interfaces for both the
length check and the pointer, making the relationship between the two
explicit.

Fixes: e8c0dacd9836 ("NFC: Update names and structs to NCI spec 1.0 d18")
Signed-off-by: Lekë Hapçiu <framemain@outlook.com>
---
v2: drop intermediate offset variable, check skb->len directly against
    ndev->num_supported_rf_interfaces + sizeof(*rsp_2) (Jakub Kicinski)

 net/nfc/nci/rsp.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/net/nfc/nci/rsp.c b/net/nfc/nci/rsp.c
index 9eeb86282..4aaf362b9 100644
--- a/net/nfc/nci/rsp.c
+++ b/net/nfc/nci/rsp.c
@@ -66,7 +66,16 @@ static u8 nci_core_init_rsp_packet_v1(struct nci_dev *ndev,
 	       rsp_1->supported_rf_interfaces,
 	       ndev->num_supported_rf_interfaces);
 
-	rsp_2 = (void *) (skb->data + 6 + rsp_1->num_supported_rf_interfaces);
+	if (skb->len < sizeof(*rsp_1) + ndev->num_supported_rf_interfaces +
+		       sizeof(*rsp_2)) {
+		pr_err("CORE_INIT_RSP too short: len=%u need=%zu\n",
+		       skb->len,
+		       sizeof(*rsp_1) + ndev->num_supported_rf_interfaces +
+		       sizeof(*rsp_2));
+		return NCI_STATUS_SYNTAX_ERROR;
+	}
+	rsp_2 = (void *)(skb->data + sizeof(*rsp_1) +
+			 ndev->num_supported_rf_interfaces);
 
 	ndev->max_logical_connections = rsp_2->max_logical_connections;
 	ndev->max_routing_table_size =
-- 
2.51.0

Re: [PATCH] nfc: nci: fix OOB heap read in nci_core_init_rsp_packet_v1()

[Simon Horman]

On Sat, Apr 04, 2026 at 12:30:16PM +0200, Lekë Hapçiu wrote:
> From: Lekë Hapçiu <framemain@outlook.com>
> 
> nci_core_init_rsp_packet_v1() uses the raw chip-supplied
> num_supported_rf_interfaces byte to compute the rsp_2 pointer, but
> the preceding min() already stores the capped value in
> ndev->num_supported_rf_interfaces.  When a hostile chip returns
> num_supported_rf_interfaces > 4 the memcpy is safe (capped) but rsp_2
> lands past the end of the skb, and the fields copied out of it corrupt
> nci_dev with data from adjacent kernel heap.
> 
> Use the already-capped ndev->num_supported_rf_interfaces for both the
> length check and the pointer, making the relationship between the two
> explicit.
> 
> Fixes: e8c0dacd9836 ("NFC: Update names and structs to NCI spec 1.0 d18")
> Signed-off-by: Lekë Hapçiu <framemain@outlook.com>
> ---
> v2: drop intermediate offset variable, check skb->len directly against
>     ndev->num_supported_rf_interfaces + sizeof(*rsp_2) (Jakub Kicinski)

I'm unable to locate v1 and Jakub's review of it.
Could you provide a link?

> 
>  net/nfc/nci/rsp.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/net/nfc/nci/rsp.c b/net/nfc/nci/rsp.c
> index 9eeb86282..4aaf362b9 100644
> --- a/net/nfc/nci/rsp.c
> +++ b/net/nfc/nci/rsp.c
> @@ -66,7 +66,16 @@ static u8 nci_core_init_rsp_packet_v1(struct nci_dev *ndev,
>  	       rsp_1->supported_rf_interfaces,
>  	       ndev->num_supported_rf_interfaces);
>  
> -	rsp_2 = (void *) (skb->data + 6 + rsp_1->num_supported_rf_interfaces);
> +	if (skb->len < sizeof(*rsp_1) + ndev->num_supported_rf_interfaces +
> +		       sizeof(*rsp_2)) {

There are accesses to skb->data before this check.
And it seems they could also overrun the length of the skb.
So I think a check needs to be placed towards the beginning
of this function. (Sashiko has something similar to say.)

Also, I don't think that checking skb->len is sufficient,
as data may be present in the non-linear portion of the skb.
I suspect pskb_may_pull is needed.

If so, I think the same problem exists in the call path.

> +		pr_err("CORE_INIT_RSP too short: len=%u need=%zu\n",
> +		       skb->len,
> +		       sizeof(*rsp_1) + ndev->num_supported_rf_interfaces +
> +		       sizeof(*rsp_2));
> +		return NCI_STATUS_SYNTAX_ERROR;
> +	}
> +	rsp_2 = (void *)(skb->data + sizeof(*rsp_1) +
> +			 ndev->num_supported_rf_interfaces);
>  
>  	ndev->max_logical_connections = rsp_2->max_logical_connections;
>  	ndev->max_routing_table_size =

Sashiko also asks if it is valid to read the packet
if num_supported_rf_interfaces is truncated. Won't the beginning
of resp_2 lie in the trailing supported_rf_interfaces of the rsp1
header?