[PATCH 2.6] dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

[PATCH 2.6] dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

[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_dev_addr == 0,
clear the buffer before returning success.

Thanks,
Matt

-- 
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;
 

Re: [PATCH 2.6] dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

[Matt Domsch]

On Fri, Oct 29, 2004 at 04:51:34PM -0500, Matt Domsch wrote:
> 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 <Matt_Domsch@dell.com>

-- 
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

Re: [PATCH 2.6] dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

[Herbert Xu]

Matt Domsch <Matt_Domsch@dell.com> wrote:
> 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?
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 2.6] dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

[Herbert Xu]

Matt Domsch <Matt_Domsch@dell.com> wrote:
> 
> 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.
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 2.6] dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

[Matt Domsch]

On Sat, Oct 30, 2004 at 11:51:01AM +1000, Herbert Xu wrote:
> > 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.

> 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

-- 
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

Re: [PATCH 2.6] dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

[jamal]

On Fri, 2004-10-29 at 23:09, Matt Domsch wrote:
> On Sat, Oct 30, 2004 at 11:51:01AM +1000, Herbert Xu wrote:
> > > 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

Re: [PATCH 2.6] dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

[Matt Domsch]

On Sat, Oct 30, 2004 at 03:10:19PM -0400, jamal wrote:
> 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.

> 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

-- 
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

Re: [PATCH 2.6] dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

[Matt Domsch]

On Sun, Oct 31, 2004 at 10:44:33PM -0600, Matt Domsch wrote:
> 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 <Matt_Domsch@dell.com>


-- 
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;
 

Re: [PATCH 2.6] dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

[Herbert Xu]

On Mon, Nov 01, 2004 at 11:34:34AM -0600, Matt Domsch wrote:
>
> +			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.
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 2.6] dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

[Matt Domsch]

On Tue, Nov 02, 2004 at 07:27:54AM +1100, Herbert Xu wrote:
> 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 <Matt_Domsch@dell.com>

-- 
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;

Re: [PATCH 2.6] dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

[Herbert Xu]

On Mon, Nov 01, 2004 at 02:38:21PM -0600, Matt Domsch wrote:
> 
> OK, I'll unconditionally clear it all first.  How's this?

It's nearly there :)

> +			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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 2.6] dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

[Matt Domsch]

On Tue, Nov 02, 2004 at 07:41:31AM +1100, Herbert Xu wrote:
> 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 <Matt_Domsch@dell.com>

-- 
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;
 

Re: [PATCH 2.6] dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

[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.

cheers,
jamal

On Mon, 2004-11-01 at 15:45, Matt Domsch wrote:
> On Tue, Nov 02, 2004 at 07:41:31AM +1100, Herbert Xu wrote:
> > 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 <Matt_Domsch@dell.com>

Re: [PATCH 2.6] dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

[Matt Domsch]

On Mon, Nov 01, 2004 at 04:50:49PM -0500, jamal wrote:
> 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

-- 
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

Re: [PATCH 2.6] dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

[Herbert Xu]

On Mon, Nov 01, 2004 at 04:50:49PM -0500, jamal wrote:
> 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.
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 2.6] dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

[Herbert Xu]

On Mon, Nov 01, 2004 at 03:59:44PM -0600, Matt Domsch wrote:
> 
> Actually, -EOVERFLOW appears throughout the kernel.  A couple examples:

I agree.  Please disregard my comment re ERANGE.
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 2.6] dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

[David S. Miller]

On Tue, 2 Nov 2004 09:06:44 +1100
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Mon, Nov 01, 2004 at 03:59:44PM -0600, Matt Domsch wrote:
> > 
> > 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.

Re: [PATCH 2.6] dev.c: clear SIOCGIFHWADDR buffer if !dev->addr_len

[Matt Domsch]

On Wed, Nov 03, 2004 at 04:07:27PM -0800, David S. Miller wrote:
> On Tue, 2 Nov 2004 09:06:44 +1100
> Herbert Xu <herbert@gondor.apana.org.au> wrote:
> 
> > On Mon, Nov 01, 2004 at 03:59:44PM -0600, Matt Domsch wrote:
> > > 
> > > 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.

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

-- 
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