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

["Lukas K." <lu@0x83.eu>]

On 15.01.26 12:25, Paolo Abeni wrote:
> 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);
>>   
As far as I can tell, the code ensures that data_len never underflows. 
If that'd happen all subsequent length checks would pass, even though 
they should not. Using  a signed type (ssize_t?) would provide some 
extra safety in here.
>> +	/* 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--;