SIOCGIFCONF is hard! -- NetBSD added sockaddr_storage, fallout and fix

[Greg Troxel]

Recently, NetBSD changed struct ifreq to add struct sockaddr_storage in
ifr_ifru alongside the struct sockaddr members, in order to avoid
overflows in other ioctls.  We then sorted out the kernel implementation
of SIOCGIFCONF.

There is an ambiguity in the 4.4BSD definition of SIOCGIFCONF.  If a
sockaddr fit in struct sockaddr (and hence in struct ifreq) then the
next ifreq followed at sizeof(struct ifreq) beyond the current one.  In
the case of sockaddrs that are too large to fit (sockaddr_dl,
sockaddr_in6), the next ifreq started at the end of the actual sockaddr
that was present, as determined by sa_len.  The ambiguity is whether the
rule is about a sockaddr being bigger than struct sockaddr, or than the
alloted space in ifr_ifru in ifreq following ifr_name.  (If anyone is
aware of an actual interface definition, please point me to it.)  In the
4.4BSD code, this is a distinction without an API/ABI difference.

One way to interpret the old rule is to say that the size test should be
about whether the struct sockaddr_foo fits inside struct ifreq.  The
other is to compare to struct sockaddr, and this has the bizarre effect
of having the next struct ifreq start earlier for _dl and _in6, but at
the full ifreq increment for _in.  I have thus chosen the 'fits in
union' approach for NetBSD-current.

The only other program we've found so far that uses SIOCGIFCONF was
racoon, and it was using the 'fits in ifreq' interpretation.

The DHCP code was checking against struct sockaddr, and thus dhclient
and dhcpd both failed on NetBSD-current once the SIOCGIFCONF kernel
implementation was fixed.  The following patch adds comments questioning
some of the hairier parts of SIOCGIFCONF usage, and does the size check
against the union.  The existing code refers to the union already, so no
new autoconf test is needed.

I would like to hear from maintainers of the other BSDs, Linux, Solaris,
etc. about two points:

1) Have you changed ifreq to have sockaddr_storage?  If not, what is
your interpretation of the rule for when ifreq doesn't follow naturally?

2) Do you implement a way to return the needed length for SIOCGIFCONF?
What happens with a non-NULL non-zero buffer that isn't big enough?  On
NetBSD, ifreqs are added as long as they fit.  4.4BSD doesn't seem to
have support for obtaining the length, so I wonder if that was added
differently in various places.

Probably dhcp should grow support for getifaddrs (in BSD/OS, now in
NetBSD), and use thatb preferentially, and SIOCGIFCONF should be
relegated to historic status.  It's just too hard to get right.

Index: discover.c
===================================================================
RCS file: /cvsroot/src/dist/dhcp/common/discover.c,v
retrieving revision 1.9
retrieving revision 1.10
diff -u -p -u -p -r1.9 -r1.10
--- discover.c	31 May 2007 02:58:10 -0000	1.9
+++ discover.c	13 Sep 2007 11:56:41 -0000	1.10
@@ -34,7 +34,7 @@
 
 #ifndef lint
 static char copyright[] =
-"$Id: discover.c,v 1.9 2007/05/31 02:58:10 christos Exp $ Copyright (c) 2004-2005 Internet Systems Consortium.  All rights reserved.\n";
+"$Id: discover.c,v 1.10 2007/09/13 11:56:41 gdt Exp $ Copyright (c) 2004-2005 Internet Systems Consortium.  All rights reserved.\n";
 #endif /* not lint */
 
 #include "dhcpd.h"
@@ -196,6 +196,14 @@ void discover_interfaces (state)
 
 	/* If the SIOCGIFCONF resulted in more data than would fit in
 	   a buffer, allocate a bigger buffer. */
+	/*
+	 * XXX This code is broken on NetBSD; the return value is the
+	 * amount of data returned, not the amount that would be
+	 * needed.  We need to e.g. double the buffer and try again.
+	 * The proper test is hairier: we can be sure that we got
+	 * everything only if there is an entire struct ifreq worth of
+	 * space left.
+	 */
 	if ((ic.ifc_ifcu.ifcu_buf == buf 
 #ifdef SIOCGIFCONF_ZERO_PROBE
 	     || ic.ifc_ifcu.ifcu_buf == 0
@@ -235,10 +243,23 @@ void discover_interfaces (state)
 
 		memcpy(&ifcpy, (caddr_t)ic.ifc_req + i, sizeof(struct ifreq));
 #ifdef HAVE_SA_LEN
-		if (ifp -> ifr_addr.sa_len > sizeof (struct sockaddr)) {
+		/*
+		 * Classically, struct ifreq used with SIOCGIFCONF had
+		 * a union where struct sockaddr was the largest
+		 * member.  If the address fit in the union, the next
+		 * ifreq followed immediately.  If not, the next ifreq
+		 * followed the end of the actual sockaddr.  In
+		 * NetBSD-current after ~2007-05, ifreq has a
+		 * sockaddr_storage member, and the next ifreq follows
+		 * the current ifreq always, because no sockaddr can
+		 * be bigger than sockaddr_storage.  Thus, compare the
+		 * length to the union's size, not struct sockaddr.
+		 */
+		if (ifp -> ifr_addr.sa_len > sizeof (ifp->ifr_ifru)) {
 			if (sizeof(struct ifreq) + ifp->ifr_addr.sa_len >
 			    sizeof(ifcpy))
 				break;
+			/* XXX This copies IFNAMSIZ bytes too many. */ 
 			memcpy(&ifcpy, (caddr_t)ic.ifc_req + i, 
 			    sizeof(struct ifreq) + ifp->ifr_addr.sa_len);
 			i += offsetof(struct ifreq, ifr_ifru) +



From: Greg Troxel <gdt at netbsd.org>
Subject: CVS commit: src/dist/dhcp/common
To: source-changes at NetBSD.org
Date: Thu, 13 Sep 2007 11:56:42 +0000 (UTC)
Reply-To: gdt at netbsd.org


Module Name:	src
Committed By:	gdt
Date:		Thu Sep 13 11:56:41 UTC 2007

Modified Files:
	src/dist/dhcp/common: discover.c

Log Message:
Follow NetBSD's interpretation of the interface to SIOCGIFCONF: the
next ifreq is sizeof(struct ifreq) after the current one unless the
sockaddr is bigger than the union in ifreq that holds it.

In the original 4.4BSD code, this interpretation results in the same
behavior as the "is the sockaddr bigger than struct sockaddr", because
sizeof(struct sockaddr) and sizeof(ifc->ifr_ifru) are the same.

Add comments pointing out problems in the 'need bigger buffer' code,
and copying excessive amounts of data.


To generate a diff of this commit:
cvs rdiff -r1.9 -r1.10 src/dist/dhcp/common/discover.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.