Re: 5.3-RELEASE TODO

On Fri, Sep 17, 2004 at 09:30:19PM +0200, Dag-Erling Smørgrav wrote:
> Brooks Davis <brooks_at_one-eyed-alien.net> writes:
> >                      Thus, so you have to know how much space you will
> > need before doing any kind of allocation, hence the double loop and the
> > potential race.
> 
> Using sbufs removes the need for loop and greatly simplifies how you
> deal with overflows.

Indeed it does.  I'm not fully happy with the hardcoding of a maximum
size, but I doubt anyone will hit it in practice.  Here's a new and
improved patch that makes a single pass and uses sbufs.

-- Brooks


--- /home/brooks/working/freebsd/p4/freebsd/sys/net/if.c	Fri Sep 17 22:11:13 2004
+++ sys/net/if.c	Fri Sep 17 22:13:06 2004
_at__at_ -36,9 +36,11 _at__at_
 #include "opt_mac.h"
 
 #include <sys/param.h>
+#include <sys/types.h>
 #include <sys/conf.h>
 #include <sys/mac.h>
 #include <sys/malloc.h>
+#include <sys/sbuf.h>
 #include <sys/bus.h>
 #include <sys/mbuf.h>
 #include <sys/systm.h>
_at__at_ -1483,28 +1485,26 _at__at_
 	struct ifconf *ifc = (struct ifconf *)data;
 	struct ifnet *ifp;
 	struct ifaddr *ifa;
-	struct ifreq ifr, *ifrp;
-	int space = ifc->ifc_len, error = 0;
+	struct ifreq ifr;
+	struct sbuf *sb;
+
+	/* Limit buffer size to MAXPHYS to avoid DoS from userspace. */
+        sb = sbuf_new(NULL, NULL,
+            (ifc->ifc_len >= MAXPHYS ? MAXPHYS : (ifc->ifc_len + 1)),
+            SBUF_FIXEDLEN);
 
-	ifrp = ifc->ifc_req;
 	IFNET_RLOCK();		/* could sleep XXX */
 	TAILQ_FOREACH(ifp, &ifnet, if_link) {
 		int addrs;
 
-		if (space < sizeof(ifr))
-			break;
 		if (strlcpy(ifr.ifr_name, ifp->if_xname, sizeof(ifr.ifr_name))
-		    >= sizeof(ifr.ifr_name)) {
-			error = ENAMETOOLONG;
-			break;
-		}
+		    >= sizeof(ifr.ifr_name))
+			return (ENAMETOOLONG);
 
 		addrs = 0;
 		TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) {
 			struct sockaddr *sa = ifa->ifa_addr;
 
-			if (space < sizeof(ifr))
-				break;
 			if (jailed(curthread->td_ucred) &&
 			    prison_if(curthread->td_ucred, sa))
 				continue;
_at__at_ -1515,48 +1515,34 _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++;
+				sbuf_bcat(sb, &ifr, sizeof(ifr));
 			} else
 #endif
 			if (sa->sa_len <= sizeof(*sa)) {
 				ifr.ifr_addr = *sa;
-				error = copyout((caddr_t)&ifr, (caddr_t)ifrp,
-						sizeof (ifr));
-				ifrp++;
+				sbuf_bcat(sb, &ifr, sizeof(ifr));
 			} 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);
+				sbuf_bcat(sb, &ifr,
+				    offsetof(struct ifreq, ifr_addr));
+				sbuf_bcat(sb, sa, sa->sa_len);
 			}
-			if (error)
+
+			if (sbuf_overflowed(sb))
 				break;
-			space -= sizeof (ifr);
+			ifc->ifc_len = sbuf_len(sb);
 		}
-		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)
+			sbuf_bcat(sb, &ifr, sizeof(ifr));
+
+			if (sbuf_overflowed(sb))
 				break;
-			space -= sizeof (ifr);
-			ifrp++;
+			ifc->ifc_len = sbuf_len(sb);
 		}
 	}
 	IFNET_RUNLOCK();
-	ifc->ifc_len -= space;
-	return (error);
+
+	return (copyout(sbuf_data(sb), ifc->ifc_req, ifc->ifc_len));
 }
 
 /*

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