[PATCH net v3] nfc: nci: fix use of uninitialized memory in NFC-DEP general bytes

[PATCH net v3] nfc: nci: fix use of uninitialized memory in NFC-DEP general bytes

[Muhammad Bilal]

nci_store_general_bytes_nfc_dep() derives the length of the NFC-DEP
general bytes by subtracting the fixed general-bytes offset from the ATR
length:

  atr_res_len - NFC_ATR_RES_GT_OFFSET   (poll, offset 15)
  atr_req_len - NFC_ATR_REQ_GT_OFFSET   (listen, offset 14)

It never checks that the ATR is at least that long. When a
RF_INTF_ACTIVATED_NTF reports an ATR shorter than the offset the
subtraction is negative; because min_t() casts its arguments to __u8 the
negative value becomes large and is then capped at
NFC_ATR_RES_GB_MAXSIZE / NFC_ATR_REQ_GB_MAXSIZE. remote_gb_len is thus
set to up to 47/48 even though only atr_res_len/atr_req_len bytes of the
on-stack atr_res/atr_req buffer were copied from the packet, and the
following memcpy() reads the uninitialized remainder into
ndev->remote_gb.

Skip storing the general bytes when the ATR is shorter than the
general-bytes offset. remote_gb_len is already zeroed unconditionally
at the top of the function by commit 9c328f54741b ("net: nfc: nci: Add
parameter validation for packet data"), so the short-ATR case needs no
separate zeroing here anymore, unlike in the v2 posted upstream.

Fixes: a99903ec4566 ("NFC: NCI: Handle Target mode activation")
Cc: stable@vger.kernel.org
Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
---
v3: Rebased onto current net.git for-next, requested by David
    Heidelberg. The function moved and gained an unconditional
    "remote_gb_len = 0" at entry (from commit 9c328f54741b, landed
    after v2 was posted), which made v2's explicit zeroing inside the
    short-ATR branch redundant, so this version drops it and only adds
    the missing length checks.
v2: Also zero remote_gb_len explicitly in the short-ATR branch so a
    stale value from a previous activation doesn't survive into the
    new session.

 net/nfc/nci/ntf.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/nfc/nci/ntf.c b/net/nfc/nci/ntf.c
index c96512bb8653..e0555c90d657 100644
--- a/net/nfc/nci/ntf.c
+++ b/net/nfc/nci/ntf.c
@@ -631,6 +631,9 @@ static int nci_store_general_bytes_nfc_dep(struct nci_dev *ndev,
 	switch (ntf->activation_rf_tech_and_mode) {
 	case NCI_NFC_A_PASSIVE_POLL_MODE:
 	case NCI_NFC_F_PASSIVE_POLL_MODE:
+		if (ntf->activation_params.poll_nfc_dep.atr_res_len <
+		    NFC_ATR_RES_GT_OFFSET)
+			break;
 		ndev->remote_gb_len = min_t(__u8,
 			(ntf->activation_params.poll_nfc_dep.atr_res_len
 						- NFC_ATR_RES_GT_OFFSET),
@@ -643,6 +646,9 @@ static int nci_store_general_bytes_nfc_dep(struct nci_dev *ndev,
 
 	case NCI_NFC_A_PASSIVE_LISTEN_MODE:
 	case NCI_NFC_F_PASSIVE_LISTEN_MODE:
+		if (ntf->activation_params.listen_nfc_dep.atr_req_len <
+		    NFC_ATR_REQ_GT_OFFSET)
+			break;
 		ndev->remote_gb_len = min_t(__u8,
 			(ntf->activation_params.listen_nfc_dep.atr_req_len
 						- NFC_ATR_REQ_GT_OFFSET),
-- 
2.55.0

Re: [PATCH net v3] nfc: nci: fix use of uninitialized memory in NFC-DEP general bytes

[David Heidelberg]

On 28/07/2026 15:28, Muhammad Bilal wrote:
> nci_store_general_bytes_nfc_dep() derives the length of the NFC-DEP
> general bytes by subtracting the fixed general-bytes offset from the ATR
> length:
> 
>    atr_res_len - NFC_ATR_RES_GT_OFFSET   (poll, offset 15)
>    atr_req_len - NFC_ATR_REQ_GT_OFFSET   (listen, offset 14)
> 
> It never checks that the ATR is at least that long. When a
> RF_INTF_ACTIVATED_NTF reports an ATR shorter than the offset the
> subtraction is negative; because min_t() casts its arguments to __u8 the
> negative value becomes large and is then capped at
> NFC_ATR_RES_GB_MAXSIZE / NFC_ATR_REQ_GB_MAXSIZE. remote_gb_len is thus
> set to up to 47/48 even though only atr_res_len/atr_req_len bytes of the
> on-stack atr_res/atr_req buffer were copied from the packet, and the
> following memcpy() reads the uninitialized remainder into
> ndev->remote_gb.
> 
> Skip storing the general bytes when the ATR is shorter than the
> general-bytes offset. remote_gb_len is already zeroed unconditionally
> at the top of the function by commit 9c328f54741b ("net: nfc: nci: Add
> parameter validation for packet data"), so the short-ATR case needs no
> separate zeroing here anymore, unlike in the v2 posted upstream.
> 
> Fixes: a99903ec4566 ("NFC: NCI: Handle Target mode activation")
> Cc: stable@vger.kernel.org
> Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
> ---
> v3: Rebased onto current net.git for-next, requested by David
>      Heidelberg. The function moved and gained an unconditional
>      "remote_gb_len = 0" at entry (from commit 9c328f54741b, landed
>      after v2 was posted), which made v2's explicit zeroing inside the
>      short-ATR branch redundant, so this version drops it and only adds
>      the missing length checks.
> v2: Also zero remote_gb_len explicitly in the short-ATR branch so a
>      stale value from a previous activation doesn't survive into the
>      new session.
> 
>   net/nfc/nci/ntf.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
Hello Muhammad and Lekë.

I see you recently addressing same issues, such as this patch:

nfc: nci: fix use of uninitialized memory in NFC-DEP general bytes

and later Lekë's:

nfc: nci: fix u8 underflow in nci_store_general_bytes_nfc_dep [1].

As I see you finding similar bugs and addressing them, I would be more than 
happy if you would be interested to sync who's working on what and - that's huge 
value here - cross review your patches, as you working in the similar field. 
Sending good patch is great, but giving a good review can have a huge value and 
move things forwards [2] (as you can see Tested-by and Reviewed-by is important 
number too :) ).

Thank you both for your work
David

[1] https://lore.kernel.org/r/20260729011547.19191-1-snowwlake@icloud.com
[2] https://lwn.net/Articles/1077425/

Re: [PATCH net v3] nfc: nci: fix use of uninitialized memory in NFC-DEP general bytes

[Muhammad Bilal]

Hi David, Lekë,

After analyzing the caller execution paths and commit history, here are
two technical details worth noting:

1. Fixes tags:
   - 767f19ae698e ("NFC: Implement NCI dep_link_up and dep_link_down")
     introduced the POLL-mode atr_res_len subtraction (2012).
   - a99903ec4566 ("NFC: NCI: Handle Target mode activation")
     refactored the code into nci_store_general_bytes_nfc_dep() and
     added the LISTEN-mode atr_req_len branch (2014).
   Since the fix addresses both branches, including both Fixes tags is
   necessary for proper stable backporting.

2. Return status:
   Returning NCI_STATUS_RF_PROTOCOL_ERROR (as proposed by Lekë) is the
   correct behavior. In nci_rf_intf_activated_ntf_packet(), a non-OK
   return value properly skips nci_target_auto_activated(), propagates
   the error via nci_req_complete(), and prevents nfc_tm_activated()
   from firing. Falling through with NCI_STATUS_OK would incorrectly
   treat a malformed ATR as a successful activation.

I have submitted v4 [1], which incorporates the NCI_STATUS_RF_PROTOCOL_ERROR
return value, includes both Fixes tags, and credits Lekë with a
Suggested-by tag.

[1] https://lore.kernel.org/all/20260812092423.161497-1-meatuni001@gmail.com/

Thanks,
Muhammad