Fix the following bugs:

- In ifc_name2unit(), disallow leading zeroes in a unit.

  Exploit: ifconfig lo01 create

- In ifc_name2unit(), properly handle overflows.  Otherwise,
  either of two local panic()'s can occur, either because
  no interface with such a name could be found after it was
  successfully created, or because the code will bogusly
  assume that it's a wildcard (unit < 0 due to overflow).

  Exploit: ifconfig lo<overflowed_integer> create

- Previous revision made the following sequence trigger
  a KASSERT() failure in queue(3):

  Exploit: ifconfig lo0 destroy; ifconfig lo0 destroy

  This is because IFC_IFLIST_REMOVE() is always called
  before ifc->ifc_destroy() has been run, not accounting
  for the fact that the latter can fail and leave the
  interface operating (like is the case for "lo0").
  So we ended up calling LIST_REMOVE() twice.  We cannot
  defer IFC_IFLIST_REMOVE() until after a call to
  ifc->ifc_destroy() because the ifnet may have been
  removed and its memory has been freed, so recover from
  this by re-inserting the ifnet in the cloned interfaces
  list if ifc->ifc_destroy() indicates a failure.

1 files changed, 20 insertions, 5 deletions

diff --git a/sys/net/if_clone.c b/sys/net/if_clone.c
index 859ece2..89d679d 100644
--- a/sys/net/if_clone.c
+++ b/sys/net/if_clone.c
@@ -32,6 +32,7 @@
 
 #include <sys/param.h>
 #include <sys/malloc.h>
+#include <sys/limits.h>
 #include <sys/lock.h>
 #include <sys/mutex.h>
 #include <sys/kernel.h>
@@ -200,17 +201,23 @@ if_clone_destroyif(struct if_clone *ifc, struct ifnet *ifp)
 {
 	int err;
 
-	IF_CLONE_LOCK(ifc);
-	IFC_IFLIST_REMOVE(ifc, ifp);
-	IF_CLONE_UNLOCK(ifc);
-
 	if (ifc->ifc_destroy == NULL) {
 		err = EOPNOTSUPP;
 		goto done;
 	}
 
+	IF_CLONE_LOCK(ifc);
+	IFC_IFLIST_REMOVE(ifc, ifp);
+	IF_CLONE_UNLOCK(ifc);
+
 	err =  (*ifc->ifc_destroy)(ifc, ifp);
 
+	if (err != 0) {
+		IF_CLONE_LOCK(ifc);
+		IFC_IFLIST_INSERT(ifc, ifp);
+		IF_CLONE_UNLOCK(ifc);
+	}
+
 done:
 	return (err);
 }
@@ -349,16 +356,24 @@ int
 ifc_name2unit(const char *name, int *unit)
 {
 	const char	*cp;
+	int		cutoff = INT_MAX / 10;
+	int		cutlim = INT_MAX % 10;
 
 	for (cp = name; *cp != '\0' && (*cp < '0' || *cp > '9'); cp++);
 	if (*cp == '\0') {
 		*unit = -1;
+	} else if (cp[0] == '0' && cp[1] != '\0') {
+		/* Disallow leading zeroes. */
+		return (EINVAL);
 	} else {
 		for (*unit = 0; *cp != '\0'; cp++) {
 			if (*cp < '0' || *cp > '9') {
 				/* Bogus unit number. */
 				return (EINVAL);
 			}
+			if (*unit > cutoff ||
+			    (*unit == cutoff && *cp - '0' > cutlim))
+				return (EINVAL);
 			*unit = (*unit * 10) + (*cp - '0');
 		}
 	}
@@ -447,7 +462,7 @@ ifc_simple_attach(struct if_clone *ifc)
 	struct ifc_simple_data *ifcs = ifc->ifc_data;
 
 	KASSERT(ifcs->ifcs_minifs - 1 <= ifc->ifc_maxunit,
-	    ("%s: %s requested more units then allowed (%d > %d)",
+	    ("%s: %s requested more units than allowed (%d > %d)",
 	    __func__, ifc->ifc_name, ifcs->ifcs_minifs,
 	    ifc->ifc_maxunit + 1));