[RFC] let mortals use ethtool

[RFC] let mortals use ethtool

[Stephen Hemminger]

There is no reason to not allow non-admin users to query network
statistics and settings.


--- linux-2.6.orig/net/core/ethtool.c
+++ linux-2.6/net/core/ethtool.c
@@ -806,13 +806,6 @@ int dev_ethtool(struct ifreq *ifr)
 	int rc;
 	unsigned long old_features;
 
-	/*
-	 * XXX: This can be pushed down into the ethtool_* handlers that
-	 * need it.  Keep existing behaviour for the moment.
-	 */
-	if (!capable(CAP_NET_ADMIN))
-		return -EPERM;
-
 	if (!dev || !netif_device_present(dev))
 		return -ENODEV;
 
@@ -822,6 +815,33 @@ int dev_ethtool(struct ifreq *ifr)
 	if (copy_from_user(&ethcmd, useraddr, sizeof (ethcmd)))
 		return -EFAULT;
 
+	/* Allow some commands to be done by anyone */
+	switch(ethcmd) {
+	case ETHTOOL_GSET:
+	case ETHTOOL_GDRVINFO:
+	case ETHTOOL_GREGS:
+	case ETHTOOL_GWOL:
+	case ETHTOOL_GMSGLVL:
+	case ETHTOOL_GLINK:
+	case ETHTOOL_GCOALESCE:
+	case ETHTOOL_GRINGPARAM:
+	case ETHTOOL_GPAUSEPARAM:
+	case ETHTOOL_GRXCSUM:
+	case ETHTOOL_GTXCSUM:
+	case ETHTOOL_GSG:
+	case ETHTOOL_GSTRINGS:
+	case ETHTOOL_PHYS_ID:
+	case ETHTOOL_GSTATS:
+	case ETHTOOL_GTSO:
+	case ETHTOOL_GPERMADDR:
+	case ETHTOOL_GUFO:
+	case ETHTOOL_GGSO:
+		break;
+	default:
+		if (!capable(CAP_NET_ADMIN))
+			return -EPERM;
+	}
+
 	if(dev->ethtool_ops->begin)
 		if ((rc = dev->ethtool_ops->begin(dev)) < 0)
 			return rc;
@@ -947,6 +967,10 @@ int dev_ethtool(struct ifreq *ifr)
 	return rc;
 
  ioctl:
+	/* Keep existing behaviour for the moment.	 */
+	if (!capable(CAP_NET_ADMIN))
+		return -EPERM;
+
 	if (dev->do_ioctl)
 		return dev->do_ioctl(dev, ifr, SIOCETHTOOL);
 	return -EOPNOTSUPP;

Re: [RFC] let mortals use ethtool

[James Morris]

On Thu, 28 Sep 2006, Stephen Hemminger wrote:

> 
> There is no reason to not allow non-admin users to query network
> statistics and settings.

Acked-by: James Morris <jmorris@namei.org>



-- 
James Morris
<jmorris@namei.org>

Re: [RFC] let mortals use ethtool

[Auke Kok]

Stephen Hemminger wrote:
> There is no reason to not allow non-admin users to query network
> statistics and settings.
> 
> 
> --- linux-2.6.orig/net/core/ethtool.c
> +++ linux-2.6/net/core/ethtool.c
> @@ -806,13 +806,6 @@ int dev_ethtool(struct ifreq *ifr)
>  	int rc;
>  	unsigned long old_features;
>  
> -	/*
> -	 * XXX: This can be pushed down into the ethtool_* handlers that
> -	 * need it.  Keep existing behaviour for the moment.
> -	 */
> -	if (!capable(CAP_NET_ADMIN))
> -		return -EPERM;
> -
>  	if (!dev || !netif_device_present(dev))
>  		return -ENODEV;
>  
> @@ -822,6 +815,33 @@ int dev_ethtool(struct ifreq *ifr)
>  	if (copy_from_user(&ethcmd, useraddr, sizeof (ethcmd)))
>  		return -EFAULT;
>  
> +	/* Allow some commands to be done by anyone */
> +	switch(ethcmd) {
> +	case ETHTOOL_GSET:
> +	case ETHTOOL_GDRVINFO:
> +	case ETHTOOL_GREGS:
> +	case ETHTOOL_GWOL:
> +	case ETHTOOL_GMSGLVL:
> +	case ETHTOOL_GLINK:
> +	case ETHTOOL_GCOALESCE:
> +	case ETHTOOL_GRINGPARAM:
> +	case ETHTOOL_GPAUSEPARAM:
> +	case ETHTOOL_GRXCSUM:
> +	case ETHTOOL_GTXCSUM:
> +	case ETHTOOL_GSG:
> +	case ETHTOOL_GSTRINGS:
> +	case ETHTOOL_PHYS_ID:

PHYS_ID pokes in hardware and makes it jump through hoops, in the case of some hardware 
this could create a local DOS attack (e1000 suffers fromt his, probably more if not all) 
where the NIC might stop receiving packets, or the big lock is help indefinately.

Not a good idea

The other ones are fine I think.

Auke


> +	case ETHTOOL_GSTATS:
> +	case ETHTOOL_GTSO:
> +	case ETHTOOL_GPERMADDR:
> +	case ETHTOOL_GUFO:
> +	case ETHTOOL_GGSO:
> +		break;
> +	default:
> +		if (!capable(CAP_NET_ADMIN))
> +			return -EPERM;
> +	}
> +
>  	if(dev->ethtool_ops->begin)
>  		if ((rc = dev->ethtool_ops->begin(dev)) < 0)
>  			return rc;
> @@ -947,6 +967,10 @@ int dev_ethtool(struct ifreq *ifr)
>  	return rc;
>  
>   ioctl:
> +	/* Keep existing behaviour for the moment.	 */
> +	if (!capable(CAP_NET_ADMIN))
> +		return -EPERM;
> +
>  	if (dev->do_ioctl)
>  		return dev->do_ioctl(dev, ifr, SIOCETHTOOL);
>  	return -EOPNOTSUPP;
> -
> 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

Re: [RFC] let mortals use ethtool

[Michael Chan]

On Thu, 2006-09-28 at 12:25 -0700, Stephen Hemminger wrote:

> +	/* Allow some commands to be done by anyone */
> +	switch(ethcmd) {
> +	case ETHTOOL_GSET:
> +	case ETHTOOL_GDRVINFO:
> +	case ETHTOOL_GREGS:
> +	case ETHTOOL_GWOL:
> +	case ETHTOOL_GMSGLVL:
> +	case ETHTOOL_GLINK:
> +	case ETHTOOL_GCOALESCE:
> +	case ETHTOOL_GRINGPARAM:
> +	case ETHTOOL_GPAUSEPARAM:
> +	case ETHTOOL_GRXCSUM:
> +	case ETHTOOL_GTXCSUM:
> +	case ETHTOOL_GSG:
> +	case ETHTOOL_GSTRINGS:
> +	case ETHTOOL_PHYS_ID:
> +	case ETHTOOL_GSTATS:
> +	case ETHTOOL_GTSO:
> +	case ETHTOOL_GPERMADDR:
> +	case ETHTOOL_GUFO:
> +	case ETHTOOL_GGSO:

I'm against letting normal users do ETHTOOL_GREGS and ETHTOOL_PHYS_ID.
Dumping 64K worth of registers and blinking the LEDs should be
restricted.  But I have no problem doing these checks in the driver if
necessary.

Re: [RFC] let mortals use ethtool

[Jeff Garzik]

Stephen Hemminger wrote:
> There is no reason to not allow non-admin users to query network
> statistics and settings.

NAK.

Some functions in the past didn't like getting hit rapidly in succession.

I would agree to this, but only after an exhaustive audit of each driver 
and each sub-ioctl.  Right now, I only have confidence in GDRVINFO 
probably being safe -- but still that requires an audit, since in rare 
cases the driver may be poking firmware and eeprom areas.

Finally, I fixed a buffer overflow in ethtool version 5, so an audit to 
make sure overflows cannot affect the kernel is basically _required_.

	Jeff

Re: [RFC] let mortals use ethtool

[Jeff Garzik]

Michael Chan wrote:
> On Thu, 2006-09-28 at 12:25 -0700, Stephen Hemminger wrote:
> 
>> +	/* Allow some commands to be done by anyone */
>> +	switch(ethcmd) {
>> +	case ETHTOOL_GSET:
>> +	case ETHTOOL_GDRVINFO:
>> +	case ETHTOOL_GREGS:
>> +	case ETHTOOL_GWOL:
>> +	case ETHTOOL_GMSGLVL:
>> +	case ETHTOOL_GLINK:
>> +	case ETHTOOL_GCOALESCE:
>> +	case ETHTOOL_GRINGPARAM:
>> +	case ETHTOOL_GPAUSEPARAM:
>> +	case ETHTOOL_GRXCSUM:
>> +	case ETHTOOL_GTXCSUM:
>> +	case ETHTOOL_GSG:
>> +	case ETHTOOL_GSTRINGS:
>> +	case ETHTOOL_PHYS_ID:
>> +	case ETHTOOL_GSTATS:
>> +	case ETHTOOL_GTSO:
>> +	case ETHTOOL_GPERMADDR:
>> +	case ETHTOOL_GUFO:
>> +	case ETHTOOL_GGSO:
> 
> I'm against letting normal users do ETHTOOL_GREGS and ETHTOOL_PHYS_ID.
> Dumping 64K worth of registers and blinking the LEDs should be
> restricted.  But I have no problem doing these checks in the driver if
> necessary.

It is because of issues like these that we should not open up the entire 
list above, all at once.  Each sub-ioctl needs careful consideration and 
driver auditing.

	Jeff

Re: [RFC] let mortals use ethtool

[James Morris]

On Thu, 28 Sep 2006, Michael Chan wrote:

> I'm against letting normal users do ETHTOOL_GREGS and ETHTOOL_PHYS_ID.
> Dumping 64K worth of registers and blinking the LEDs should be
> restricted.

Out of curiosity -- why?



-- 
James Morris
<jmorris@namei.org>

Re: [RFC] let mortals use ethtool

[Jeff Garzik]

James Morris wrote:
> On Thu, 28 Sep 2006, Michael Chan wrote:
> 
>> I'm against letting normal users do ETHTOOL_GREGS and ETHTOOL_PHYS_ID.
>> Dumping 64K worth of registers and blinking the LEDs should be
>> restricted.
> 
> Out of curiosity -- why?

In the past, dumping certain tg3 registers has led to lock-ups, for one.

Also, blindly dumping registers can negatively affect driver operation, 
for the case where reading registers has side effects, such as PHY 
register bit shifting, or read-and-clear registers.

	Jeff

Re: [RFC] let mortals use ethtool

[Michael Chan]

On Thu, 2006-09-28 at 16:35 -0400, Jeff Garzik wrote:
> James Morris wrote:
> > On Thu, 28 Sep 2006, Michael Chan wrote:
> > 
> >> I'm against letting normal users do ETHTOOL_GREGS and ETHTOOL_PHYS_ID.
> >> Dumping 64K worth of registers and blinking the LEDs should be
> >> restricted.
> > 
> > Out of curiosity -- why?
> 
> In the past, dumping certain tg3 registers has led to lock-ups, for one.
> 
> Also, blindly dumping registers can negatively affect driver operation, 
> for the case where reading registers has side effects, such as PHY 
> register bit shifting, or read-and-clear registers.
> 

That's right.  For the LED blinking part, you don't want normal users to
override the normal functions of LEDs which are to indicate link, speed,
traffic, etc.

Re: [RFC] let mortals use ethtool

[Jeff Garzik]

Another example:  registers often have sensitive information such as WoL 
passwords or WEP keys stored in them.

Re: [RFC] let mortals use ethtool

[David Miller]

From: Auke Kok <auke-jan.h.kok@intel.com>
Date: Thu, 28 Sep 2006 12:56:25 -0700

> PHYS_ID pokes in hardware and makes it jump through hoops, in the case of some hardware 
> this could create a local DOS attack (e1000 suffers fromt his, probably more if not all) 
> where the NIC might stop receiving packets, or the big lock is help indefinately.
> 
> Not a good idea
> 
> The other ones are fine I think.

I've applied Stephen's patch with PHYS_ID removed from the
allowed list, thanks for the feedback.

Re: [RFC] let mortals use ethtool

[Michael Chan]

On Thu, 2006-09-28 at 14:53 -0700, David Miller wrote:

> I've applied Stephen's patch with PHYS_ID removed from the
> allowed list, thanks for the feedback.

Can you remove GREGS as well?  Allowing users to dump all the registers
will be very problematic.

Re: [RFC] let mortals use ethtool

[David Miller]

From: "Michael Chan" <mchan@broadcom.com>
Date: Thu, 28 Sep 2006 13:17:04 -0700

> I'm against letting normal users do ETHTOOL_GREGS and ETHTOOL_PHYS_ID.
> Dumping 64K worth of registers and blinking the LEDs should be
> restricted.  But I have no problem doing these checks in the driver if
> necessary.

Ok I removed PHYS_ID and GREGS from the allowed list.
Any others?

Re: [RFC] let mortals use ethtool

[David Miller]

From: Jeff Garzik <jgarzik@pobox.com>
Date: Thu, 28 Sep 2006 16:23:46 -0400

> NAK.
> 
> Some functions in the past didn't like getting hit rapidly in succession.
> 
> I would agree to this, but only after an exhaustive audit of each driver 
> and each sub-ioctl.  Right now, I only have confidence in GDRVINFO 
> probably being safe -- but still that requires an audit, since in rare 
> cases the driver may be poking firmware and eeprom areas.

I think the purely software state ones are safe, such as GDRVINGO,
GSG, GTSO, etc.  This absolutely excludes things like PHYS_ID
and GREGS which almost always have to touch the hardware.

> Finally, I fixed a buffer overflow in ethtool version 5, so an audit to 
> make sure overflows cannot affect the kernel is basically _required_.

Absolutely.

I think we should put in Stephen's change with the obvious PHYS_ID
and GREGS exceptions removed.  We have a lot of time to make sure
everything remaining is kosher for 2.6.19 ok?

Re: [RFC] let mortals use ethtool

[David Miller]

From: James Morris <jmorris@namei.org>
Date: Thu, 28 Sep 2006 16:32:38 -0400 (EDT)

> On Thu, 28 Sep 2006, Michael Chan wrote:
> 
> > I'm against letting normal users do ETHTOOL_GREGS and ETHTOOL_PHYS_ID.
> > Dumping 64K worth of registers and blinking the LEDs should be
> > restricted.
> 
> Out of curiosity -- why?

Reading some registers might have side effects, that's one thing.

Secondarily, looping over reading all of the registers of the chip
might kill performance since the IO accesses compete with the
normal packet sending/receiving operations.

Re: [RFC] let mortals use ethtool

[David Miller]

From: "Michael Chan" <mchan@broadcom.com>
Date: Thu, 28 Sep 2006 15:03:40 -0700

> On Thu, 2006-09-28 at 14:53 -0700, David Miller wrote:
> 
> > I've applied Stephen's patch with PHYS_ID removed from the
> > allowed list, thanks for the feedback.
> 
> Can you remove GREGS as well?  Allowing users to dump all the registers
> will be very problematic.

I did, see my subsequent email.

Re: [RFC] let mortals use ethtool

[Jeff Garzik]

David Miller wrote:
> From: "Michael Chan" <mchan@broadcom.com>
> Date: Thu, 28 Sep 2006 13:17:04 -0700
> 
>> I'm against letting normal users do ETHTOOL_GREGS and ETHTOOL_PHYS_ID.
>> Dumping 64K worth of registers and blinking the LEDs should be
>> restricted.  But I have no problem doing these checks in the driver if
>> necessary.
> 
> Ok I removed PHYS_ID and GREGS from the allowed list.
> Any others?

GWOL now spits out a password for all users -> security risk.  Ditto 
GEEPROM.  GSET has been known to cause hangs if done in a tight loop, on 
some 10/100 cards, which is now permitted by any user.  At the very 
least, it should be rate-limited.

I wasn't just being obstinate, when requesting an audit.

	Jeff

Re: [RFC] let mortals use ethtool

[Jeff Garzik]

David Miller wrote:
> Secondarily, looping over reading all of the registers of the chip
> might kill performance since the IO accesses compete with the
> normal packet sending/receiving operations.

This can be true of any ethtool sub-ioctl that beats the registers, if 
run in a tight loop.  PHYs are particularly nasty, because outside of 
hardware problems mentioned in the previous email, many phy read 
routines contain a metric ton of udelay() and mdelay()s.  Now, an 
unpriv'd user can cause the kernel to do tons of in-kernel busy-waits, 
in effect spinning the CPU at their mercy.

	Jeff

Re: [RFC] let mortals use ethtool

[David Miller]

From: Jeff Garzik <jgarzik@pobox.com>
Date: Thu, 28 Sep 2006 18:25:26 -0400

> GWOL now spits out a password for all users -> security risk.  Ditto 
> GEEPROM.  GSET has been known to cause hangs if done in a tight loop, on 
> some 10/100 cards, which is now permitted by any user.  At the very 
> least, it should be rate-limited.
> 
> I wasn't just being obstinate, when requesting an audit.

Ok, I've removed GSET, GWOL and GSTATS (GEEPROM was not in the
original list in Stephen's patch).  In fact I'll remove
GLINK too as that might touch the hardware as well.

That leaves us with:

	case ETHTOOL_GDRVINFO:
	case ETHTOOL_GMSGLVL:
	case ETHTOOL_GCOALESCE:
	case ETHTOOL_GRINGPARAM:
	case ETHTOOL_GPAUSEPARAM:
	case ETHTOOL_GRXCSUM:
	case ETHTOOL_GTXCSUM:
	case ETHTOOL_GSG:
	case ETHTOOL_GSTRINGS:
	case ETHTOOL_GTSO:
	case ETHTOOL_GPERMADDR:
	case ETHTOOL_GUFO:
	case ETHTOOL_GGSO:

Which should be ok.

And once again, take even this list with a grain of salt, we
have until 2.6.19-final to sort this out and audit things.

Re: [RFC] let mortals use ethtool

[Stephen Hemminger]

On Thu, 28 Sep 2006 16:23:46 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> Stephen Hemminger wrote:
> > There is no reason to not allow non-admin users to query network
> > statistics and settings.
> 
> NAK.
> 
> Some functions in the past didn't like getting hit rapidly in succession.
> 
> I would agree to this, but only after an exhaustive audit of each driver 
> and each sub-ioctl.  Right now, I only have confidence in GDRVINFO 
> probably being safe -- but still that requires an audit, since in rare 
> cases the driver may be poking firmware and eeprom areas.
> 
> Finally, I fixed a buffer overflow in ethtool version 5, so an audit to 
> make sure overflows cannot affect the kernel is basically _required_.
> 
> 	Jeff

The first step should be conservative, so why not allow GDRVINFO, and the
various offload setting GTSO, GxSUM, ...

Agreed, that PHY stuff, register area, WOL, are bad. The statistics stuff
also might be a problem for some chips.

-- 
Stephen Hemminger <shemminger@osdl.org>