Do propper kernel input validation for in_control() ioctl(2)

SIOCGIFADDR, SIOCGIFNETMASK, SIOCGIFDSTADDR, SIOCGIFBRDADDR,
SIOCSIFADDR, SIOCSIFNETMASK, SIOCSIFDSTADDR, and SIOCSIFBRDADDR.
Name in_ioctl_set_ifaddr() consistently.  Use in_sa2sin() to validate
inet address.  Combine if_addrlist loops and add comment.  Although
netmask is not a inet address, length must be valid.
Reported-by: syzbot+5fc6da002fc4e8d994be@syzkaller.appspotmail.com
OK visa@

1 files changed, 63 insertions, 40 deletions

diff --git a/sys/netinet/in.c b/sys/netinet/in.c
index 9263a59df8a..a9a56e2b325 100644
--- a/sys/netinet/in.c
+++ b/sys/netinet/in.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: in.c,v 1.164 2019/10/23 19:58:32 bluhm Exp $	*/
+/*	$OpenBSD: in.c,v 1.165 2019/11/07 13:25:44 bluhm Exp $	*/
 /*	$NetBSD: in.c,v 1.26 1996/02/13 23:41:39 christos Exp $	*/
 
 /*
@@ -84,7 +84,7 @@
 
 void in_socktrim(struct sockaddr_in *);
 
-int in_ioctl_sifaddr(u_long, caddr_t, struct ifnet *, int);
+int in_ioctl_set_ifaddr(u_long, caddr_t, struct ifnet *, int);
 int in_ioctl_change_ifaddr(u_long, caddr_t, struct ifnet *, int);
 int in_ioctl_get(u_long, caddr_t, struct ifnet *);
 void in_purgeaddr(struct ifaddr *);
@@ -227,7 +227,7 @@ in_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, int privileged)
 	struct ifreq *ifr = (struct ifreq *)data;
 	struct ifaddr *ifa;
 	struct in_ifaddr *ia = NULL;
-	struct sockaddr_in oldaddr;
+	struct sockaddr_in *sin = NULL, oldaddr;
 	int error = 0;
 
 	if (ifp == NULL)
@@ -240,7 +240,7 @@ in_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, int privileged)
 	case SIOCGIFBRDADDR:
 		return in_ioctl_get(cmd, data, ifp);
 	case SIOCSIFADDR:
-		return in_ioctl_sifaddr(cmd, data, ifp, privileged);
+		return in_ioctl_set_ifaddr(cmd, data, ifp, privileged);
 	case SIOCAIFADDR:
 	case SIOCDIFADDR:
 		return in_ioctl_change_ifaddr(cmd, data, ifp, privileged);
@@ -252,28 +252,31 @@ in_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, int privileged)
 		return (EOPNOTSUPP);
 	}
 
+	if (ifr->ifr_addr.sa_family == AF_INET) {
+		error = in_sa2sin(&ifr->ifr_addr, &sin);
+		if (error)
+			return (error);
+	}
+
 	NET_LOCK();
 
 	TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
-		if (ifa->ifa_addr->sa_family == AF_INET) {
+		if (ifa->ifa_addr->sa_family != AF_INET)
+			continue;
+		/* find first address or exact match */
+		if (ia == NULL)
+			ia = ifatoia(ifa);
+		if (sin == NULL || sin->sin_addr.s_addr == INADDR_ANY)
+			break;
+		if (ifatoia(ifa)->ia_addr.sin_addr.s_addr ==
+		    sin->sin_addr.s_addr) {
 			ia = ifatoia(ifa);
 			break;
-		}
-	}
-
-	if (ia && satosin(&ifr->ifr_addr)->sin_addr.s_addr) {
-		for (; ifa != NULL; ifa = TAILQ_NEXT(ifa, ifa_list)) {
-			if ((ifa->ifa_addr->sa_family == AF_INET) &&
-			    ifatoia(ifa)->ia_addr.sin_addr.s_addr ==
-			    satosin(&ifr->ifr_addr)->sin_addr.s_addr) {
-				ia = ifatoia(ifa);
-				break;
-			}
 		}
 	}
 	if (ia == NULL) {
-		NET_UNLOCK();
-		return (EADDRNOTAVAIL);
+		error = EADDRNOTAVAIL;
+		goto err;
 	}
 
 	switch (cmd) {
@@ -287,8 +290,11 @@ in_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, int privileged)
 			error = EINVAL;
 			break;
 		}
+		error = in_sa2sin(&ifr->ifr_dstaddr, &sin);
+		if (error)
+			break;
 		oldaddr = ia->ia_dstaddr;
-		ia->ia_dstaddr = *satosin(&ifr->ifr_dstaddr);
+		ia->ia_dstaddr = *sin;
 		error = (*ifp->if_ioctl)(ifp, SIOCSIFDSTADDR, (caddr_t)ia);
 		if (error) {
 			ia->ia_dstaddr = oldaddr;
@@ -308,7 +314,10 @@ in_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, int privileged)
 			error = EINVAL;
 			break;
 		}
-		ifa_update_broadaddr(ifp, &ia->ia_ifa, &ifr->ifr_broadaddr);
+		error = in_sa2sin(&ifr->ifr_broadaddr, &sin);
+		if (error)
+			break;
+		ifa_update_broadaddr(ifp, &ia->ia_ifa, sintosa(sin));
 		break;
 
 	case SIOCSIFNETMASK:
@@ -317,21 +326,27 @@ in_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, int privileged)
 			break;
 		}
 
+		if (ifr->ifr_addr.sa_len < 8) {
+			error = EINVAL;
+			break;
+		}
 		ia->ia_netmask = ia->ia_sockmask.sin_addr.s_addr =
 		    satosin(&ifr->ifr_addr)->sin_addr.s_addr;
 		break;
 	}
-
+err:
 	NET_UNLOCK();
 	return (error);
 }
 
 int
-in_ioctl_sifaddr(u_long cmd, caddr_t data, struct ifnet *ifp, int privileged)
+in_ioctl_set_ifaddr(u_long cmd, caddr_t data, struct ifnet *ifp,
+    int privileged)
 {
 	struct ifreq *ifr = (struct ifreq *)data;
 	struct ifaddr *ifa;
 	struct in_ifaddr *ia = NULL;
+	struct sockaddr_in *sin;
 	int error = 0;
 	int newifaddr;
 
@@ -341,15 +356,19 @@ in_ioctl_sifaddr(u_long cmd, caddr_t data, struct ifnet *ifp, int privileged)
 	if (!privileged)
 		return (EPERM);
 
+	error = in_sa2sin(&ifr->ifr_addr, &sin);
+	if (error)
+		return (error);
+
 	NET_LOCK();
 
 	TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
-		if (ifa->ifa_addr->sa_family == AF_INET) {
-			ia = ifatoia(ifa);
-			break;
-		}
+		if (ifa->ifa_addr->sa_family != AF_INET)
+			continue;
+		/* find first address */
+		ia = ifatoia(ifa);
+		break;
 	}
-
 	if (ia == NULL) {
 		ia = malloc(sizeof *ia, M_IFADDR, M_WAITOK | M_ZERO);
 		ia->ia_addr.sin_family = AF_INET;
@@ -369,7 +388,7 @@ in_ioctl_sifaddr(u_long cmd, caddr_t data, struct ifnet *ifp, int privileged)
 		newifaddr = 0;
 
 	in_ifscrub(ifp, ia);
-	error = in_ifinit(ifp, ia, satosin(&ifr->ifr_addr), newifaddr);
+	error = in_ifinit(ifp, ia, sin, newifaddr);
 	if (!error)
 		dohooks(ifp->if_addrhooks, 0);
 
@@ -521,25 +540,29 @@ in_ioctl_get(u_long cmd, caddr_t data, struct ifnet *ifp)
 	struct ifreq *ifr = (struct ifreq *)data;
 	struct ifaddr *ifa;
 	struct in_ifaddr *ia = NULL;
+	struct sockaddr_in *sin = NULL;
 	int error = 0;
 
+	if (ifr->ifr_addr.sa_family == AF_INET) {
+		error = in_sa2sin(&ifr->ifr_addr, &sin);
+		if (error)
+			return (error);
+	}
+
 	NET_RLOCK();
 
 	TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
-		if (ifa->ifa_addr->sa_family == AF_INET) {
+		if (ifa->ifa_addr->sa_family != AF_INET)
+			continue;
+		/* find first address or exact match */
+		if (ia == NULL)
+			ia = ifatoia(ifa);
+		if (sin == NULL || sin->sin_addr.s_addr == INADDR_ANY)
+			break;
+		if (ifatoia(ifa)->ia_addr.sin_addr.s_addr ==
+		    sin->sin_addr.s_addr) {
 			ia = ifatoia(ifa);
 			break;
-		}
-	}
-
-	if (ia && satosin(&ifr->ifr_addr)->sin_addr.s_addr) {
-		for (; ifa != NULL; ifa = TAILQ_NEXT(ifa, ifa_list)) {
-			if ((ifa->ifa_addr->sa_family == AF_INET) &&
-			    ifatoia(ifa)->ia_addr.sin_addr.s_addr ==
-			    satosin(&ifr->ifr_addr)->sin_addr.s_addr) {
-				ia = ifatoia(ifa);
-				break;
-			}
 		}
 	}
 	if (ia == NULL) {