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

[Thorsten Leemhuis]

On 2/7/26 01:58, Lukas K. wrote:
> 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@xxxxxxxxxxxxxxx
>>> Signed-off-by: Michael Thalmeier <michael.thalmeier@xxxxxxx>
>>
>> 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.

Please correct me if I'm wrong, but seems things are stalled here and
there is no fix for the regression in sight that is know since
2025-12-09:
https://lore.kernel.org/all/20251209132103.3736761-1-michael.thalmeier@xxxxxxx/

I might be missing something, but if not I guess it's best if we revert
the culprit quickly, given these factors:
* We are way past the "[fix regressions] within a week, preferably
before the next rc." that Linus would like to see:
https://lore.kernel.org/all/CAHk-%3Dwi86AosXs66-yi54%2BmpQjPu0upxB8ZAfG%2BLsMyJmcuMSA@xxxxxxxxxxxxxx/
* Lukas running into and reporting same problem recently – and
confirming that a revert helps:
https://lore.kernel.org/all/11b7567e-b9a4-4546-9b1c-bb9820bead0c@xxxxxxx/
* The culprit was backported to various stable/longterm kernels.

Or has anyone a better idea?

Ciao, Thorsten

>>> @@ -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--;
> 
> 
>