Skip to contentSkip to conversation

NFC: llcp: integer underflow in nfc_llcp_set_remote_gb()

PATCH In review Dan Carpenter 2 participants 3 messages Jan 31, 2013 – Mar 1, 2013 1 message not in the archive

Subsystems (4)
Networking Network drivers Kernel janitors NFC
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 NFC: llcp: integer underflow in nfc_llcp_set_remote_gb()

If gb_len is less than 3 it would cause an integer underflow and possibly memory corruption in nfc_llcp_parse_gb_tlv().

I removed the old test for gb_len = 0. I also removed the test for ->remote_gb = NULL. It's not possible for ->remote_gb to be NULL and we have already dereferenced ->remote_gb_len so it's too late to test.

The old test return -ENODEV but my test returns -EINVAL.

Signed-off-by Dan Carpenter <[email protected]>
net/nfc/llcp/llcp.c +2 -3
index 85bc75c..746f5a2 100644
@@ -549,14 +549,13 @@ int nfc_llcp_set_remote_gb(struct nfc_dev *dev, u8 *gb, u8 gb_len)
 		pr_err("No LLCP device\n");
 		return -ENODEV;
 	}
+	if (gb_len < 3)
+		return -EINVAL;
 
 	memset(local->remote_gb, 0, NFC_MAX_GT_LEN);
 	memcpy(local->remote_gb, gb, gb_len);
 	local->remote_gb_len = gb_len;
 
-	if (local->remote_gb = NULL || local->remote_gb_len = 0)
-		return -ENODEV;
-
 	if (memcmp(local->remote_gb, llcp_magic, 3)) {
 		pr_err("MAC does not support LLCP\n");
 		return -EINVAL;

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw

Hi Dan,

Quoted 16 lines · On Thursday 31 of January 2013 10:16:46 Dan Carpenter wrote:

If gb_len is less than 3 it would cause an integer underflow and possibly memory corruption in nfc_llcp_parse_gb_tlv().

I removed the old test for gb_len = 0. I also removed the test for ->remote_gb = NULL. It's not possible for ->remote_gb to be NULL and we have already dereferenced ->remote_gb_len so it's too late to test.

The old test return -ENODEV but my test returns -EINVAL.

Signed-off-by Dan Carpenter <[email protected]>
net/nfc/llcp/llcp.c +0 -0
index 85bc75c..746f5a2 100644

net/nfc/llcp/llcp.c int nfc_llcp_set_remote_gb(struct nfc_dev *dev, u8 *gb, u8 gb_len) · line 553

Quoted 5 lines
net/nfc/llcp/llcp.c +2 -0
@@ -549,14 +549,13 @@ int nfc_llcp_set_remote_gb(struct nfc_dev *dev, u8 *gb, u8 gb_len)
 		pr_err("No LLCP device\n");
 		return -ENODEV;
 	}
+	if (gb_len < 3)
+		return -EINVAL;

Maybe define NFC_MIN_GT_LEN and test it together with NFC_MAX_GT_LEN in nfc_set_remote_general_bytes() ?

Quoted 11 lines
net/nfc/llcp/llcp.c +0 -3
 	memset(local->remote_gb, 0, NFC_MAX_GT_LEN);
 	memcpy(local->remote_gb, gb, gb_len);
 	local->remote_gb_len = gb_len;
 
-	if (local->remote_gb = NULL || local->remote_gb_len = 0)
-		return -ENODEV;
-
 	if (memcmp(local->remote_gb, llcp_magic, 3)) {
 		pr_err("MAC does not support LLCP\n");
 		return -EINVAL;
Signature · 2 lines
BR
Szymon Janc
author
#3 lore raw

PATCH 1/1 NFC: llcp: cleanup underflow check

Szymon Janc suggested I should move the lower bound check into the caller and make it match the check for NFC_MAX_GT_LEN.

Signed-off-by Dan Carpenter <[email protected]>
include/net/nfc/nfc.h +1 -0
index 87a6417..e50dd2c 100644
@@ -74,6 +74,7 @@ struct nfc_ops {
 
 #define NFC_TARGET_IDX_ANY -1
 #define NFC_MAX_GT_LEN 48
+#define NFC_MIN_GT_LEN 3
 #define NFC_ATR_RES_GT_OFFSET 15
 
 struct nfc_target {
net/nfc/core.c +1 -1
index 6ceee8e..e2c3b6e 100644
@@ -450,7 +450,7 @@ int nfc_set_remote_general_bytes(struct nfc_dev *dev, u8 *gb, u8 gb_len)
 {
 	pr_debug("dev_name=%s gb_len=%d\n", dev_name(&dev->dev), gb_len);
 
-	if (gb_len > NFC_MAX_GT_LEN)
+	if (gb_len < NFC_MIN_GT_LEN || gb_len > NFC_MAX_GT_LEN)
 		return -EINVAL;
 
 	return nfc_llcp_set_remote_gb(dev, gb, gb_len);
net/nfc/llcp/llcp.c +0 -2
index 7f8266d..7be27fb 100644
@@ -547,8 +547,6 @@ int nfc_llcp_set_remote_gb(struct nfc_dev *dev, u8 *gb, u8 gb_len)
 		pr_err("No LLCP device\n");
 		return -ENODEV;
 	}
-	if (gb_len < 3)
-		return -EINVAL;
 
 	memset(local->remote_gb, 0, NFC_MAX_GT_LEN);
 	memcpy(local->remote_gb, gb, gb_len);