* [RFC] Validate ethtool autoneg before relaying
@ 2008-11-26 22:17 Matt Carlson
2008-11-26 23:28 ` Michael Chan
0 siblings, 1 reply; 4+ messages in thread
From: Matt Carlson @ 2008-11-26 22:17 UTC (permalink / raw)
To: netdev@vger.kernel.org
I think I want to get an opinion on the direction of this before I
expend too much effort. As I was auditing the tg3 driver, I found
that ethtool commands are not validated or even range checked before
being passed to the driver. While this maximizes the flexibility of the
ethtool interface, it can and probably will lead to a lot of duplicated
parameter validation checks within the drivers. Rather than go off and
selfishly code an iron set of parameter checks for the tg3 driver, I
wanted to see how much of this work would be accepted as part of the
ethtool interface.
The patch below validates the autoneg parameter of
ethtool_set_settings(). As it is coded, the check makes sure the
parameter can either be AUTONEG_ENABLED or AUTONEG_DISABLED. This makes
any other value an error where it wasn't before. Correcting out of
range values is possible, but I stumbled when trying to decide which
enumeration of autoneg they should be corrected to.
If this type of work is desirable, I'll continue to trickle in patches.
(Patch is not compile tested at the moment.)
=======================================================================
This patch validates the ethtool autoneg parameter before relaying it on
to the driver's set_settings() function. All implementers of
set_settings have been audited and redundant checks have been removed.
---
drivers/net/arm/etherh.c | 10 ++--------
drivers/net/cassini.c | 4 ----
drivers/net/forcedeth.c | 4 +---
drivers/net/ibm_newemac/core.c | 2 --
drivers/net/mii.c | 2 --
drivers/net/natsemi.c | 4 +---
drivers/net/phy/phy.c | 3 ---
drivers/net/r8169.c | 7 +++----
drivers/net/sc92031.c | 2 --
drivers/net/sungem.c | 4 ----
drivers/net/sunhme.c | 3 ---
drivers/net/tulip/de2104x.c | 2 --
net/core/ethtool.c | 3 +++
13 files changed, 10 insertions(+), 40 deletions(-)
diff --git a/drivers/net/arm/etherh.c b/drivers/net/arm/etherh.c
index 9eb9d1b..76c1ace 100644
--- a/drivers/net/arm/etherh.c
+++ b/drivers/net/arm/etherh.c
@@ -601,12 +601,9 @@ static int etherh_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
static int etherh_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
{
- switch (cmd->autoneg) {
- case AUTONEG_ENABLE:
+ if (cmd->autoneg == AUTONEG_ENABLE)
dev->flags |= IFF_AUTOMEDIA;
- break;
-
- case AUTONEG_DISABLE:
+ else {
switch (cmd->port) {
case PORT_TP:
dev->if_port = IF_PORT_10BASET;
@@ -621,9 +618,6 @@ static int etherh_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
}
dev->flags &= ~IFF_AUTOMEDIA;
break;
-
- default:
- return -EINVAL;
}
etherh_setif(dev);
diff --git a/drivers/net/cassini.c b/drivers/net/cassini.c
index bc84c4c..339b181 100644
--- a/drivers/net/cassini.c
+++ b/drivers/net/cassini.c
@@ -4725,10 +4725,6 @@ static int cas_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
unsigned long flags;
/* Verify the settings we care about. */
- if (cmd->autoneg != AUTONEG_ENABLE &&
- cmd->autoneg != AUTONEG_DISABLE)
- return -EINVAL;
-
if (cmd->autoneg == AUTONEG_DISABLE &&
((cmd->speed != SPEED_1000 &&
cmd->speed != SPEED_100 &&
diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index 0d7e575..9c31e62 100644
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -4268,7 +4268,7 @@ static int nv_set_settings(struct net_device *dev, struct ethtool_cmd *ecmd)
if ((ecmd->advertising & mask) == 0)
return -EINVAL;
- } else if (ecmd->autoneg == AUTONEG_DISABLE) {
+ } else {
/* Note: autonegotiation disable, speed 1000 intentionally
* forbidden - noone should need that. */
@@ -4276,8 +4276,6 @@ static int nv_set_settings(struct net_device *dev, struct ethtool_cmd *ecmd)
return -EINVAL;
if (ecmd->duplex != DUPLEX_HALF && ecmd->duplex != DUPLEX_FULL)
return -EINVAL;
- } else {
- return -EINVAL;
}
netif_carrier_off(dev);
diff --git a/drivers/net/ibm_newemac/core.c b/drivers/net/ibm_newemac/core.c
index 87a7066..3660865 100644
--- a/drivers/net/ibm_newemac/core.c
+++ b/drivers/net/ibm_newemac/core.c
@@ -1960,8 +1960,6 @@ static int emac_ethtool_set_settings(struct net_device *ndev,
/* Basic sanity checks */
if (dev->phy.address < 0)
return -EOPNOTSUPP;
- if (cmd->autoneg != AUTONEG_ENABLE && cmd->autoneg != AUTONEG_DISABLE)
- return -EINVAL;
if (cmd->autoneg == AUTONEG_ENABLE && cmd->advertising == 0)
return -EINVAL;
if (cmd->duplex != DUPLEX_HALF && cmd->duplex != DUPLEX_FULL)
diff --git a/drivers/net/mii.c b/drivers/net/mii.c
index 9205605..5484079 100644
--- a/drivers/net/mii.c
+++ b/drivers/net/mii.c
@@ -144,8 +144,6 @@ int mii_ethtool_sset(struct mii_if_info *mii, struct ethtool_cmd *ecmd)
return -EINVAL;
if (ecmd->phy_address != mii->phy_id)
return -EINVAL;
- if (ecmd->autoneg != AUTONEG_DISABLE && ecmd->autoneg != AUTONEG_ENABLE)
- return -EINVAL;
if ((ecmd->speed == SPEED_1000) && (!mii->supports_gmii))
return -EINVAL;
diff --git a/drivers/net/natsemi.c b/drivers/net/natsemi.c
index 9f81fcb..11cd648 100644
--- a/drivers/net/natsemi.c
+++ b/drivers/net/natsemi.c
@@ -2903,13 +2903,11 @@ static int netdev_set_ecmd(struct net_device *dev, struct ethtool_cmd *ecmd)
ADVERTISED_100baseT_Full)) == 0) {
return -EINVAL;
}
- } else if (ecmd->autoneg == AUTONEG_DISABLE) {
+ } else {
if (ecmd->speed != SPEED_10 && ecmd->speed != SPEED_100)
return -EINVAL;
if (ecmd->duplex != DUPLEX_HALF && ecmd->duplex != DUPLEX_FULL)
return -EINVAL;
- } else {
- return -EINVAL;
}
/*
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e4ede60..b912d62 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -248,9 +248,6 @@ int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd)
cmd->advertising &= phydev->supported;
/* Verify the settings we care about. */
- if (cmd->autoneg != AUTONEG_ENABLE && cmd->autoneg != AUTONEG_DISABLE)
- return -EINVAL;
-
if (cmd->autoneg == AUTONEG_ENABLE && cmd->advertising == 0)
return -EINVAL;
diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index dddf6ae..b8a9a20 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -805,11 +805,10 @@ static int rtl8169_set_speed_tbi(struct net_device *dev,
u32 reg;
reg = RTL_R32(TBICSR);
- if ((autoneg == AUTONEG_DISABLE) && (speed == SPEED_1000) &&
- (duplex == DUPLEX_FULL)) {
- RTL_W32(TBICSR, reg & ~(TBINwEnable | TBINwRestart));
- } else if (autoneg == AUTONEG_ENABLE)
+ if (autoneg == AUTONEG_ENABLE)
RTL_W32(TBICSR, reg | TBINwEnable | TBINwRestart);
+ else if (speed == SPEED_1000 && duplex == DUPLEX_FULL)
+ RTL_W32(TBICSR, reg & ~(TBINwEnable | TBINwRestart));
else {
if (netif_msg_link(tp)) {
printk(KERN_WARNING "%s: "
diff --git a/drivers/net/sc92031.c b/drivers/net/sc92031.c
index 590f217..0747d07 100644
--- a/drivers/net/sc92031.c
+++ b/drivers/net/sc92031.c
@@ -1207,8 +1207,6 @@ static int sc92031_ethtool_set_settings(struct net_device *dev,
return -EINVAL;
if (!(cmd->transceiver == XCVR_INTERNAL))
return -EINVAL;
- if (!(cmd->autoneg == AUTONEG_DISABLE || cmd->autoneg == AUTONEG_ENABLE))
- return -EINVAL;
if (cmd->autoneg == AUTONEG_ENABLE) {
if (!(cmd->advertising & (ADVERTISED_Autoneg
diff --git a/drivers/net/sungem.c b/drivers/net/sungem.c
index 44be8df..280122d 100644
--- a/drivers/net/sungem.c
+++ b/drivers/net/sungem.c
@@ -2690,10 +2690,6 @@ static int gem_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
struct gem *gp = netdev_priv(dev);
/* Verify the settings we care about. */
- if (cmd->autoneg != AUTONEG_ENABLE &&
- cmd->autoneg != AUTONEG_DISABLE)
- return -EINVAL;
-
if (cmd->autoneg == AUTONEG_ENABLE &&
cmd->advertising == 0)
return -EINVAL;
diff --git a/drivers/net/sunhme.c b/drivers/net/sunhme.c
index b22d335..9098984 100644
--- a/drivers/net/sunhme.c
+++ b/drivers/net/sunhme.c
@@ -2448,9 +2448,6 @@ static int hme_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
struct happy_meal *hp = netdev_priv(dev);
/* Verify the settings we care about. */
- if (cmd->autoneg != AUTONEG_ENABLE &&
- cmd->autoneg != AUTONEG_DISABLE)
- return -EINVAL;
if (cmd->autoneg == AUTONEG_DISABLE &&
((cmd->speed != SPEED_100 &&
cmd->speed != SPEED_10) ||
diff --git a/drivers/net/tulip/de2104x.c b/drivers/net/tulip/de2104x.c
index 3aa60fa..dea3f99 100644
--- a/drivers/net/tulip/de2104x.c
+++ b/drivers/net/tulip/de2104x.c
@@ -1520,8 +1520,6 @@ static int __de_set_settings(struct de_private *de, struct ethtool_cmd *ecmd)
return -EINVAL;
if (ecmd->transceiver != XCVR_INTERNAL)
return -EINVAL;
- if (ecmd->autoneg != AUTONEG_DISABLE && ecmd->autoneg != AUTONEG_ENABLE)
- return -EINVAL;
if (ecmd->advertising & ~de->media_supported)
return -EINVAL;
if (ecmd->autoneg == AUTONEG_ENABLE &&
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 14ada53..6362f56 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -164,6 +164,9 @@ static int ethtool_set_settings(struct net_device *dev, void __user *useraddr)
if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
return -EFAULT;
+ if (cmd.autoneg != AUTONEG_ENABLE && cmd.autoneg != AUTONEG_DISABLE)
+ return -EFAULT;
+
return dev->ethtool_ops->set_settings(dev, &cmd);
}
--
1.5.6.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC] Validate ethtool autoneg before relaying
2008-11-26 22:17 [RFC] Validate ethtool autoneg before relaying Matt Carlson
@ 2008-11-26 23:28 ` Michael Chan
2008-11-26 23:44 ` Matt Carlson
0 siblings, 1 reply; 4+ messages in thread
From: Michael Chan @ 2008-11-26 23:28 UTC (permalink / raw)
To: Matt Carlson; +Cc: netdev@vger.kernel.org
On Wed, 2008-11-26 at 14:17 -0800, Matt Carlson wrote:
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 14ada53..6362f56 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -164,6 +164,9 @@ static int ethtool_set_settings(struct net_device *dev, void __user *useraddr)
> if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
> return -EFAULT;
>
> + if (cmd.autoneg != AUTONEG_ENABLE && cmd.autoneg != AUTONEG_DISABLE)
> + return -EFAULT;
> +
Matt, you should return -EINVAL here instead.
> return dev->ethtool_ops->set_settings(dev, &cmd);
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC] Validate ethtool autoneg before relaying
2008-11-26 23:28 ` Michael Chan
@ 2008-11-26 23:44 ` Matt Carlson
2008-11-27 8:07 ` David Miller
0 siblings, 1 reply; 4+ messages in thread
From: Matt Carlson @ 2008-11-26 23:44 UTC (permalink / raw)
To: Michael Chan; +Cc: Matthew Carlson, netdev@vger.kernel.org
On Wed, Nov 26, 2008 at 03:28:12PM -0800, Michael Chan wrote:
>
> On Wed, 2008-11-26 at 14:17 -0800, Matt Carlson wrote:
> > diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> > index 14ada53..6362f56 100644
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> > @@ -164,6 +164,9 @@ static int ethtool_set_settings(struct net_device *dev, void __user *useraddr)
> > if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
> > return -EFAULT;
> >
> > + if (cmd.autoneg != AUTONEG_ENABLE && cmd.autoneg != AUTONEG_DISABLE)
> > + return -EFAULT;
> > +
>
> Matt, you should return -EINVAL here instead.
Yes. You are right. I'll fix this in a revised patch if everyone
decides this is an effort worth pursuing.
> > return dev->ethtool_ops->set_settings(dev, &cmd);
> > }
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC] Validate ethtool autoneg before relaying
2008-11-26 23:44 ` Matt Carlson
@ 2008-11-27 8:07 ` David Miller
0 siblings, 0 replies; 4+ messages in thread
From: David Miller @ 2008-11-27 8:07 UTC (permalink / raw)
To: mcarlson; +Cc: mchan, netdev
From: "Matt Carlson" <mcarlson@broadcom.com>
Date: Wed, 26 Nov 2008 15:44:06 -0800
> On Wed, Nov 26, 2008 at 03:28:12PM -0800, Michael Chan wrote:
> >
> > On Wed, 2008-11-26 at 14:17 -0800, Matt Carlson wrote:
> > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> > > index 14ada53..6362f56 100644
> > > --- a/net/core/ethtool.c
> > > +++ b/net/core/ethtool.c
> > > @@ -164,6 +164,9 @@ static int ethtool_set_settings(struct net_device *dev, void __user *useraddr)
> > > if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
> > > return -EFAULT;
> > >
> > > + if (cmd.autoneg != AUTONEG_ENABLE && cmd.autoneg != AUTONEG_DISABLE)
> > > + return -EFAULT;
> > > +
> >
> > Matt, you should return -EINVAL here instead.
>
> Yes. You are right. I'll fix this in a revised patch if everyone
> decides this is an effort worth pursuing.
I think this is a great idea, checking the validity in one spot.
So please, submit a final version of these changes when you get
a chance.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2008-11-27 8:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-11-26 22:17 [RFC] Validate ethtool autoneg before relaying Matt Carlson
2008-11-26 23:28 ` Michael Chan
2008-11-26 23:44 ` Matt Carlson
2008-11-27 8:07 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).