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

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

[Michael Thalmeier]

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>
---
v4:
- formatting fixes

v3:
- perform complete checks
- replace magic numbers with offsetofend and sizeof

v2:
- Reference correct commit hash

---
 net/nfc/nci/ntf.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/net/nfc/nci/ntf.c b/net/nfc/nci/ntf.c
index 418b84e2b260..a5cafcd10cc3 100644
--- a/net/nfc/nci/ntf.c
+++ b/net/nfc/nci/ntf.c
@@ -58,7 +58,7 @@ static int nci_core_conn_credits_ntf_packet(struct nci_dev *ndev,
 	struct nci_conn_info *conn_info;
 	int i;
 
-	if (skb->len < sizeof(struct nci_core_conn_credit_ntf))
+	if (skb->len < offsetofend(struct nci_core_conn_credit_ntf, num_entries))
 		return -EINVAL;
 
 	ntf = (struct nci_core_conn_credit_ntf *)skb->data;
@@ -68,6 +68,10 @@ static int nci_core_conn_credits_ntf_packet(struct nci_dev *ndev,
 	if (ntf->num_entries > NCI_MAX_NUM_CONN)
 		ntf->num_entries = NCI_MAX_NUM_CONN;
 
+	if (skb->len < offsetofend(struct nci_core_conn_credit_ntf, num_entries) +
+			ntf->num_entries * sizeof(struct conn_credit_entry))
+		return -EINVAL;
+
 	/* update the credits */
 	for (i = 0; i < ntf->num_entries; i++) {
 		ntf->conn_entries[i].conn_id =
@@ -364,7 +368,7 @@ static int nci_rf_discover_ntf_packet(struct nci_dev *ndev,
 	const __u8 *data;
 	bool add_target = true;
 
-	if (skb->len < sizeof(struct nci_rf_discover_ntf))
+	if (skb->len < offsetofend(struct nci_rf_discover_ntf, rf_tech_specific_params_len))
 		return -EINVAL;
 
 	data = skb->data;
@@ -380,6 +384,10 @@ static int nci_rf_discover_ntf_packet(struct nci_dev *ndev,
 	pr_debug("rf_tech_specific_params_len %d\n",
 		 ntf.rf_tech_specific_params_len);
 
+	if (skb->len < (data - skb->data) +
+			ntf.rf_tech_specific_params_len + sizeof(ntf.ntf_type))
+		return -EINVAL;
+
 	if (ntf.rf_tech_specific_params_len > 0) {
 		switch (ntf.rf_tech_and_mode) {
 		case NCI_NFC_A_PASSIVE_POLL_MODE:
@@ -596,7 +604,7 @@ static int nci_rf_intf_activated_ntf_packet(struct nci_dev *ndev,
 	const __u8 *data;
 	int err = NCI_STATUS_OK;
 
-	if (skb->len < sizeof(struct nci_rf_intf_activated_ntf))
+	if (skb->len < offsetofend(struct nci_rf_intf_activated_ntf, rf_tech_specific_params_len))
 		return -EINVAL;
 
 	data = skb->data;
@@ -628,6 +636,9 @@ static int nci_rf_intf_activated_ntf_packet(struct nci_dev *ndev,
 	if (ntf.rf_interface == NCI_RF_INTERFACE_NFCEE_DIRECT)
 		goto listen;
 
+	if (skb->len < (data - skb->data) + ntf.rf_tech_specific_params_len)
+		return -EINVAL;
+
 	if (ntf.rf_tech_specific_params_len > 0) {
 		switch (ntf.activation_rf_tech_and_mode) {
 		case NCI_NFC_A_PASSIVE_POLL_MODE:
@@ -668,6 +679,13 @@ static int nci_rf_intf_activated_ntf_packet(struct nci_dev *ndev,
 		}
 	}
 
+	if (skb->len < (data - skb->data) +
+			sizeof(ntf.data_exch_rf_tech_and_mode) +
+			sizeof(ntf.data_exch_tx_bit_rate) +
+			sizeof(ntf.data_exch_rx_bit_rate) +
+			sizeof(ntf.activation_params_len))
+		return -EINVAL;
+
 	ntf.data_exch_rf_tech_and_mode = *data++;
 	ntf.data_exch_tx_bit_rate = *data++;
 	ntf.data_exch_rx_bit_rate = *data++;
@@ -679,6 +697,9 @@ static int nci_rf_intf_activated_ntf_packet(struct nci_dev *ndev,
 	pr_debug("data_exch_rx_bit_rate 0x%x\n", ntf.data_exch_rx_bit_rate);
 	pr_debug("activation_params_len %d\n", ntf.activation_params_len);
 
+	if (skb->len < (data - skb->data) + ntf.activation_params_len)
+		return -EINVAL;
+
 	if (ntf.activation_params_len > 0) {
 		switch (ntf.rf_interface) {
 		case NCI_RF_INTERFACE_ISO_DEP:
-- 
2.52.0

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

[Jakub Kicinski]

On Tue, 23 Dec 2025 08:25:52 +0100 Michael Thalmeier wrote:
> diff --git a/net/nfc/nci/ntf.c b/net/nfc/nci/ntf.c
> index 418b84e2b260..a5cafcd10cc3 100644
> --- a/net/nfc/nci/ntf.c
> +++ b/net/nfc/nci/ntf.c

> @@ -380,6 +384,10 @@ static int nci_rf_discover_ntf_packet(struct nci_dev *ndev,
>  	pr_debug("rf_tech_specific_params_len %d\n",
>  		 ntf.rf_tech_specific_params_len);
>  
> +	if (skb->len < (data - skb->data) +
> +			ntf.rf_tech_specific_params_len + sizeof(ntf.ntf_type))
> +		return -EINVAL;

Are we validating ntf.rf_tech_specific_params_len against the
extraction logic in nci_extract_rf_params_nfca_passive_poll()
and friends?

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

[Michael Thalmeier]

Am 04.01.26 um 19:13 schrieb Jakub Kicinski:
> On Tue, 23 Dec 2025 08:25:52 +0100 Michael Thalmeier wrote:
>> diff --git a/net/nfc/nci/ntf.c b/net/nfc/nci/ntf.c
>> index 418b84e2b260..a5cafcd10cc3 100644
>> --- a/net/nfc/nci/ntf.c
>> +++ b/net/nfc/nci/ntf.c
> 
>> @@ -380,6 +384,10 @@ static int nci_rf_discover_ntf_packet(struct nci_dev *ndev,
>>   	pr_debug("rf_tech_specific_params_len %d\n",
>>   		 ntf.rf_tech_specific_params_len);
>>   
>> +	if (skb->len < (data - skb->data) +
>> +			ntf.rf_tech_specific_params_len + sizeof(ntf.ntf_type))
>> +		return -EINVAL;
> 
> Are we validating ntf.rf_tech_specific_params_len against the
> extraction logic in nci_extract_rf_params_nfca_passive_poll()
> and friends?

You are right. The current patch is only validating that the received 
packet is consistent in the way that the rf_tech_specific_params_len 
number of bytes is also contained in the buffer.

There is currently no code that validates that 
nci_extract_rf_params_nfca_passive_poll and friends only access the 
given number of bytes in their logic.
And to be frank, I do not know how to implement this without either 
cluttering the code with validation logic or re-implementing half the 
parsing logic for length validation.

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

[Jakub Kicinski]

On Wed, 7 Jan 2026 11:06:27 +0100 Michael Thalmeier wrote:
> >> @@ -380,6 +384,10 @@ static int nci_rf_discover_ntf_packet(struct nci_dev *ndev,
> >>   	pr_debug("rf_tech_specific_params_len %d\n",
> >>   		 ntf.rf_tech_specific_params_len);
> >>   
> >> +	if (skb->len < (data - skb->data) +
> >> +			ntf.rf_tech_specific_params_len + sizeof(ntf.ntf_type))
> >> +		return -EINVAL;  
> > 
> > Are we validating ntf.rf_tech_specific_params_len against the
> > extraction logic in nci_extract_rf_params_nfca_passive_poll()
> > and friends?  
> 
> You are right. The current patch is only validating that the received 
> packet is consistent in the way that the rf_tech_specific_params_len 
> number of bytes is also contained in the buffer.
> 
> There is currently no code that validates that 
> nci_extract_rf_params_nfca_passive_poll and friends only access the 
> given number of bytes in their logic.
> And to be frank, I do not know how to implement this without either 
> cluttering the code with validation logic or re-implementing half the 
> parsing logic for length validation.

Don't think there's a magic bullet, we'd either have to pass the skb
or "remaining length" to all the parsing helpers and have them ensure
they are not going out of bounds.

There doesn't seem to be a huge number of those helpers but if you
don't wanna tackle trying to validate the accesses maybe just add a
couple of comments that the validation is missing in the helpers called
in the switch statement?

BTW are you actually using the Linux NFC implementation or just playing
with it? I'm not sure I'd trust this code for anything but accumulating
C-V-Es, TBH.

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

[Michael Thalmeier]

Am 08.01.26 um 03:15 schrieb Jakub Kicinski:
> On Wed, 7 Jan 2026 11:06:27 +0100 Michael Thalmeier wrote:
>>>> @@ -380,6 +384,10 @@ static int nci_rf_discover_ntf_packet(struct nci_dev *ndev,
>>>>    	pr_debug("rf_tech_specific_params_len %d\n",
>>>>    		 ntf.rf_tech_specific_params_len);
>>>>    
>>>> +	if (skb->len < (data - skb->data) +
>>>> +			ntf.rf_tech_specific_params_len + sizeof(ntf.ntf_type))
>>>> +		return -EINVAL;
>>>
>>> Are we validating ntf.rf_tech_specific_params_len against the
>>> extraction logic in nci_extract_rf_params_nfca_passive_poll()
>>> and friends?
>>
>> You are right. The current patch is only validating that the received
>> packet is consistent in the way that the rf_tech_specific_params_len
>> number of bytes is also contained in the buffer.
>>
>> There is currently no code that validates that
>> nci_extract_rf_params_nfca_passive_poll and friends only access the
>> given number of bytes in their logic.
>> And to be frank, I do not know how to implement this without either
>> cluttering the code with validation logic or re-implementing half the
>> parsing logic for length validation.
> 
> Don't think there's a magic bullet, we'd either have to pass the skb
> or "remaining length" to all the parsing helpers and have them ensure
> they are not going out of bounds.
> 
> There doesn't seem to be a huge number of those helpers but if you
> don't wanna tackle trying to validate the accesses maybe just add a
> couple of comments that the validation is missing in the helpers called
> in the switch statement?

Will be sending an updated patch with validation in the helpers.

> 
> BTW are you actually using the Linux NFC implementation or just playing
> with it? I'm not sure I'd trust this code for anything but accumulating
> C-V-Es, TBH.

Yes, we are using the Linux NFC implementation with the NCI driver (with 
a NXP PN7150 NFC chip) in production devices. It was actually working 
quite well and stable until the referenced commit completely broke it.

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

[Juraj Šarinay]

On Tue, 2025-12-23 at 08:25 +0100, 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 commit broke existing user workflows, should therefore be handled
as a regression. Please consider reverting it until more refined
validation code has been thoroughly tested.


#regzbot introduced: 9c328f54741bd5465ca1dc717c84c04242fac2e1