| Message ID | 20130624160503.GC31984@elgon.mountain |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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]. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html