Re: [RFC] let mortals use ethtool
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 [EMAIL PROTECTED] -- James Morris [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] let mortals use ethtool
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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] let mortals use ethtool
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.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] let mortals use ethtool
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] let mortals use ethtool
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
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] let mortals use ethtool
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] let mortals use ethtool
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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] let mortals use ethtool
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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] let mortals use ethtool
Another example: registers often have sensitive information such as WoL passwords or WEP keys stored in them. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] let mortals use ethtool
From: Auke Kok [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] let mortals use ethtool
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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] let mortals use ethtool
From: James Morris [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] let mortals use ethtool
From: Jeff Garzik [EMAIL PROTECTED] 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? - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] let mortals use ethtool
From: Michael Chan [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] let mortals use ethtool
David Miller wrote: From: Michael Chan [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] let mortals use ethtool
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] let mortals use ethtool
From: Jeff Garzik [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] let mortals use ethtool
On Thu, 28 Sep 2006 16:23:46 -0400 Jeff Garzik [EMAIL PROTECTED] 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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
