Re: 5.3-RELEASE TODO

On Fri, Sep 17, 2004 at 01:41:16AM -0600, Scott Long wrote:
>  |--------------------+-----------+-------------+-------------------------|
>  |                    |           |             |The ifconf() ioctl for   |
>  |                    |           |             |listing network          |
>  |                    |           |             |interfaces performs a    |
>  |                    |           |             |copyout() while holding  |
>  |ifconf() sleep      |           |             |the global ifnet list    |
>  |warning             |In progress|Robert Watson|mutex. This generates a  |
>  |                    |           |             |witness warning in the   |
>  |                    |           |             |event that copyout()     |
>  |                    |           |             |generates a page fault,  |
>  |                    |           |             |and risks more serious   |
>  |                    |           |             |problems.                |
>  +------------------------------------------------------------------------+

I've got a patch for this one.  If someone will review it, I will commit
it to HEAD for further testing.

-- Brooks

http://perforce.freebsd.org/chv.cgi?CH=61588

Change 61588 by brooks_at_brooks_minya on 2004/09/16 05:07:10

	Fix a potential LOR in ifconf caused by using copyout while
	holding a lock.
	
	Reported by:	rwatson

Affected files ...

.. //depot/user/brooks/cleanup/sys/net/if.c#38 edit

Differences ...

==== //depot/user/brooks/cleanup/sys/net/if.c#38 (text+ko) ====

_at__at_ -123,6 +123,7 _at__at_
 SYSINIT(interfaces, SI_SUB_INIT_IF, SI_ORDER_FIRST, if_init, NULL)
 SYSINIT(interface_check, SI_SUB_PROTO_IF, SI_ORDER_FIRST, if_check, NULL)
 
+MALLOC_DEFINE(M_IFCONF, "ifconf", "interface configuration info");
 MALLOC_DEFINE(M_IFADDR, "ifaddr", "interface address");
 MALLOC_DEFINE(M_IFMADDR, "ether_multi", "link-level multicast address");
 
_at__at_ -1484,10 +1485,27 _at__at_
 	struct ifconf *ifc = (struct ifconf *)data;
 	struct ifnet *ifp;
 	struct ifaddr *ifa;
-	struct ifreq ifr, *ifrp;
+	struct ifreq ifr, *ifrp, *ifrbuf = NULL;
 	int space = ifc->ifc_len, error = 0;
+	size_t buflen = 0;
 
-	ifrp = ifc->ifc_req;
+	/*
+	 * Because it is not safe to do a copyout while a lock it held,
+	 * we need to avoid doing a copyout while walking the interface
+	 * list.  The easiest way to do that is to stage all the data
+	 * into a single buffer rather then doing many copyouts.
+	 * Unfortunatly, we can't just trust the users buffer length
+	 * because that might cause use to allocate too much kernel
+	 * memory.  Thus we need to bound the memory by allocate by the 
+	 * actual space needed.  In order to do that with the minimum
+	 * duplication of code, we make two passes through the loop over
+	 * interfaces and addresses.  In the first pass, we have no
+	 * buffer and we count how much space we needed and malloc it
+	 * after releasing the lock.  Then we go through again and
+	 * actually fill the buffer followed by copying it out.
+	 */
+again:
+	ifrp = ifrbuf;
 	IFNET_RLOCK();		/* could sleep XXX */
 	TAILQ_FOREACH(ifp, &ifnet, if_link) {
 		int addrs;
_at__at_ -1516,47 +1534,68 _at__at_
 					 (struct osockaddr *)&ifr.ifr_addr;
 				ifr.ifr_addr = *sa;
 				osa->sa_family = sa->sa_family;
-				error = copyout((caddr_t)&ifr, (caddr_t)ifrp,
-						sizeof (ifr));
-				ifrp++;
+				if (ifrp == NULL) {
+					buflen += sizeof(ifr);
+				} else {
+					bcopy(&ifr, ifrp, sizeof(ifr));
+					ifrp++;
+				}
+					
 			} else
 #endif
 			if (sa->sa_len <= sizeof(*sa)) {
 				ifr.ifr_addr = *sa;
-				error = copyout((caddr_t)&ifr, (caddr_t)ifrp,
-						sizeof (ifr));
-				ifrp++;
+				if (ifrp == NULL) {
+					buflen += sizeof(struct ifreq);
+				} else {
+					bcopy(&ifr, ifrp, sizeof(ifr));
+					ifrp++;
+				}
 			} else {
 				if (space < sizeof (ifr) + sa->sa_len -
 					    sizeof(*sa))
 					break;
 				space -= sa->sa_len - sizeof(*sa);
-				error = copyout((caddr_t)&ifr, (caddr_t)ifrp,
-						sizeof (ifr.ifr_name));
-				if (error == 0)
-				    error = copyout((caddr_t)sa,
-				      (caddr_t)&ifrp->ifr_addr, sa->sa_len);
-				ifrp = (struct ifreq *)
-					(sa->sa_len + (caddr_t)&ifrp->ifr_addr);
+				if (ifrp == NULL) {
+					buflen +=
+					    offsetof(struct ifreq, ifr_addr) +
+					    sa->sa_len;
+				} else {
+					bcopy(&ifr, ifrp,
+					    sizeof(ifr.ifr_name));
+					bcopy(sa, &ifrp->ifr_addr, sa->sa_len);
+					ifrp = (struct ifreq *)
+					    (sa->sa_len +
+					    (caddr_t)&ifrp->ifr_addr);
+				}
 			}
-			if (error)
-				break;
 			space -= sizeof (ifr);
 		}
-		if (error)
-			break;
-		if (!addrs) {
+		if (addrs == 0) {
 			bzero((caddr_t)&ifr.ifr_addr, sizeof(ifr.ifr_addr));
-			error = copyout((caddr_t)&ifr, (caddr_t)ifrp,
-			    sizeof (ifr));
-			if (error)
-				break;
+			if (ifrp == NULL) {
+				buflen += sizeof(struct ifreq);
+			} else {
+				bcopy(&ifr, ifrp, sizeof(ifr));
+				ifrp++;
+			}
 			space -= sizeof (ifr);
-			ifrp++;
 		}
 	}
 	IFNET_RUNLOCK();
-	ifc->ifc_len -= space;
+
+	if (error != 0)
+		return (error);
+
+	if (ifrp == NULL) {
+		ifrbuf = malloc(buflen, M_IFCONF, M_ZERO | M_NOWAIT);
+		space = buflen;
+		goto again;
+	}
+
+	ifc->ifc_len = buflen - space;
+	error = copyout(ifrbuf, ifc->ifc_req, ifc->ifc_len);
+	free(ifrbuf, M_IFCONF);
 	return (error);
 }
 

-- 
Any statement of the form "X is the one, true Y" is FALSE.
PGP fingerprint 655D 519C 26A7 82E7 2529  9BF0 5D8E 8BE9 F238 1AD4