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:
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?
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.
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.
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.
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
linux-stable-mirror@lists.linaro.org
-
Jakub Kicinski -
Juraj Šarinay -
Michael Thalmeier