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

Muhammad Bilal <[email protected]>
Newsgroups dev.linux.lists.oe-linux-nfc,org.kernel.vger.linux-kernel,org.kernel.vger.netdev,org.kernel.vger.stable
Message-ID <[email protected]>
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.

Reject the notification with NCI_STATUS_RF_PROTOCOL_ERROR when the ATR
is shorter than the general-bytes offset, rather than silently
continuing. The return value is not decorative: in
nci_rf_intf_activated_ntf_packet(), a non-OK status here skips
nci_target_auto_activated(), is propagated as the completion status by
nci_req_complete(), and skips nfc_tm_activated(). Silently continuing
with remote_gb_len left at 0 would report the malformed activation as
a successful one with empty general bytes quietly substituted in,
rather than as the failure it is.

This bug has two independent points of origin. The POLL-mode
subtraction was introduced in commit 767f19ae698e ("NFC: Implement
NCI dep_link_up and dep_link_down"), which is where remote_gb and
remote_gb_len were first added. The LISTEN-mode subtraction did not
exist until commit a99903ec4566 ("NFC: NCI: Handle Target mode
activation"), which refactored the POLL-only code into the current
nci_store_general_bytes_nfc_dep() and added LISTEN-mode support fresh.
Both are tagged below.

Fixes: 767f19ae698e ("NFC: Implement NCI dep_link_up and dep_link_down")
Fixes: a99903ec4566 ("NFC: NCI: Handle Target mode activation")
Cc: [email protected]
Suggested-by: Lekë Hapçiu <[email protected]>
Signed-off-by: Muhammad Bilal <[email protected]>
---
v4: Return NCI_STATUS_RF_PROTOCOL_ERROR instead of silently breaking
    out of the switch, matching the behavior independently proposed
    by Lekë Hapçiu. The break left remote_gb_len at 0 but still fell
    through to the function's final "return NCI_STATUS_OK", so a
    malformed ATR was reported up the stack as a successful
    activation instead of a failed one. Added a second Fixes tag:
    767f19ae698e is where the POLL-mode subtraction was first
    introduced in 2012, predating a99903ec4566 (2014), which only
    introduced the LISTEN-mode branch.
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 c96512b..c2352ea 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)
+			return NCI_STATUS_RF_PROTOCOL_ERROR;
 		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)
+			return NCI_STATUS_RF_PROTOCOL_ERROR;
 		ndev->remote_gb_len = min_t(__u8,
 			(ntf->activation_params.listen_nfc_dep.atr_req_len
 						- NFC_ATR_REQ_GT_OFFSET),
-- 
2.55.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.