From owner-svn-src-all@FreeBSD.ORG Mon Nov 21 14:10:13 2011 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id A77BE1065670; Mon, 21 Nov 2011 14:10:13 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from svn.freebsd.org (svn.freebsd.org [IPv6:2001:4f8:fff6::2c]) by mx1.freebsd.org (Postfix) with ESMTP id 9659B8FC15; Mon, 21 Nov 2011 14:10:13 +0000 (UTC) Received: from svn.freebsd.org (localhost [127.0.0.1]) by svn.freebsd.org (8.14.4/8.14.4) with ESMTP id pALEAD0T046141; Mon, 21 Nov 2011 14:10:13 GMT (envelope-from glebius@svn.freebsd.org) Received: (from glebius@localhost) by svn.freebsd.org (8.14.4/8.14.4/Submit) id pALEAD9B046139; Mon, 21 Nov 2011 14:10:13 GMT (envelope-from glebius@svn.freebsd.org) Message-Id: <201111211410.pALEAD9B046139@svn.freebsd.org> From: Gleb Smirnoff Date: Mon, 21 Nov 2011 14:10:13 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cc: Subject: svn commit: r227791 - head/sys/netinet X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 21 Nov 2011 14:10:13 -0000 Author: glebius Date: Mon Nov 21 14:10:13 2011 New Revision: 227791 URL: http://svn.freebsd.org/changeset/base/227791 Log: Historically in_control() did not check sockaddrs supplied with structs ifreq/in_aliasreq and there've been several panics due to that problem. All these panics were fixed just a couple of lines above the panicing code. Take a more general approach: sanity check sockaddrs supplied with SIOCAIFADDR and SIOCSIF*ADDR at the beggining of the function and drop all checks below. One check is now disabled due to strange code in ifconfig(8) that I've removed recently. I'm going to enable it with next __FreeBSD_version bump. Historically in_ifinit() was able to recover from an error and restore old address. Nowadays this feature isn't working for all error cases, but for some of them. I suppose no software relies on this behavior, so I'd like to remove it, since this simplifies code a lot. Also, move if_scrub() earlier in the in_ifinit(). It is more correct to wipe routes before removing address from local address list, and interface address list. Silence from: bz, brooks, andre, rwatson, 3 weeks Modified: head/sys/netinet/in.c Modified: head/sys/netinet/in.c ============================================================================== --- head/sys/netinet/in.c Mon Nov 21 13:40:35 2011 (r227790) +++ head/sys/netinet/in.c Mon Nov 21 14:10:13 2011 (r227791) @@ -234,16 +234,42 @@ in_control(struct socket *so, u_long cmd * in_lifaddr_ioctl() and ifp->if_ioctl(). */ switch (cmd) { - case SIOCAIFADDR: - case SIOCDIFADDR: case SIOCGIFADDR: case SIOCGIFBRDADDR: case SIOCGIFDSTADDR: case SIOCGIFNETMASK: + case SIOCDIFADDR: + break; + case SIOCAIFADDR: + /* + * ifra_addr must be present and be of INET family. + * ifra_broadaddr and ifra_mask are optional. + */ + if (ifra->ifra_addr.sin_len != sizeof(struct sockaddr_in) || + ifra->ifra_addr.sin_family != AF_INET) + return (EINVAL); + if (ifra->ifra_broadaddr.sin_len != 0 && + (ifra->ifra_broadaddr.sin_len != sizeof(struct sockaddr_in) || + ifra->ifra_broadaddr.sin_family != AF_INET)) + return (EINVAL); +#if 0 + /* + * ifconfig(8) historically doesn't set af_family for mask + * for unknown reason. + */ + if (ifra->ifra_mask.sin_len != 0 && + (ifra->ifra_mask.sin_len != sizeof(struct sockaddr_in) || + ifra->ifra_mask.sin_family != AF_INET)) + return (EINVAL); +#endif + break; case SIOCSIFADDR: case SIOCSIFBRDADDR: case SIOCSIFDSTADDR: case SIOCSIFNETMASK: + if (ifr->ifr_addr.sa_family != AF_INET || + ifr->ifr_addr.sa_len != sizeof(struct sockaddr_in)) + return (EINVAL); break; case SIOCALIFADDR: @@ -349,7 +375,7 @@ in_control(struct socket *so, u_long cmd switch (cmd) { case SIOCAIFADDR: case SIOCDIFADDR: - if (ifra->ifra_addr.sin_family == AF_INET) { + { struct in_ifaddr *oia; IN_IFADDR_RLOCK(); @@ -506,7 +532,7 @@ in_control(struct socket *so, u_long cmd goto out; case SIOCSIFNETMASK: - ia->ia_sockmask.sin_addr = ifra->ifra_addr.sin_addr; + ia->ia_sockmask = *(struct sockaddr_in *)&ifr->ifr_addr; ia->ia_subnetmask = ntohl(ia->ia_sockmask.sin_addr.s_addr); goto out; @@ -514,14 +540,12 @@ in_control(struct socket *so, u_long cmd maskIsNew = 0; hostIsNew = 1; error = 0; - if (ia->ia_addr.sin_family == AF_INET) { - if (ifra->ifra_addr.sin_len == 0) { - ifra->ifra_addr = ia->ia_addr; - hostIsNew = 0; - } else if (ifra->ifra_addr.sin_addr.s_addr == - ia->ia_addr.sin_addr.s_addr) - hostIsNew = 0; - } + if (ifra->ifra_addr.sin_len == 0) { + ifra->ifra_addr = ia->ia_addr; + hostIsNew = 0; + } else if (ifra->ifra_addr.sin_addr.s_addr == + ia->ia_addr.sin_addr.s_addr) + hostIsNew = 0; if (ifra->ifra_mask.sin_len) { /* * QL: XXX @@ -552,7 +576,7 @@ in_control(struct socket *so, u_long cmd break; if ((ifp->if_flags & IFF_BROADCAST) && - (ifra->ifra_broadaddr.sin_family == AF_INET)) + ifra->ifra_broadaddr.sin_len) ia->ia_broadaddr = ifra->ifra_broadaddr; if (error == 0) { ii = ((struct in_ifinfo *)ifp->if_afdata[AF_INET]); @@ -608,31 +632,26 @@ in_control(struct socket *so, u_long cmd IN_IFADDR_WLOCK(); TAILQ_REMOVE(&V_in_ifaddrhead, ia, ia_link); - if (ia->ia_addr.sin_family == AF_INET) { - struct in_ifaddr *if_ia; - LIST_REMOVE(ia, ia_hash); - IN_IFADDR_WUNLOCK(); - /* - * If this is the last IPv4 address configured on this - * interface, leave the all-hosts group. - * No state-change report need be transmitted. - */ - if_ia = NULL; - IFP_TO_IA(ifp, if_ia); - if (if_ia == NULL) { - ii = ((struct in_ifinfo *)ifp->if_afdata[AF_INET]); - IN_MULTI_LOCK(); - if (ii->ii_allhosts) { - (void)in_leavegroup_locked(ii->ii_allhosts, - NULL); - ii->ii_allhosts = NULL; - } - IN_MULTI_UNLOCK(); - } else - ifa_free(&if_ia->ia_ifa); + LIST_REMOVE(ia, ia_hash); + IN_IFADDR_WUNLOCK(); + /* + * If this is the last IPv4 address configured on this + * interface, leave the all-hosts group. + * No state-change report need be transmitted. + */ + IFP_TO_IA(ifp, iap); + if (iap == NULL) { + ii = ((struct in_ifinfo *)ifp->if_afdata[AF_INET]); + IN_MULTI_LOCK(); + if (ii->ii_allhosts) { + (void)in_leavegroup_locked(ii->ii_allhosts, NULL); + ii->ii_allhosts = NULL; + } + IN_MULTI_UNLOCK(); } else - IN_IFADDR_WUNLOCK(); + ifa_free(&iap->ia_ifa); + ifa_free(&ia->ia_ifa); /* in_ifaddrhead */ out: if (ia != NULL) @@ -828,50 +847,29 @@ in_ifinit(struct ifnet *ifp, struct in_i int scrub) { register u_long i = ntohl(sin->sin_addr.s_addr); - struct sockaddr_in oldaddr; int flags = RTF_UP, error = 0; - oldaddr = ia->ia_addr; - if (oldaddr.sin_family == AF_INET) + if (scrub) + in_scrubprefix(ia, LLE_STATIC); + + IN_IFADDR_WLOCK(); + if (ia->ia_addr.sin_family == AF_INET) LIST_REMOVE(ia, ia_hash); ia->ia_addr = *sin; - if (ia->ia_addr.sin_family == AF_INET) { - IN_IFADDR_WLOCK(); - LIST_INSERT_HEAD(INADDR_HASH(ia->ia_addr.sin_addr.s_addr), - ia, ia_hash); - IN_IFADDR_WUNLOCK(); - } + LIST_INSERT_HEAD(INADDR_HASH(ia->ia_addr.sin_addr.s_addr), + ia, ia_hash); + IN_IFADDR_WUNLOCK(); + /* * Give the interface a chance to initialize * if this is its first address, * and to validate the address if necessary. */ - if (ifp->if_ioctl != NULL) { - error = (*ifp->if_ioctl)(ifp, SIOCSIFADDR, (caddr_t)ia); - if (error) { + if (ifp->if_ioctl != NULL && + (error = (*ifp->if_ioctl)(ifp, SIOCSIFADDR, (caddr_t)ia)) != 0) /* LIST_REMOVE(ia, ia_hash) is done in in_control */ - ia->ia_addr = oldaddr; - IN_IFADDR_WLOCK(); - if (ia->ia_addr.sin_family == AF_INET) - LIST_INSERT_HEAD(INADDR_HASH( - ia->ia_addr.sin_addr.s_addr), ia, ia_hash); - else - /* - * If oldaddr family is not AF_INET (e.g. - * interface has been just created) in_control - * does not call LIST_REMOVE, and we end up - * with bogus ia entries in hash - */ - LIST_REMOVE(ia, ia_hash); - IN_IFADDR_WUNLOCK(); return (error); - } - } - if (scrub) { - ia->ia_ifa.ifa_addr = (struct sockaddr *)&oldaddr; - in_ifscrub(ifp, ia, LLE_STATIC); - ia->ia_ifa.ifa_addr = (struct sockaddr *)&ia->ia_addr; - } + /* * Be compatible with network classes, if netmask isn't supplied, * guess it based on classes. @@ -916,11 +914,9 @@ in_ifinit(struct ifnet *ifp, struct in_i if (ia->ia_addr.sin_addr.s_addr == INADDR_ANY) return (0); - if (ifp->if_flags & IFF_POINTOPOINT) { - if (ia->ia_dstaddr.sin_addr.s_addr == ia->ia_addr.sin_addr.s_addr) + if (ifp->if_flags & IFF_POINTOPOINT && + ia->ia_dstaddr.sin_addr.s_addr == ia->ia_addr.sin_addr.s_addr) return (0); - } - /* * add a loopback route to self