[PATCH FOR REVIEW] Fix SIOCGIFDESCR when buffer is too small

[Xin LI]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Here is a patch that addressed the issue, where when SIOCGIFDESCR is fed
with a smaller buffer.  As reported by Bernhard, this would cause an
infinite loop in ifconfig(8).

The previous implementation claims that the 'length' field would be set
to the number of length returned, and an error is returned.  However,
our ioctl(2) system call will not do copyout if there is errno being
set, as discussed on -arch@ and thus the API needs to be tweaked.

To minimize impact on ABI I have choose to use buffer as an indicator
that the buffer length from userland is not sufficient, instead of
returning ENAMETOOLONG.

I'll also submit a patch for libpcap if this proposed change is
considered be a good one.  The libpcap in contrib/libpcap is not
affected since it doesn't support dynamic length description.

Cheers,
- -- 
Xin LI <delphij at delphij.net>	http://www.delphij.net/
FreeBSD - The Power to Serve!	       Live free or die
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.14 (FreeBSD)

iQEcBAEBAgAGBQJLxMXWAAoJEATO+BI/yjfBWc4H/jO7i2Rm+GqeYXX2eNWUjE2W
5dpNFq0kxqQWpLTr8qPskQ7o/ZDIl8ASbNJPdr/G+U1mYGVwNWVa6z0TR3huZZCB
gPnR+84a+C/8rwtJjhOuyFKt/fdZfD4kI+rnWB+9Cq/uLX4aqziY1YO7SIAtb/1b
RrjyM6rgYsMcnrqJKrmAQQEU1k6Yqkcy5PEEzU6MTSsHYL4wuKujZzmIYdZRg4rI
OLSdLQEWq+u4PuOnrRMrvrrZZCObOURCWpjnJiP1yyMBE/ZW6itfMp6BE6k29vUz
vZcDtqUFj3j1tVvaA4MzuX+isMUqnO8DvcnIawjwefs9Rq0mWY796kGSEjZYxuQ=
=lyPJ
-----END PGP SIGNATURE-----
-------------- next part --------------
Index: sbin/ifconfig/ifconfig.c
===================================================================
--- sbin/ifconfig/ifconfig.c	(revision 206558)
+++ sbin/ifconfig/ifconfig.c	(working copy)
@@ -922,19 +922,21 @@
 			ifr.ifr_buffer.buffer = descr;
 			ifr.ifr_buffer.length = descrlen;
 			if (ioctl(s, SIOCGIFDESCR, &ifr) == 0) {
-				if (strlen(descr) > 0)
-					printf("\tdescription: %s\n", descr);
-				break;
-			} else if (errno == ENAMETOOLONG)
-				descrlen = ifr.ifr_buffer.length;
-			else
-				break;
-		} else {
+				if (ifr.ifr_buffer.buffer == descr) {
+					if (strlen(descr) > 0)
+						printf("\tdescription: %s\n",
+						    descr);
+					break;
+				} else if (ifr.ifr_buffer.length > descrlen) {
+					descrlen = ifr.ifr_buffer.length;
+					continue;
+				}
+			}
+		} else
 			warn("unable to allocate memory for interface"
 			    "description");
-			break;
-		}
-	};
+		break;
+	}
 
 	if (ioctl(s, SIOCGIFCAP, (caddr_t)&ifr) == 0) {
 		if (ifr.ifr_curcap != 0) {
Index: share/man/man4/netintro.4
===================================================================
--- share/man/man4/netintro.4	(revision 206558)
+++ share/man/man4/netintro.4	(working copy)
@@ -292,8 +292,11 @@
 struct passed in as parameter, and the length would include
 the terminating nul character.
 If there is not enough space to hold the interface length,
-no copy would be done and an
-error would be returned.
+no copy would be done and the
+.Va buffer
+field of
+.Va ifru_buffer
+would be set to NULL.
 The kernel will store the buffer length in the
 .Va length
 field upon return, regardless whether the buffer itself is
Index: sys/net/if.c
===================================================================
--- sys/net/if.c	(revision 206558)
+++ sys/net/if.c	(working copy)
@@ -2049,14 +2049,13 @@
 	case SIOCGIFDESCR:
 		error = 0;
 		sx_slock(&ifdescr_sx);
-		if (ifp->if_description == NULL) {
-			ifr->ifr_buffer.length = 0;
+		if (ifp->if_description == NULL)
 			error = ENOMSG;
-		} else {
+		else {
 			/* space for terminating nul */
 			descrlen = strlen(ifp->if_description) + 1;
 			if (ifr->ifr_buffer.length < descrlen)
-				error = ENAMETOOLONG;
+				ifr->ifr_buffer.buffer = NULL;
 			else
 				error = copyout(ifp->if_description,
 				    ifr->ifr_buffer.buffer, descrlen);