dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

PATCH In review Matt Domsch 4 participants 18 messages Oct 29, 2004 – Nov 4, 2004

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Matt Domsch

author

2004-10-29 21:51 UTC #0 lore raw

PATCH 1/1 dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

If dev->dev_addr is zero, then the memcpy() never takes place, and the same data that was in the caller's buffer is still in the caller's buffer on successful return. The caller can't know that the data in its buffer isn't the right answer. So, if dev->dev_dev_addr == 0, clear the buffer before returning success.

Thanks,
Matt

Signature · 21 lines

Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

===== linux-2.6/net/core/dev.c 1.169 vs edited =====
--- 1.169/net/core/dev.c	2004-10-26 11:09:33 -05:00
+++ edited/linux-2.6/net/core/dev.c	2004-10-29 16:39:33 -05:00
@@ -2375,8 +2375,11 @@
 			return dev_set_mtu(dev, ifr->ifr_mtu);
 
 		case SIOCGIFHWADDR:
-			memcpy(ifr->ifr_hwaddr.sa_data, dev->dev_addr,
-			       min(sizeof ifr->ifr_hwaddr.sa_data, (size_t) dev->addr_len));
+			if (!dev->addr_len)
+				memset(ifr->ifr_hwaddr.sa_data, 0, sizeof ifr->ifr_hwaddr.sa_data);
+			else
+				memcpy(ifr->ifr_hwaddr.sa_data, dev->dev_addr,
+				       min(sizeof ifr->ifr_hwaddr.sa_data, (size_t) dev->addr_len));
 			ifr->ifr_hwaddr.sa_family = dev->type;
 			return 0;

Matt Domsch

author

2004-10-30 01:37 UTC #1 lore raw

Quoted 5 lines · Matt Domsch

If dev->dev_addr is zero, then the memcpy() never takes place, and the same data that was in the caller's buffer is still in the caller's buffer on successful return. The caller can't know that the data in its buffer isn't the right answer. So, if dev->dev_addr == 0, clear the buffer before returning success.

Some additional detail I forgot to include.

s/dev_addr/addr_len in the comments above, that's the field we care about being non-zero.

This directly affects ppp devices, as those have dev->addr_len == 0. This was seen because net-snmp reports the MAC address of devices in the system, and for ppp devices was reporting an address of 0x00FFFFFF because that was the data in the buffer prior to calling ioctl(). This patch causes the 2.6 behaviour to match that of the 2.4 kernel which has a fixed length MAX_ADDR_LEN instead of a variable addr_len and always copies MAX_ADDR_LEN bytes.

Thanks to Jordan Hargrave for root cause analysis and suggesting the fix.

Signed-off-by Matt Domsch <[email protected]>

Signature · 4 lines

Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Herbert Xu

2004-10-30 01:51 UTC #2 lore raw

Quoted 3 lines · Matt Domsch

s/dev_addr/addr_len in the comments above, that's the field we care about being non-zero.

This still doesn't make sense. What if dev->addr_len is less than the size of the buffer? The caller has to know what the length is anyway.

BTW, the ioctl interface is obsolete. Please use the rtnetlink interface where dev->addr_len can be read properly.

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Matt Domsch

author

2004-10-30 03:09 UTC #3 lore raw

Quoted 5 lines · Herbert Xu

Quoted 2 lines
s/dev_addr/addr_len in the comments above, that's the field we care about being non-zero.
This still doesn't make sense. What if dev->addr_len is less than the size of the buffer? The caller has to know what the length is anyway.

Ahh, indeed. net-snmp has hard-coded the number 6 or uses the definition of IFHWADDRLEN (from include/linux/if.h, a copy of which is in /usr/include/linux/if.h of course) in several places for this.

Quoted 2 lines

BTW, the ioctl interface is obsolete. Please use the rtnetlink interface where dev->addr_len can be read properly.

More than I wanted to do tonight, but will investigate.

Thanks,
Matt

Signature · 4 lines

Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

jamal

2004-10-30 19:10 UTC #4 lore raw

Quoted 10 lines · Matt Domsch

Quoted 5 lines · Herbert Xu
Quoted 2 lines
s/dev_addr/addr_len in the comments above, that's the field we care about being non-zero.
This still doesn't make sense. What if dev->addr_len is less than the size of the buffer? The caller has to know what the length is anyway.
Ahh, indeed. net-snmp has hard-coded the number 6 or uses the definition of IFHWADDRLEN (from include/linux/if.h, a copy of which is in /usr/include/linux/if.h of course) in several places for this.

fix the net-snmp code. The addr_len is dependent on the device type. 6 is good for ethernet but may not equate for others.

Having said that i think we should somehow signal that info to user space. perhaps returning -EINVAL in the case the L2 address is 0? EINVAL will break a few apps and make them puke as opposed to silently returning something wrong.

cheers,
jamal

Matt Domsch

author

2004-11-01 04:44 UTC #5 lore raw

Quoted 2 lines · jamal

fix the net-snmp code. The addr_len is dependent on the device type. 6 is good for ethernet but may not equate for others.

I wish it were that simple. The problem, in my mind, is that the SIOCGIFHWADDR ioctl does not behave in 2.6 kernels as it has behaved in previous kernels, and applications have no way to know this. This will lead to unexpected behaviour in many applications that (wrongly) assume that the first 6 or more bytes of sa_data after ioctl() contain valid data, unless told otherwise by a failure return value.

I took the liberty of unpacking all the sources to Fedora Core 3 development tree as of yesterday. Of those I looked into the source for, nearly all the packages that call SIOCGIFHWADDR make an assumption on the number of bytes returned and the validity of such, nearly none clear the request structure before calling it (so when ioctl() returns 0 the app believes the data is correct).

anaconda-10.1.0.0/isys/getmacaddr.c: if (ioctl(sock, SIOCGIFHWADDR, &ifr) < 0)
BROKEN: clears ifr before ioctl, copies first 6 bytes of ifr.ifr_hwaddr.sa_data

busybox-1.00.rc1/networking/udhcp/socket.c: if (ioctl(fd, SIOCGIFHWADDR, &ifr) == 0) {
BROKEN: oesn't clear ifr before ioctl, copies first 6 bytes of ifr.ifr_hraddr.sa_data

busybox-1.00.rc1/networking/nameif.c: if (ioctl(ctl_sk, SIOCGIFHWADDR, &ifr))
BROKEN: doesn't clear ifr before ioctl, compares first ETH_ALEN bytes of ifr.ifr_hraddr.sa_data

busybox-1.00.rc1/networking/libiproute/iptunnel.c: if (ioctl(fd, SIOCGIFHWADDR, &ifr)) {
OK, only copies ifr.ifr_addr.sa_family

busybox-1.00.rc1/libbb/interface.c: if (ioctl(skfd, SIOCGIFHWADDR, &ifr) < 0)
BROKEN: doesn't clear ifr before ioctl, copies first 8 bytes of ifr.ifr_hraddr.sa_data

dhcp-0.10/common.c: if (ioctl(skfd, SIOCGIFHWADDR, &if_hwaddr) < 0)
OK, doesn't clear ifr before ioctl, copies first 6 bytes of ifr.ifr_hraddr.sa_data for sa_family ETHERNET, 0 for PPP.

dhcp-3.0.1/common/discover.c: if (ioctl (sock, SIOCGIFHWADDR, &ifr) < 0)
OK, doesn't clear ifr before ioctl, copies first 6 bytes of ifr.ifr_hraddr.sa_data for sa_family ETHERNET, considers PPP an error.

e2fsprogs-1.35/lib/uuid/gen_uuid.c: if (ioctl(sd, SIOCGIFHWADDR, &ifr) < 0)
BROKEN: doesn't clear ifr before ioctl, examines and copies first 6 bytes of ifr.ifr_hraddr.sa_data

gnome-netstatus-2.8.0/src/netstatus-iface.c: if (ioctl (fd, SIOCGIFHWADDR, &if_req) < 0)
BROKEN: doesn't clear ifr before ioctl, uses sa_family to determine action.

gnome-nettool-0.99.3/src/info.c: ioctl (sockfd, SIOCGIFHWADDR, &ifrcopy);
BROKEN: doesn't clear ifr before ioctl, prints first 6 bytes of sa_data

howl-0.9.6/src/lib/howl/Posix/posix_interface.c: res = ioctl(sock, SIOCGIFHWADDR, &ifr);
howl-0.9.6/src/lib/howl/Posix/posix_interface.c: res = ioctl(sock, SIOCGIFHWADDR, ifr);
BROKEN: doesn't clear ifr before ioctl, copies first 6 bytes of sa_data (2 occurances)

net-snmp-5.1.2/snmplib/snmpv3.c: if (ioctl(sock, SIOCGIFHWADDR, &request)) {
BROKEN: does clear request, copies IFHWADDRLEN bytes of sa_data

net-snmp-5.1.2/agent/mibgroup/tunnel/tunnel.c: if (ioctl(fd, SIOCGIFHWADDR, &ifrq) == 0)
OK: doesn't clear request, doesn't look at sa_data, only sa_family.

net-snmp-5.1.2/agent/mibgroup/mibII/interfaces.c: if (ioctl(fd, SIOCGIFHWADDR, &ifrq) < 0)
BROKEN: doesn't clear ifrq, if ioctl fails it clears the 6 destination bytes. If ioctl succeeds, it copies the first 6 bytes of sa_data. (this was the instance that made me realise the ioctl behaved differently)

net-snmp-5.1.2/agent/mibgroup/mibII/ipv6.c: if (ioctl(s, SIOCGIFHWADDR, &ifr) < 0) {
BROKEN: does clear ifrq, if ioctl fails it clears the 6 destination bytes. If ioctl succeeds, it copies the first 6 bytes of sa_data.

Exercise left to reader:
iproute2-2.6.9/ip/iptunnel.c: err = ioctl(fd, SIOCGIFHWADDR, &ifr);
iproute2-2.6.9/misc/arpd.c: if (ioctl(udp_sock, SIOCGIFHWADDR, &ifr))
iputils/rarpd.c: if (ioctl(fd, SIOCGIFHWADDR, ifrp)) {
iputils/ifenslave.c: rv = ioctl(skfd, SIOCGIFHWADDR, &if_hwaddr);
iputils/ifenslave.c: if (ioctl(skfd, SIOCGIFHWADDR, &ifr) < 0)
irda-utils-0.9.16/irdaping/irdaping.c: if (ioctl(self.fd, SIOCGIFHWADDR, &self.ifr) < 0 ) {
isdn4k-utils-CVS-2003-09-23/ipppd/sys-linux.c: if (ioctl (sockfd, SIOCGIFHWADDR, &ifreq) < 0) {
isdn4k-utils-CVS-2003-09-23/libpcap-0.7.2/pcap-linux.c: if (ioctl(fd, SIOCGIFHWADDR, &ifr) == -1) {
kudzu-1.1.95/kudzu.c: if (ioctl(fd, SIOCGIFHWADDR, &ifr) < 0) goto next;
libgtop-2.8.0/sysdeps/linux/netload.c: if (!ioctl (skfd, SIOCGIFHWADDR, &ifr)) {
ncpfs-2.2.4/ipx-1.0/ipx_cmd.c: err = ioctl(fd_ipx, SIOCGIFHWADDR, &ifr);
net-tools-1.60/lib/interface.c: if (ioctl(skfd, SIOCGIFHWADDR, &ifr) < 0)
net-tools-1.60/lib/interface.c.virtualname: if (ioctl(skfd, SIOCGIFHWADDR, &ifr) < 0)
net-tools-1.60/lib/interface.c.cycle: if (ioctl(skfd, SIOCGIFHWADDR, &ifr) < 0)
net-tools-1.60/lib/interface.c.siunits: if (ioctl(skfd, SIOCGIFHWADDR, &ifr) < 0)
net-tools-1.60/arp.c: if (ioctl(sockfd, SIOCGIFHWADDR, &ifr) < 0) {
net-tools-1.60/iptunnel.c: err = ioctl(fd, SIOCGIFHWADDR, &ifr);
net-tools-1.60/nameif.c: r = ioctl(ctl_sk, SIOCGIFHWADDR, &ifr);
net-tools-1.60/ether-wake.c: if (ioctl(s, SIOCGIFHWADDR, &if_hwaddr) < 0) {
nmap-3.70/tcpip.cc:if (ioctl(sd, SIOCGIFHWADDR, &ifr) < 0 ) {
nmap-3.70/libpcap-possiblymodified/pcap-linux.c: if (ioctl(fd, SIOCGIFHWADDR, &ifr) == -1) {
openswan-2.1.5/testing/attacks/espiv/ipsec_hack.c: if (ioctl (listen_s, SIOCGIFHWADDR, &ifr) < 0) {
openswan-2.1.5/testing/attacks/espiv/ipsec_hack.c: if (ioctl (send_s, SIOCGIFHWADDR, &ifr) < 0) {
ppp-2.4.2/pppd/plugins/rp-pppoe/plugin.c: if (ioctl(fd, SIOCGIFHWADDR, &ifr) < 0) {
ppp-2.4.2/pppd/plugins/rp-pppoe/if.c: if (ioctl(fd, SIOCGIFHWADDR, &ifr) < 0) {
ppp-2.4.2/pppd/sys-linux.c: if (ioctl (sock_fd, SIOCGIFHWADDR, &bestifreq) < 0) {
ppp-2.4.2/pppd/sys-linux.c: ret = ioctl(sock_fd, SIOCGIFHWADDR, &ifreq);
ppp-2.4.2/pppd/sys-linux.c: ok = ioctl (s, SIOCGIFHWADDR, (caddr_t) &ifr) >= 0;

ppp-2.4.2/pppd/sys-linux.c:    if(ioctl(skfd, SIOCGIFHWADDR, &ifr) < 0)
pump-0.8.21/dhcp.c:    if (ioctl(sock, SIOCGIFHWADDR, &req))
quagga-0.97.0/zebra/if_ioctl.c:  ret = if_ioctl (SIOCGIFHWADDR, (caddr_t) &ifreq);
radvd-0.7.2/device-linux.c:	if (ioctl(sock, SIOCGIFHWADDR, &ifr) < 0)
rarpd/rarpd.c:			if (ioctl(fd, SIOCGIFHWADDR, ifrp)) {
rarpd/rarpd.c.ss981107:			if (ioctl(fd, SIOCGIFHWADDR, ifrp)) {
rhpl-0.148/src/ethtool/ethtool.c:  err = ioctl(fd, SIOCGIFHWADDR, &ifr);
rhpl-0.148/src/ethtool/iwlib.c:  if((ioctl(skfd, SIOCGIFHWADDR, &ifr) < 0) ||

rp-pppoe-3.5/src/if.c: if (ioctl(fd, SIOCGIFHWADDR, &ifr) < 0) {
rp-pppoe-3.5/src/plugin.c: if (ioctl(fd, SIOCGIFHWADDR, &ifr) < 0) {
scrollkeeper-0.3.14/libuuid/gen_uuid.c: if (ioctl(sd, SIOCGIFHWADDR, &ifr) < 0)
tcpdump-3.8.2/libpcap-0.8.3/pcap-linux.c: if (ioctl(fd, SIOCGIFHWADDR, &ifr) == -1) {
wvstreams-3.75.0/linuxstreams/wvinterface.cc: if (req(SIOCGIFHWADDR, &ifr))
efibootmgr (not in FC, but I'm the upstream maintainer on it and know it looks at just 6 bytes).

Here's what 2.2.0 (possibly earlier) through 2.4.(current) has:
memcpy(ifr->ifr_hwaddr.sa_data,dev->dev_addr, MAX_ADDR_LEN); ifr->ifr_hwaddr.sa_family=dev->type;
return 0;

Here's what 2.6 has:
memcpy(ifr->ifr_hwaddr.sa_data, dev->dev_addr,

         min(sizeof ifr->ifr_hwaddr.sa_data,
	          (size_t) dev->addr_len));

ifr->ifr_hwaddr.sa_family = dev->type;
return 0;

Thus for dev->addr_len < MAX_ADDR_LEN (8), the behavior is different (fewer bytes are copied, discarding what was most likely zeros) and for dev->addr_len > MAX_ADDR_LEN, the behaviour is different too (more valid bytes are copied), a good thing for apps that care and a no-op for those that thought 6 was a useful number.

Quoted 4 lines

Having said that i think we should somehow signal that info to user space. perhaps returning -EINVAL in the case the L2 address is 0? EINVAL will break a few apps and make them puke as opposed to silently returning something wrong.

My immediate concern was net-snmp, which, on failure, will do the right thing if returning -EINVAL in this case. I did not review all the above to know if they would behave correctly on -EINVAL in that case.

I think -EOVERFLOW would be appropriate return for dev->addr_len > sizeof sa_data, yes?

I'd prefer though, that an "obsolete" function, be marked as such somehow (perhaps print a net_ratelimit()ed KERN_DEBUG message when it's called telling apps to move to rtnetlink), and that the behaviour for everything except overflow be consistent with the prior implementation, at least until such a time that all the apps in the distros are converted to rtnetlink.

Thanks,
Matt

Signature · 4 lines

Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Matt Domsch

author

2004-11-01 17:34 UTC #6 lore raw

Quoted 9 lines · Matt Domsch

I think -EOVERFLOW would be appropriate return for dev->addr_len > sizeof sa_data, yes?
I'd prefer though, that an "obsolete" function, be marked as such somehow (perhaps print a net_ratelimit()ed KERN_DEBUG message when it's called telling apps to move to rtnetlink), and that the behaviour for everything except overflow be consistent with the prior implementation, at least until such a time that all the apps in the distros are converted to rtnetlink.

How's this look?

This makes ioctl(SIOCIFHWADDR) behavior in 2.6 consistent with that of previous kernels wherever possible. It returns -EOVERFLOW if it can't represent the address, zeros the data if dev->addr_len is zero, and prints a KERN_DEBUG message telling people to fix their applications to use rtnetlink.

Compiles on x86 and x86-64 cleanly.

Signed-off-by Matt Domsch <[email protected]>

Signature · 26 lines

Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

===== net/core/dev.c 1.169 vs edited =====
--- 1.169/net/core/dev.c	2004-10-26 11:09:33 -05:00
+++ edited/net/core/dev.c	2004-11-01 11:20:25 -06:00
@@ -2375,8 +2375,16 @@
 			return dev_set_mtu(dev, ifr->ifr_mtu);
 
 		case SIOCGIFHWADDR:
-			memcpy(ifr->ifr_hwaddr.sa_data, dev->dev_addr,
-			       min(sizeof ifr->ifr_hwaddr.sa_data, (size_t) dev->addr_len));
+			if (net_ratelimit())
+				printk(KERN_DEBUG "Warning: %s uses obsolete ioctl(SIOCGIFHWADDR), please convert it to rtnetlink(3,7)\n", current->comm);
+
+			if ((size_t) dev->addr_len > sizeof ifr->ifr_hwaddr.sa_data)
+				return -EOVERFLOW;
+			else if (!dev->addr_len)
+				memset(ifr->ifr_hwaddr.sa_data, 0, sizeof ifr->ifr_hwaddr.sa_data);
+			else
+				memcpy(ifr->ifr_hwaddr.sa_data, dev->dev_addr,
+				       min(sizeof ifr->ifr_hwaddr.sa_data, (size_t) dev->addr_len));
 			ifr->ifr_hwaddr.sa_family = dev->type;
 			return 0;

Herbert Xu

2004-11-01 20:27 UTC #7 lore raw

Review

Quoted 6 lines · Matt Domsch

(quoted diff) +5 -0

+			else if (!dev->addr_len)
+				memset(ifr->ifr_hwaddr.sa_data, 0, sizeof ifr->ifr_hwaddr.sa_data);
+			else
+				memcpy(ifr->ifr_hwaddr.sa_data, dev->dev_addr,
+				       min(sizeof ifr->ifr_hwaddr.sa_data, (size_t) dev->addr_len));

Same problem as before. If dev->addr_len is greater than zero but less than sizeof ifr->ifr_hwaddr.sa_data, then you've got garbage in there. Zero is not a special case.

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Matt Domsch

author

2004-11-01 20:38 UTC #8 lore raw

Quoted 3 lines · Herbert Xu

Same problem as before. If dev->addr_len is greater than zero but less than sizeof ifr->ifr_hwaddr.sa_data, then you've got garbage in there. Zero is not a special case.

OK, I'll unconditionally clear it all first. How's this?

Signed-off-by Matt Domsch <[email protected]>

Signature · 21 lines

Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

===== net/core/dev.c 1.169 vs edited =====
--- 1.169/net/core/dev.c	2004-10-26 11:09:33 -05:00
+++ edited/net/core/dev.c	2004-11-01 14:35:02 -06:00
@@ -2375,6 +2375,12 @@
 			return dev_set_mtu(dev, ifr->ifr_mtu);
 
 		case SIOCGIFHWADDR:
+			if (net_ratelimit())
+				printk(KERN_DEBUG "Warning: %s uses obsolete ioctl(SIOCGIFHWADDR), please convert it to rtnetlink(3,7)\n", current->comm);
+
+			if ((size_t) dev->addr_len > sizeof ifr->ifr_hwaddr.sa_data)
+				return -EOVERFLOW;
+			memset(ifr->ifr_hwaddr.sa_data, 0, sizeof ifr->ifr_hwaddr.sa_data);
 			memcpy(ifr->ifr_hwaddr.sa_data, dev->dev_addr,
 			       min(sizeof ifr->ifr_hwaddr.sa_data, (size_t) dev->addr_len));
 			ifr->ifr_hwaddr.sa_family = dev->type;

Herbert Xu

2004-11-01 20:41 UTC #9 lore raw

Quoted 2 lines · Matt Domsch

OK, I'll unconditionally clear it all first. How's this?

It's nearly there :)

Review

Quoted 5 lines

(quoted diff) +3 -0

+			if ((size_t) dev->addr_len > sizeof ifr->ifr_hwaddr.sa_data)
+				return -EOVERFLOW;
+			memset(ifr->ifr_hwaddr.sa_data, 0, sizeof ifr->ifr_hwaddr.sa_data);
 			memcpy(ifr->ifr_hwaddr.sa_data, dev->dev_addr,
 			       min(sizeof ifr->ifr_hwaddr.sa_data, (size_t) dev->addr_len));

You don't need the min anymore since you've checked that dev->addr_len is not greater than sizeof ifr->ifr_hwaddr.sa_data.

Cheers,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Matt Domsch

author

2004-11-01 20:45 UTC #10 lore raw

Quoted 2 lines · Herbert Xu

You don't need the min anymore since you've checked that dev->addr_len is not greater than sizeof ifr->ifr_hwaddr.sa_data.

Good catch.

Signed-off-by Matt Domsch <[email protected]>

Signature · 23 lines

Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

===== net/core/dev.c 1.169 vs edited =====
--- 1.169/net/core/dev.c	2004-10-26 11:09:33 -05:00
+++ edited/net/core/dev.c	2004-11-01 14:42:59 -06:00
@@ -2375,8 +2375,13 @@
 			return dev_set_mtu(dev, ifr->ifr_mtu);
 
 		case SIOCGIFHWADDR:
-			memcpy(ifr->ifr_hwaddr.sa_data, dev->dev_addr,
-			       min(sizeof ifr->ifr_hwaddr.sa_data, (size_t) dev->addr_len));
+			if (net_ratelimit())
+				printk(KERN_DEBUG "Warning: %s uses obsolete ioctl(SIOCGIFHWADDR), please convert it to rtnetlink(3,7)\n", current->comm);
+
+			if ((size_t) dev->addr_len > sizeof ifr->ifr_hwaddr.sa_data)
+				return -EOVERFLOW;
+			memset(ifr->ifr_hwaddr.sa_data, 0, sizeof ifr->ifr_hwaddr.sa_data);
+			memcpy(ifr->ifr_hwaddr.sa_data, dev->dev_addr, dev->addr_len);
 			ifr->ifr_hwaddr.sa_family = dev->type;
 			return 0;

jamal

2004-11-01 21:50 UTC #11 lore raw

My onluy comment is on use of -EOVERFLOW which i have seen only being used in the context of floating point computation (same as -EUNDERFLOW). I dont know what the right thing to return would be.

cheers,
jamal

Quoted 7 lines · Matt Domsch

Quoted 2 lines · Herbert Xu
You don't need the min anymore since you've checked that dev->addr_len is not greater than sizeof ifr->ifr_hwaddr.sa_data.
Good catch.
Signed-off-by Matt Domsch <[email protected]>

Matt Domsch

author

2004-11-01 21:59 UTC #12 lore raw

Quoted 4 lines · jamal

My onluy comment is on use of -EOVERFLOW which i have seen only being used in the context of floating point computation (same as -EUNDERFLOW). I dont know what the right thing to return would be.

Actually, -EOVERFLOW appears throughout the kernel. A couple examples:

drivers/net/ppp_generic.:ppp_read() uses it to indicate skb->len > sizeof buffer to put the data into.

fs/stat.c:cp_new_stat() and friends

    if (stat->size > MAX_NON_LFS)
          return -EOVERFLOW;

indicates that the file size is larger than can be represented by the app.

I believe our use would be consistent.

Thanks,
Matt

Signature · 4 lines

Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Herbert Xu

2004-11-01 22:06 UTC #13 lore raw

Quoted 2 lines · Matt Domsch

Actually, -EOVERFLOW appears throughout the kernel. A couple examples:

I agree. Please disregard my comment re ERANGE.

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

David S. Miller

maintainer

2004-11-04 00:07 UTC #14 lore raw

Quoted 5 lines · On Tue, 2 Nov 2004 09:06:44 +1100 Herbert Xu <[email protected]>…

Quoted 2 lines · Matt Domsch
Actually, -EOVERFLOW appears throughout the kernel. A couple examples:
I agree. Please disregard my comment re ERANGE.

I think there is nothing wrong with clearing out the buffer for the !dev->addr_len case. This is not to say that what the apps are doing is correct or not, it merely preserves 2.4.x behavior which was changed unintentionally.

I'm going to apply Matt's patch which began this thread.

Matt Domsch

author

2004-11-04 03:42 UTC #15 lore raw

Quoted 7 lines · David S. Miller

Quoted 4 lines · On Tue, 2 Nov 2004 09:06:44 +1100 Herbert Xu <[email protected]>…
Quoted 2 lines · Matt Domsch
Actually, -EOVERFLOW appears throughout the kernel. A couple examples:

Quoted 8 lines

Quoted 1 line
I agree. Please disregard my comment re ERANGE.
I think there is nothing wrong with clearing out the buffer for the !dev->addr_len case. This is not to say that what the apps are doing is correct or not, it merely preserves 2.4.x behavior which was changed unintentionally.
I'm going to apply Matt's patch which began this thread.

If it's all the same to you, I prefer the last version. If you want to remove the net_ratelimited printk, that would be fine too. Either way the apps will work as expected again.

Thanks,
Matt

Signature · 4 lines

Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Herbert Xu

2004-11-01 22:03 UTC #16 lore raw

In reply to jamal

Quoted 4 lines · jamal

My onluy comment is on use of -EOVERFLOW which i have seen only being used in the context of floating point computation (same as -EUNDERFLOW). I dont know what the right thing to return would be.

How about ERANGE? getcwd(3) uses it.

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Herbert Xu

2004-10-30 01:48 UTC #17 lore raw

In reply to Matt Domsch

Quoted 1 line · Matt Domsch

If dev->dev_addr is zero, then the memcpy() never takes place, and the

Huh? You mean dev->addr_len? Surely the caller has to know what the address length is to use this anyway, no?

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt