repair policy-ikesa-linking by replacing the broken RB_TREE w/TAILQ

(e.g. the policy might be used-after-free on 'ikectl reconfig')
ok mikeb@

4 files changed, 37 insertions, 98 deletions

diff --git a/sbin/iked/config.c b/sbin/iked/config.c
index 0adbf2c0de3..0e05d8f0361 100644
--- a/sbin/iked/config.c
+++ b/sbin/iked/config.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: config.c,v 1.35 2015/02/06 10:39:01 deraadt Exp $	*/
+/*	$OpenBSD: config.c,v 1.36 2015/07/07 19:13:31 markus Exp $	*/
 
 /*
  * Copyright (c) 2010-2013 Reyk Floeter <reyk@openbsd.org>
@@ -106,7 +106,7 @@ config_free_sa(struct iked *env, struct iked_sa *sa)
 	}
 
 	if (sa->sa_policy) {
-		(void)RB_REMOVE(iked_sapeers, &sa->sa_policy->pol_sapeers, sa);
+		TAILQ_REMOVE(&sa->sa_policy->pol_sapeers, sa, sa_peer_entry);
 		policy_unref(env, sa->sa_policy);
 	}
 
@@ -157,8 +157,10 @@ config_new_policy(struct iked *env)
 	if ((pol = calloc(1, sizeof(*pol))) == NULL)
 		return (NULL);
 
+	/* XXX caller does this again */
 	TAILQ_INIT(&pol->pol_proposals);
-	RB_INIT(&pol->pol_sapeers);
+	TAILQ_INIT(&pol->pol_sapeers);
+	RB_INIT(&pol->pol_flows);
 
 	return (pol);
 }
@@ -173,10 +175,13 @@ config_free_policy(struct iked *env, struct iked_policy *pol)
 
 	TAILQ_REMOVE(&env->sc_policies, pol, pol_entry);
 
-	RB_FOREACH(sa, iked_sapeers, &pol->pol_sapeers) {
-		/* Remove from the policy tree, but keep for existing SAs */
+	TAILQ_FOREACH(sa, &pol->pol_sapeers, sa_peer_entry) {
+		/* Remove from the policy list, but keep for existing SAs */
 		if (sa->sa_policy == pol)
 			policy_ref(env, pol);
+		else
+			log_warnx("%s: ERROR: sa_policy %p != pol %p",
+			    __func__, sa->sa_policy, pol);
 	}
 
 	if (pol->pol_refcnt)
@@ -687,6 +692,7 @@ config_getpolicy(struct iked *env, struct imsg *imsg)
 	offset += sizeof(*pol);
 
 	TAILQ_INIT(&pol->pol_proposals);
+	TAILQ_INIT(&pol->pol_sapeers);
 	RB_INIT(&pol->pol_flows);
 
 	for (i = 0; i < pol->pol_nproposals; i++) {
diff --git a/sbin/iked/iked.h b/sbin/iked/iked.h
index 568fceba9a1..7af0bc726b5 100644
--- a/sbin/iked/iked.h
+++ b/sbin/iked/iked.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: iked.h,v 1.85 2015/06/11 18:49:09 reyk Exp $	*/
+/*	$OpenBSD: iked.h,v 1.86 2015/07/07 19:13:31 markus Exp $	*/
 
 /*
  * Copyright (c) 2010-2013 Reyk Floeter <reyk@openbsd.org>
@@ -215,7 +215,7 @@ struct iked_cfg {
 	} cfg;
 };
 
-RB_HEAD(iked_sapeers, iked_sa);
+TAILQ_HEAD(iked_sapeers, iked_sa);
 
 struct iked_lifetime {
 	u_int64_t			 lt_bytes;
@@ -363,7 +363,6 @@ struct iked_sa {
 #define IKED_SATYPE_LOCAL		 1		/* Local SA */
 
 	struct iked_addr		 sa_peer;
-	struct iked_addr		 sa_polpeer;
 	struct iked_addr		 sa_local;
 	int				 sa_fd;
 
@@ -443,7 +442,7 @@ struct iked_sa {
 	struct iked_msgqueue		 sa_responses;	/* response queue */
 #define IKED_RESPONSE_TIMEOUT		 120		/* 2 minutes */
 
-	RB_ENTRY(iked_sa)		 sa_peer_entry;
+	TAILQ_ENTRY(iked_sa)		 sa_peer_entry;
 	RB_ENTRY(iked_sa)		 sa_entry;
 
 	struct iked_addr		*sa_addrpool;	/* address from pool */
@@ -677,19 +676,16 @@ struct iked_sa *
 void	 sa_free(struct iked *, struct iked_sa *);
 void	 sa_free_flows(struct iked *, struct iked_saflows *);
 int	 sa_address(struct iked_sa *, struct iked_addr *,
-	    struct sockaddr_storage *, int);
+	    struct sockaddr_storage *);
 void	 childsa_free(struct iked_childsa *);
 struct iked_childsa *
 	 childsa_lookup(struct iked_sa *, u_int64_t, u_int8_t);
 void	 flow_free(struct iked_flow *);
 struct iked_sa *
 	 sa_lookup(struct iked *, u_int64_t, u_int64_t, u_int);
-struct iked_sa *
-	 sa_peer_lookup(struct iked_policy *, struct sockaddr_storage *);
 struct iked_user *
 	 user_lookup(struct iked *, const char *);
 RB_PROTOTYPE(iked_sas, iked_sa, sa_entry, sa_cmp);
-RB_PROTOTYPE(iked_sapeers, iked_sa, sa_peer_entry, sa_peer_cmp);
 RB_PROTOTYPE(iked_addrpool, iked_sa, sa_addrpool_entry, sa_addrpool_cmp);
 RB_PROTOTYPE(iked_users, iked_user, user_entry, user_cmp);
 RB_PROTOTYPE(iked_activesas, iked_childsa, csa_node, childsa_cmp);
diff --git a/sbin/iked/ikev2.c b/sbin/iked/ikev2.c
index a2c94622a8b..7b676e6262f 100644
--- a/sbin/iked/ikev2.c
+++ b/sbin/iked/ikev2.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ikev2.c,v 1.120 2015/03/26 19:52:35 markus Exp $	*/
+/*	$OpenBSD: ikev2.c,v 1.121 2015/07/07 19:13:31 markus Exp $	*/
 
 /*
  * Copyright (c) 2010-2013 Reyk Floeter <reyk@openbsd.org>
@@ -473,10 +473,8 @@ ikev2_recv(struct iked *env, struct iked_message *msg)
 		ikev2_msg_prevail(env, &sa->sa_responses, msg);
 	}
 
-	if (sa_address(sa, &sa->sa_peer, &msg->msg_peer,
-	    sa->sa_hdr.sh_initiator) == -1 ||
-	    sa_address(sa, &sa->sa_local, &msg->msg_local,
-	    sa->sa_hdr.sh_initiator) == -1)
+	if (sa_address(sa, &sa->sa_peer, &msg->msg_peer) == -1 ||
+	    sa_address(sa, &sa->sa_local, &msg->msg_local) == -1)
 		return;
 
 	sa->sa_fd = msg->msg_fd;
@@ -532,19 +530,10 @@ ikev2_ike_auth_recv(struct iked *env, struct iked_sa *sa,
 		sa->sa_policy = NULL;
 		if (policy_lookup(env, msg) == 0 && msg->msg_policy &&
 		    msg->msg_policy != old) {
-			log_debug("%s: policy switch %p/%s to %p/%s",
-			    __func__, old, old->pol_name,
-			    msg->msg_policy, msg->msg_policy->pol_name);
-			RB_REMOVE(iked_sapeers, &old->pol_sapeers, sa);
-			if (RB_INSERT(iked_sapeers,
-			    &msg->msg_policy->pol_sapeers, sa)) {
-				/* failed, restore */
-				log_debug("%s: conflicting sa", __func__);
-				RB_INSERT(iked_sapeers, &old->pol_sapeers, sa);
-				msg->msg_policy = old;
-			} else
-				policy_unref(env, old);
+			/* move sa to new policy */
 			policy = sa->sa_policy = msg->msg_policy;
+			TAILQ_REMOVE(&old->pol_sapeers, sa, sa_peer_entry);
+			TAILQ_INSERT_TAIL(&policy->pol_sapeers, sa, sa_peer_entry);
 		} else {
 			/* restore */
 			msg->msg_policy = sa->sa_policy = old;
@@ -773,7 +762,7 @@ ikev2_init_ike_sa(struct iked *env, void *arg)
 	TAILQ_FOREACH(pol, &env->sc_policies, pol_entry) {
 		if ((pol->pol_flags & IKED_POLICY_ACTIVE) == 0)
 			continue;
-		if (sa_peer_lookup(pol, &pol->pol_peer.addr) != NULL) {
+		if (!TAILQ_EMPTY(&pol->pol_sapeers)) {
 			log_debug("%s: \"%s\" is already active",
 			    __func__, pol->pol_name);
 			continue;
@@ -942,12 +931,6 @@ ikev2_init_ike_sa_peer(struct iked *env, struct iked_policy *pol,
 		goto done;
 	}
 
-	memcpy(&sa->sa_polpeer, &pol->pol_peer, sizeof(sa->sa_polpeer));
-	if (RB_INSERT(iked_sapeers, &pol->pol_sapeers, sa)) {
-		log_debug("%s: conflicting sa", __func__);
-		goto done;
-	}
-
 	if ((ret = ikev2_msg_send(env, &req)) == 0)
 		sa_state(env, sa, IKEV2_STATE_SA_INIT);
 
@@ -2880,7 +2863,6 @@ ikev2_ikesa_enable(struct iked *env, struct iked_sa *sa, struct iked_sa *nsa)
 	struct iked_childsa		*csa, *nextcsa;
 	struct iked_flow		*flow, *nextflow;
 	struct iked_proposal		*prop, *nextprop;
-	int				 initiator;
 
 	log_debug("%s: IKE SA %p ispi %s rspi %s replaced"
 	    " by SA %p ispi %s rspi %s ",
@@ -2891,29 +2873,6 @@ ikev2_ikesa_enable(struct iked *env, struct iked_sa *sa, struct iked_sa *nsa)
 	    print_spi(nsa->sa_hdr.sh_ispi, 8),
 	    print_spi(nsa->sa_hdr.sh_rspi, 8));
 
-	/*
-	 * Transfer policy and address:
-	 * - Remember if we initiated the original IKE-SA because of our policy.
-	 * - Note that sa_address() will insert the new SA when we set sa_peer.
-	 */
-	initiator = !memcmp(&sa->sa_polpeer, &sa->sa_policy->pol_peer,
-	    sizeof(sa->sa_polpeer));
-	nsa->sa_policy = sa->sa_policy;
-	RB_REMOVE(iked_sapeers, &sa->sa_policy->pol_sapeers, sa);
-	sa->sa_policy = NULL;
-	if (sa_address(nsa, &nsa->sa_peer, &sa->sa_peer.addr,
-	    initiator) == -1 ||
-	    sa_address(nsa, &nsa->sa_local, &sa->sa_local.addr,
-	    initiator) == -1) {
-		/* reinsert old SA :/ */
-		sa->sa_policy = nsa->sa_policy;
-		if (RB_FIND(iked_sapeers, &nsa->sa_policy->pol_sapeers, nsa))
-			RB_REMOVE(iked_sapeers, &nsa->sa_policy->pol_sapeers, nsa);
-		RB_INSERT(iked_sapeers, &sa->sa_policy->pol_sapeers, sa);
-		nsa->sa_policy = NULL;
-		return (-1);
-	}
-
 	/* Transfer socket and NAT information */
 	nsa->sa_fd = sa->sa_fd;
 	nsa->sa_natt = sa->sa_natt;
diff --git a/sbin/iked/policy.c b/sbin/iked/policy.c
index 99e55dccf6b..86087ef2df7 100644
--- a/sbin/iked/policy.c
+++ b/sbin/iked/policy.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: policy.c,v 1.36 2015/01/16 06:39:58 deraadt Exp $	*/
+/*	$OpenBSD: policy.c,v 1.37 2015/07/07 19:13:31 markus Exp $	*/
 
 /*
  * Copyright (c) 2010-2013 Reyk Floeter <reyk@openbsd.org>
@@ -214,6 +214,16 @@ policy_unref(struct iked *env, struct iked_policy *pol)
 		return;
 	if (--(pol->pol_refcnt) <= 0)
 		config_free_policy(env, pol);
+	else {
+		struct iked_sa		*tmp;
+		int			 count = 0;
+
+		TAILQ_FOREACH(tmp, &pol->pol_sapeers, sa_peer_entry)
+			count++;
+		if (count != pol->pol_refcnt)
+			log_warnx("%s: ERROR pol %p pol_refcnt %d != count %d",
+			    __func__, pol, pol->pol_refcnt, count);
+	}
 }
 
 void
@@ -332,9 +342,10 @@ sa_new(struct iked *env, u_int64_t ispi, u_int64_t rspi,
 	if (initiator && sa->sa_hdr.sh_rspi == 0 && rspi)
 		sa->sa_hdr.sh_rspi = rspi;
 
-	if (sa->sa_policy == NULL)
+	if (sa->sa_policy == NULL) {
 		sa->sa_policy = pol;
-	else
+		TAILQ_INSERT_TAIL(&pol->pol_sapeers, sa, sa_peer_entry);
+	} else
 		pol = sa->sa_policy;
 
 	sa->sa_statevalid = IKED_REQ_AUTH|IKED_REQ_AUTHVALID|IKED_REQ_SA;
@@ -401,15 +412,8 @@ sa_free_flows(struct iked *env, struct iked_saflows *head)
 
 int
 sa_address(struct iked_sa *sa, struct iked_addr *addr,
-    struct sockaddr_storage *peer, int initiator)
+    struct sockaddr_storage *peer)
 {
-	struct iked_policy	*pol = sa->sa_policy;
-
-	if (sa->sa_state != IKEV2_STATE_CLOSING && pol == NULL) {
-		log_debug("%s: missing policy", __func__);
-		return (-1);
-	}
-
 	bzero(addr, sizeof(*addr));
 	addr->addr_af = peer->ss_family;
 	addr->addr_port = htons(socket_getport((struct sockaddr *)peer));
@@ -418,15 +422,6 @@ sa_address(struct iked_sa *sa, struct iked_addr *addr,
 		log_debug("%s: invalid address", __func__);
 		return (-1);
 	}
-
-	if (addr == &sa->sa_peer && pol) {
-		/* XXX Re-insert node into the tree */
-		RB_REMOVE(iked_sapeers, &pol->pol_sapeers, sa);
-		memcpy(&sa->sa_polpeer, initiator ? &pol->pol_peer :
-		    &sa->sa_peer, sizeof(sa->sa_polpeer));
-		RB_INSERT(iked_sapeers, &pol->pol_sapeers, sa);
-	}
-
 	return (0);
 }
 
@@ -519,22 +514,6 @@ sa_cmp(struct iked_sa *a, struct iked_sa *b)
 	return (0);
 }
 
-struct iked_sa *
-sa_peer_lookup(struct iked_policy *pol, struct sockaddr_storage *peer)
-{
-	struct iked_sa	 key;
-
-	memcpy(&key.sa_polpeer.addr, peer, sizeof(*peer));
-	return (RB_FIND(iked_sapeers, &pol->pol_sapeers, &key));
-}
-
-static __inline int
-sa_peer_cmp(struct iked_sa *a, struct iked_sa *b)
-{
-	return (sockaddr_cmp((struct sockaddr *)&a->sa_polpeer.addr,
-	    (struct sockaddr *)&b->sa_polpeer.addr, -1));
-}
-
 static __inline int
 sa_addrpool_cmp(struct iked_sa *a, struct iked_sa *b)
 {
@@ -603,7 +582,6 @@ flow_cmp(struct iked_flow *a, struct iked_flow *b)
 }
 
 RB_GENERATE(iked_sas, iked_sa, sa_entry, sa_cmp);
-RB_GENERATE(iked_sapeers, iked_sa, sa_peer_entry, sa_peer_cmp);
 RB_GENERATE(iked_addrpool, iked_sa, sa_addrpool_entry, sa_addrpool_cmp);
 RB_GENERATE(iked_users, iked_user, usr_entry, user_cmp);
 RB_GENERATE(iked_activesas, iked_childsa, csa_node, childsa_cmp);