netdev - [PATCH] nfc: st21nfca: Refactor EVT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-Id: <20220329175431.3175472-1-mfaltesek@google.com>
Date:   Tue, 29 Mar 2022 12:54:31 -0500
From:   Martin Faltesek <mfaltesek@...omium.org>
To:     netdev@...r.kernel.org, krzk@...nel.org,
        christophe.ricard@...il.com, jordy@...ing.systems
Cc:     sameo@...ux.intel.com, wklin@...gle.com, groeck@...gle.com,
        surenb@...gle.com, mfaltesek@...gle.com, gregkh@...uxfoundation.org
Subject: [PATCH] nfc: st21nfca: Refactor EVT_TRANSACTION

EVT_TRANSACTION has four different bugs:

1. First conditional has logical AND but should be OR. It should
   always check if it isn't NFC_EVT_TRANSACTION_AID_TAG, then
   bail.

2. Potential under allocating memory:devm_kzalloc (skb->len - 2)
   when the aid_len specified in the packet is less than the fixed
   NFC_MAX_AID_LENGTH in struct nfc_evt_transaction. In addition,
   aid_len is u32 in the data structure, and u8 in the packet,
   under counting 3 more bytes.

3. Memory leaks after kzalloc when returning error.

4. The final conditional check is also incorrect, for the same reasons
   explained in #2.

Fixes: 26fc6c7f02cb ("NFC: st21nfca: Add HCI transaction event support")
Fixes: 4fbcc1a4cb20 ("nfc: st21nfca: Fix potential buffer overflows in EVT_TRANSACTION")
Signed-off-by: Martin Faltesek <mfaltesek@...gle.com>
---
 drivers/nfc/st21nfca/se.c | 65 ++++++++++++++++++++++++++-------------
 1 file changed, 43 insertions(+), 22 deletions(-)

diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
index c922f10d0d7b..acc8d831246a 100644
--- a/drivers/nfc/st21nfca/se.c
+++ b/drivers/nfc/st21nfca/se.c
@@ -292,6 +292,8 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
 	int r = 0;
 	struct device *dev = &hdev->ndev->dev;
 	struct nfc_evt_transaction *transaction;
+	u32 aid_len;
+	u8 params_len;
 
 	pr_debug("connectivity gate event: %x\n", event);
 
@@ -306,37 +308,56 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
 		 * Description	Tag	Length
 		 * AID		81	5 to 16
 		 * PARAMETERS	82	0 to 255
+		 *
+		 * The key differences are aid storage length is variably sized
+		 * in the packet, but fixed in nfc_evt_transaction, and that the aid_len
+		 * is u8 in the packet, but u32 in the structure, and the tags in
+		 * the packet are not part of nfc_evt_transaction.
+		 *
+		 * size in bytes: 1          1       5-16  1             1	      0-255
+		 * offset:                   1	     2	   aid_len + 2   aid_len + 3  aid_len + 4
+		 * member name  : aid_tag(M) aid_len aid   params_tag(M) params_len   params
+		 * example      :  0x81      5-16     X    0x82          0-255	      X
 		 */
-		if (skb->len < NFC_MIN_AID_LENGTH + 2 &&
-		    skb->data[0] != NFC_EVT_TRANSACTION_AID_TAG)
-			return -EPROTO;
-
-		transaction = devm_kzalloc(dev, skb->len - 2, GFP_KERNEL);
-		if (!transaction)
-			return -ENOMEM;
 
-		transaction->aid_len = skb->data[1];
+		/*
+		 * Validate the packet is large enough to read the first two bytes
+		 * containing the aid_tag and aid_len, and then read both. Capacity
+		 * checks are expanded incrementally after this, for clarity.
+		 */
+		if (skb->len < 2 || skb->data[0] != NFC_EVT_TRANSACTION_AID_TAG)
+			return -EPROTO;
 
-		/* Checking if the length of the AID is valid */
-		if (transaction->aid_len > sizeof(transaction->aid))
-			return -EINVAL;
+		aid_len = skb->data[1];
+		/*
+		 * With the actual aid_len, verify there is enough space in
+		 * the packet to read params_tag and params_len, and that
+		 * aid_len does not exceed destination capacity. Reference
+		 * offset comment above for +4 +3 +2 offsets used.
+		 */
+		if (skb->len < aid_len + 4 || aid_len > sizeof(transaction->aid))
+			return -EPROTO;
 
-		memcpy(transaction->aid, &skb->data[2],
-		       transaction->aid_len);
+		params_len = skb->data[aid_len + 3];
 
-		/* Check next byte is PARAMETERS tag (82) */
-		if (skb->data[transaction->aid_len + 2] !=
-		    NFC_EVT_TRANSACTION_PARAMS_TAG)
+		/*
+		 * Verify PARAMETERS tag is (82), and final validation that enough
+		 * space in packet to read everything.
+		 */
+		if ((skb->data[aid_len + 2] != NFC_EVT_TRANSACTION_PARAMS_TAG) ||
+		    (skb->len < aid_len + 4 + params_len))
 			return -EPROTO;
 
-		transaction->params_len = skb->data[transaction->aid_len + 3];
+		transaction = devm_kzalloc(dev, sizeof(struct nfc_evt_transaction) +
+			params_len, GFP_KERNEL);
+		if (!transaction)
+			return -ENOMEM;
 
-		/* Total size is allocated (skb->len - 2) minus fixed array members */
-		if (transaction->params_len > ((skb->len - 2) - sizeof(struct nfc_evt_transaction)))
-			return -EINVAL;
+		transaction->aid_len = aid_len;
+		transaction->params_len = params_len;
 
-		memcpy(transaction->params, skb->data +
-		       transaction->aid_len + 4, transaction->params_len);
+		memcpy(transaction->aid, &skb->data[2], aid_len);
+		memcpy(transaction->params, &skb->data[aid_len + 4], params_len);
 
 		r = nfc_se_transaction(hdev->ndev, host, transaction);
 	break;
-- 
2.35.1.1021.g381101b075-goog