Re: [PATCH net v5] net: nfc: nci: Fix parameter validation for packet data

[Paolo Abeni <pabeni@redhat.com>]

On 1/12/26 1:48 PM, Michael Thalmeier wrote:
> Since commit 9c328f54741b ("net: nfc: nci: Add parameter validation for
> packet data") communication with nci nfc chips is not working any more.
> 
> The mentioned commit tries to fix access of uninitialized data, but
> failed to understand that in some cases the data packet is of variable
> length and can therefore not be compared to the maximum packet length
> given by the sizeof(struct).
> 
> Fixes: 9c328f54741b ("net: nfc: nci: Add parameter validation for packet data")
> Cc: stable@vger.kernel.org
> Signed-off-by: Michael Thalmeier <michael.thalmeier@hale.at>

AFAICS this patch is doing at least 2 separate things:

- what described above,
- adding the missing checkes in
nci_extract_rf_params_nfcf_passive_listen and nci_rf_discover_ntf_packet

the latter is completely not described above and should land in separate
patch; note that whatever follows the '---' separator will not enter the
changelog.

> @@ -138,23 +142,49 @@ static int nci_core_conn_intf_error_ntf_packet(struct nci_dev *ndev,
>  static const __u8 *
>  nci_extract_rf_params_nfca_passive_poll(struct nci_dev *ndev,
>  					struct rf_tech_specific_params_nfca_poll *nfca_poll,
> -					const __u8 *data)
> +					const __u8 *data, size_t data_len)
>  {
> +	/* Check if we have enough data for sens_res (2 bytes) */
> +	if (data_len < 2)
> +		return ERR_PTR(-EINVAL);
> +
>  	nfca_poll->sens_res = __le16_to_cpu(*((__le16 *)data));
>  	data += 2;
> +	data_len -= 2;
> +
> +	/* Check if we have enough data for nfcid1_len (1 byte) */
> +	if (data_len < 1)
> +		return ERR_PTR(-EINVAL);
>  
>  	nfca_poll->nfcid1_len = min_t(__u8, *data++, NFC_NFCID1_MAXSIZE);
> +	data_len--;
>  
>  	pr_debug("sens_res 0x%x, nfcid1_len %d\n",
>  		 nfca_poll->sens_res, nfca_poll->nfcid1_len);
>  
> +	/* Check if we have enough data for nfcid1 */
> +	if (data_len < nfca_poll->nfcid1_len)
> +		return ERR_PTR(-EINVAL);
> +
>  	memcpy(nfca_poll->nfcid1, data, nfca_poll->nfcid1_len);
>  	data += nfca_poll->nfcid1_len;
> +	data_len -= nfca_poll->nfcid1_len;
> +
> +	/* Check if we have enough data for sel_res_len (1 byte) */
> +	if (data_len < 1)
> +		return ERR_PTR(-EINVAL);
>  
>  	nfca_poll->sel_res_len = *data++;
> +	data_len--;
> +
> +	if (nfca_poll->sel_res_len != 0) {
> +		/* Check if we have enough data for sel_res (1 byte) */
> +		if (data_len < 1)
> +			return ERR_PTR(-EINVAL);
>  
> -	if (nfca_poll->sel_res_len != 0)
>  		nfca_poll->sel_res = *data++;
> +		data_len--;

Last decrement not needed as data_len is never used afterwards.

> @@ -181,16 +221,32 @@ nci_extract_rf_params_nfcb_passive_poll(struct nci_dev *ndev,
>  static const __u8 *
>  nci_extract_rf_params_nfcf_passive_poll(struct nci_dev *ndev,
>  					struct rf_tech_specific_params_nfcf_poll *nfcf_poll,
> -					const __u8 *data)
> +					const __u8 *data, size_t data_len)
>  {
> +	/* Check if we have enough data for bit_rate (1 byte) */
> +	if (data_len < 1)
> +		return ERR_PTR(-EINVAL);
> +
>  	nfcf_poll->bit_rate = *data++;
> +	data_len--;
> +
> +	/* Check if we have enough data for sensf_res_len (1 byte) */
> +	if (data_len < 1)
> +		return ERR_PTR(-EINVAL);
> +
>  	nfcf_poll->sensf_res_len = min_t(__u8, *data++, NFC_SENSF_RES_MAXSIZE);
> +	data_len--;
>  
>  	pr_debug("bit_rate %d, sensf_res_len %d\n",
>  		 nfcf_poll->bit_rate, nfcf_poll->sensf_res_len);
>  
> +	/* Check if we have enough data for sensf_res */
> +	if (data_len < nfcf_poll->sensf_res_len)
> +		return ERR_PTR(-EINVAL);
> +
>  	memcpy(nfcf_poll->sensf_res, data, nfcf_poll->sensf_res_len);
>  	data += nfcf_poll->sensf_res_len;
> +	data_len -= nfcf_poll->sensf_res_len;

Same here.

/P