From 44a87e427eb5c9e1373c7675af4404f8b7788074 Mon Sep 17 00:00:00 2001 From: Sepherosa Ziehau Date: Mon, 26 May 2008 13:29:33 +0000 Subject: [PATCH] Fix following possible bugs for SIOCSIFADDR, if in_ifinit() fails Conditions: o ifaceX has an AF_INET ia o SIOCSIFADDR is used to change address, and new address' hash value is different from ia's o ia is currently in hash bucket B1 o ia is removed from B1 and installed into hash table using new address hash value, assume its new hash bucket is B2, and B1 != B2 1) Dangling ia reference in inaddr hash table o ifnet.if_ioctl fails o ia is reinstalled into hash bucket B1, but without being first removed from hash bucket B2 Hash bucket B2 will have a dangling reference to ia 2) ia is left in wrong hash bucket o rtinit fails o ia's address is restored to oldaddr ia itself is left in hash bucket indexed by new address's hash value - In in_ifinit(), if it fails, unlink ia from inaddr hash table instead of delaying the unlinking to in_control_internal(). If necessary reinstall ia into inaddr hash table with original address - After the above fix, in_control_internal() needs to unlink ia from inaddr only if cmd is SIOCDIFADDR and ia resides in inaddr hash table. Whether ia is in inaddr hash table or not, is currently indicated by ia address's family; add XXX comment that this assumption is not good - Constfy 'sin' parameter to in_ifinit() Reviewed-by: dillon@ --- sys/netinet/in.c | 54 +++++++++++++++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 23 deletions(-) diff --git a/sys/netinet/in.c b/sys/netinet/in.c index 8c7ecd7d99..c29aadf54c 100644 --- a/sys/netinet/in.c +++ b/sys/netinet/in.c @@ -32,7 +32,7 @@ * * @(#)in.c 8.4 (Berkeley) 1/9/95 * $FreeBSD: src/sys/netinet/in.c,v 1.44.2.14 2002/11/08 00:45:50 suz Exp $ - * $DragonFly: src/sys/netinet/in.c,v 1.34 2008/05/24 08:04:42 sephe Exp $ + * $DragonFly: src/sys/netinet/in.c,v 1.35 2008/05/26 13:29:33 sephe Exp $ */ #include "opt_bootp.h" @@ -68,8 +68,8 @@ static int in_lifaddr_ioctl (struct socket *, u_long, caddr_t, struct ifnet *, struct thread *); static void in_socktrim (struct sockaddr_in *); -static int in_ifinit (struct ifnet *, - struct in_ifaddr *, struct sockaddr_in *, int); +static int in_ifinit(struct ifnet *, struct in_ifaddr *, + const struct sockaddr_in *, int); static void in_control_dispatch(struct netmsg *); static int in_control_internal(u_long, caddr_t, struct ifnet *, @@ -438,7 +438,7 @@ in_control_internal(u_long cmd, caddr_t data, struct ifnet *ifp, case SIOCSIFADDR: error = in_ifinit(ifp, ia, - (struct sockaddr_in *) &ifr->ifr_addr, 1); + (const struct sockaddr_in *)&ifr->ifr_addr, 1); if (error != 0 && iaIsNew) break; if (error == 0) @@ -524,7 +524,10 @@ in_control_internal(u_long cmd, caddr_t data, struct ifnet *ifp, */ crit_enter(); /* XXX MP */ TAILQ_REMOVE(&in_ifaddrhead, ia, ia_link); - LIST_REMOVE(ia, ia_hash); + if (cmd == SIOCDIFADDR && ia->ia_addr.sin_family == AF_INET) { + /* XXX Assume that 'ia' is in hash table */ + LIST_REMOVE(ia, ia_hash); + } crit_exit(); /* XXX MP */ ifa_destroy(&ia->ia_ifa); @@ -732,19 +735,24 @@ in_ifscrub(struct ifnet *ifp, struct in_ifaddr *ia) * and routing table entry. */ static int -in_ifinit(struct ifnet *ifp, struct in_ifaddr *ia, struct sockaddr_in *sin, int scrub) +in_ifinit(struct ifnet *ifp, struct in_ifaddr *ia, + const struct sockaddr_in *sin, int scrub) { u_long i = ntohl(sin->sin_addr.s_addr); struct sockaddr_in oldaddr; int flags = RTF_UP, error = 0; + int old_hash = 0, new_hash = 0; crit_enter(); oldaddr = ia->ia_addr; - if (oldaddr.sin_family == AF_INET) + if (oldaddr.sin_family == AF_INET) { + old_hash = 1; LIST_REMOVE(ia, ia_hash); + } ia->ia_addr = *sin; if (ia->ia_addr.sin_family == AF_INET) { + new_hash = 1; LIST_INSERT_HEAD(INADDR_HASH(ia->ia_addr.sin_addr.s_addr), ia, ia_hash); } @@ -759,18 +767,8 @@ in_ifinit(struct ifnet *ifp, struct in_ifaddr *ia, struct sockaddr_in *sin, int lwkt_serialize_enter(ifp->if_serializer); error = ifp->if_ioctl(ifp, SIOCSIFADDR, (caddr_t)ia, NULL); lwkt_serialize_exit(ifp->if_serializer); - if (error) { - /* LIST_REMOVE(ia, ia_hash) is done in in_control */ - ia->ia_addr = oldaddr; - if (ia->ia_addr.sin_family == AF_INET) { - crit_enter(); - LIST_INSERT_HEAD( - INADDR_HASH(ia->ia_addr.sin_addr.s_addr), - ia, ia_hash); - crit_exit(); - } - return (error); - } + if (error) + goto fail; } /* @@ -836,10 +834,8 @@ in_ifinit(struct ifnet *ifp, struct in_ifaddr *ia, struct sockaddr_in *sin, int if (ia->ia_addr.sin_addr.s_addr != INADDR_ANY || ia->ia_netmask != IN_CLASSA_NET || ia->ia_dstaddr.sin_addr.s_addr != htonl(IN_CLASSA_HOST)) { - if ((error = rtinit(&ia->ia_ifa, RTM_ADD, flags)) != 0) { - ia->ia_addr = oldaddr; - return (error); - } + if ((error = rtinit(&ia->ia_ifa, RTM_ADD, flags)) != 0) + goto fail; ia->ia_flags |= IFA_ROUTE; } @@ -853,6 +849,18 @@ in_ifinit(struct ifnet *ifp, struct in_ifaddr *ia, struct sockaddr_in *sin, int addr.s_addr = htonl(INADDR_ALLHOSTS_GROUP); in_addmulti(&addr, ifp); } + return (0); +fail: + crit_enter(); + if (new_hash) + LIST_REMOVE(ia, ia_hash); + + ia->ia_addr = oldaddr; + if (old_hash) { + LIST_INSERT_HEAD(INADDR_HASH(ia->ia_addr.sin_addr.s_addr), + ia, ia_hash); + } + crit_exit(); return (error); } -- 2.11.4.GIT