Skip to contentSkip to conversation

tehuti: using uninitialized data in bdx_ioctl_priv()

PATCH In review Dan Carpenter 3 participants 4 messages Jun 24, 2013

Subsystems (4)
Networking Network drivers Kernel janitors Tehuti Ethernet driver
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 tehuti: using uninitialized data in bdx_ioctl_priv()

If we "cmd = SIOCDEVPRIVATE" then we use data[] without initializing it. The most common case is that we would return -EOPNOTSUPP. The other case is that we'd end up reading and writing to randomish places. This requires CAP_SYS_RAWIO so it's not very bad.

The fix is to not allow SIOCDEVPRIVATE because it doesn't work. I returned -EOPNOTSUPP instead of -ENOTTY because that's what is used in the rest of the file.

Signed-off-by Dan Carpenter <[email protected]>

---
This bug is several years old.

drivers/net/ethernet/tehuti/tehuti.c +9 -7
index 571452e..5d08f38 100644
@@ -647,14 +647,16 @@ static int bdx_ioctl_priv(struct net_device *ndev, struct ifreq *ifr, int cmd)
 	ENTER;
 
 	DBG("jiffies=%ld cmd=%d\n", jiffies, cmd);
-	if (cmd != SIOCDEVPRIVATE) {
-		error = copy_from_user(data, ifr->ifr_data, sizeof(data));
-		if (error) {
-			pr_err("can't copy from user\n");
-			RET(-EFAULT);
-		}
-		DBG("%d 0x%x 0x%x\n", data[0], data[1], data[2]);
+
+	if (cmd = SIOCDEVPRIVATE)
+		RET(-EOPNOTSUPP);
+
+	error = copy_from_user(data, ifr->ifr_data, sizeof(data));
+	if (error) {
+		pr_err("can't copy from user\n");
+		RET(-EFAULT);
 	}
+	DBG("%d 0x%x 0x%x\n", data[0], data[1], data[2]);
 
 	if (!capable(CAP_SYS_RAWIO))
 		return -EPERM;
maintainer
#1 lore raw

From: Dan Carpenter <[email protected]>
Date: Mon, 24 Jun 2013 19:05:03 +0300

Quoted 10 lines

If we "cmd = SIOCDEVPRIVATE" then we use data[] without initializing it. The most common case is that we would return -EOPNOTSUPP. The other case is that we'd end up reading and writing to randomish places. This requires CAP_SYS_RAWIO so it's not very bad.

The fix is to not allow SIOCDEVPRIVATE because it doesn't work. I returned -EOPNOTSUPP instead of -ENOTTY because that's what is used in the rest of the file.

Signed-off-by Dan Carpenter <[email protected]>

I think the intention is to only allow SIOCDEVPRIVATE, rather than accept any and all values other than it which are inside of the private ioctl range.

The 'cmd' validation is one step, and it determines the interpretation of data[0].

#2 lore raw
Quoted 12 lines · David Miller

From: Dan Carpenter <[email protected]>
Date: Mon, 24 Jun 2013 19:05:03 +0300

Quoted 9 lines

If we "cmd = SIOCDEVPRIVATE" then we use data[] without initializing it. The most common case is that we would return -EOPNOTSUPP. The other case is that we'd end up reading and writing to randomish places. This requires CAP_SYS_RAWIO so it's not very bad.

The fix is to not allow SIOCDEVPRIVATE because it doesn't work. I returned -EOPNOTSUPP instead of -ENOTTY because that's what is used in the rest of the file.

Quoted 8 lines
Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>

I think the intention is to only allow SIOCDEVPRIVATE, rather than accept any and all values other than it which are inside of the private ioctl range.

The 'cmd' validation is one step, and it determines the interpretation of data[0].

But data is only initialised on the error path. So this whole function is useless. It might as well be removed entirely.

(Also, drivers generally should not assign SIOCDEVPRIVATE+{0,1,2}, as those numbers used to be conventionally used for MII operations.)

Ben.

Signature · 3 lines
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
maintainer
#3 lore raw

From: Ben Hutchings <[email protected]>
Date: Mon, 24 Jun 2013 21:24:12 +0100

Quoted 19 lines · David Miller
Quoted 18 lines

From: Dan Carpenter <[email protected]>
Date: Mon, 24 Jun 2013 19:05:03 +0300

Quoted 10 lines

If we "cmd = SIOCDEVPRIVATE" then we use data[] without initializing it. The most common case is that we would return -EOPNOTSUPP. The other case is that we'd end up reading and writing to randomish places. This requires CAP_SYS_RAWIO so it's not very bad.

The fix is to not allow SIOCDEVPRIVATE because it doesn't work. I returned -EOPNOTSUPP instead of -ENOTTY because that's what is used in the rest of the file.

Signed-off-by Dan Carpenter <[email protected]>

I think the intention is to only allow SIOCDEVPRIVATE, rather than accept any and all values other than it which are inside of the private ioctl range.

Quoted 8 lines
Quoted 2 lines

The 'cmd' validation is one step, and it determines the interpretation of data[0].

But data is only initialised on the error path. So this whole function is useless. It might as well be removed entirely.

(Also, drivers generally should not assign SIOCDEVPRIVATE+{0,1,2}, as those numbers used to be conventionally used for MII operations.)

Agreed, this function should be removed, and if someone has to have this functionality they can implement ETHTOOL_GREGS.