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

[Lukas K.]

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