[patch] tehuti: using uninitialized data in bdx_ioctl_priv()

[patch] tehuti: using uninitialized data in bdx_ioctl_priv()

[Dan Carpenter]

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 <dan.carpenter@oracle.com>
---
This bug is several years old.

diff --git a/drivers/net/ethernet/tehuti/tehuti.c b/drivers/net/ethernet/tehuti/tehuti.c
index 571452e..5d08f38 100644
--- a/drivers/net/ethernet/tehuti/tehuti.c
+++ b/drivers/net/ethernet/tehuti/tehuti.c
@@ -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;

Re: [patch] tehuti: using uninitialized data in bdx_ioctl_priv()

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Mon, 24 Jun 2013 19:05:03 +0300

> 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 <dan.carpenter@oracle.com>

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].

Re: [patch] tehuti: using uninitialized data in bdx_ioctl_priv()

[Ben Hutchings]

On Mon, 2013-06-24 at 13:01 -0700, David Miller wrote:
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Date: Mon, 24 Jun 2013 19:05:03 +0300
> 
> > 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 <dan.carpenter@oracle.com>
> 
> 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.

-- 
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.

Re: [patch] tehuti: using uninitialized data in bdx_ioctl_priv()

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Mon, 24 Jun 2013 21:24:12 +0100

> On Mon, 2013-06-24 at 13:01 -0700, David Miller wrote:
>> From: Dan Carpenter <dan.carpenter@oracle.com>
>> Date: Mon, 24 Jun 2013 19:05:03 +0300
>> 
>> > 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 <dan.carpenter@oracle.com>
>> 
>> 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.)

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