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

[Simon Horman <horms@kernel.org>]

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 skb length checks to nci_core_init_rsp_packet_v1() and
nci_core_init_rsp_packet_v2() before parsing the CORE_INIT_RSP packet to
prevent reads of uninitialized memory.

> diff --git a/net/nfc/nci/rsp.c b/net/nfc/nci/rsp.c
> index 9eeb862825c5f..36e49fb7d8941 100644
> --- a/net/nfc/nci/rsp.c
> +++ b/net/nfc/nci/rsp.c
> @@ -50,6 +50,9 @@ 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;
>  
> +	if (skb->len < sizeof(*rsp_1))
> +		return NCI_STATUS_SYNTAX_ERROR;
> +

Does this length check break the handling of valid NCI error responses?

If the device returns an error status, the response payload might only
contain the 1-byte status field. By requiring the full sizeof(*rsp_1)
before checking the status, will this return NCI_STATUS_SYNTAX_ERROR
instead of propagating the actual error code from the device?

>  	pr_debug("status 0x%x\n", rsp_1->status);
>  
>  	if (rsp_1->status != NCI_STATUS_OK)

[ ... ]

> @@ -87,15 +94,21 @@ 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;
>  
> +	if (skb->len < sizeof(*rsp))
> +		return NCI_STATUS_SYNTAX_ERROR;
> +

Could the same issue occur here for v2 responses?

If a command fails and the response payload only includes the status
field, checking for the full sizeof(*rsp) will drop the error response
and obscure the true failure reason.

>  	pr_debug("status %x\n", rsp->status);
>  
>  	if (rsp->status != NCI_STATUS_OK)
>  		return rsp->status;