Review feedback

20 files changed, 152 insertions, 122 deletions

diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/config.c b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/config.c
index 9379c1e055..7cff00a777 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/config.c
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/config.c
@@ -322,8 +322,10 @@ config_id_copy(const config_id_t *src)
 int
 config_id_cmp(const config_id_t *l, const config_id_t *r)
 {
-	if (l->cid_type != r->cid_type)
-		return (l->cid_type - r->cid_type);
+	if (l->cid_type < r->cid_type)
+		return (-1);
+	if (l->cid_type > r->cid_type)
+		return (1);
 	return (memcmp(l->cid_data, r->cid_data, MIN(l->cid_len, r->cid_len)));
 }
 
@@ -348,8 +350,6 @@ cfg_rule_free(config_rule_t *rule)
 
 	if (rule->rule_xf != NULL) {
 		for (i = 0; rule->rule_xf[i] != NULL; i++) {
-			char *s = rule->rule_xf[i]->xf_str;
-
 			ustrfree(rule->rule_xf[i]->xf_str);
 			umem_free(rule->rule_xf[i], sizeof (config_xf_t));
 		}
diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/config_parse.c b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/config_parse.c
index 3894c44297..5b3f516c6c 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/config_parse.c
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/config_parse.c
@@ -653,8 +653,7 @@ parse_xform(input_cursor_t *restrict ic, config_xf_t **restrict xfp)
 
 	start = start_t->t_linep + start_t->t_col;
 
-	/*CONSTCOND*/
-	while (1) {
+	for (;;) {
 		t = input_token(ic, B_FALSE);
 		if (t == NULL) {
 			(void) bunyan_error(log,
diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_child_sa.c b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_child_sa.c
index 2c1b24f44c..c43dbf0219 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_child_sa.c
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_child_sa.c
@@ -30,9 +30,15 @@
 #include "ts.h"
 #include "worker.h"
 
-#define	IS_AUTH(args) ((args)->i2a_is_auth)
-#define	INITIATOR(args) ((args)->i2a_child[0].csa_child->i2c_initiator)
-#define	TRANSPORT_MODE(args) ((args)->i2a_child[0].csa_child->i2c_transport)
+/*
+ * Is the ikev2_sa_args_t being used during an IKE_AUTH exchange for the
+ * piggybacked child SA
+ */
+#define	IS_AUTH(csa) ((csa)->i2a_is_auth)
+/* Are we initiating the creation of this child SA */
+#define	INITIATOR(csa) ((csa)->i2a_child[0].csa_child->i2c_initiator)
+/* Is the child SA we're creating transport mode? */
+#define	TRANSPORT_MODE(csa) ((csa)->i2a_child[0].csa_child->i2c_transport)
 
 static boolean_t ikev2_create_child_sa_init_common(ikev2_sa_t *restrict,
     pkt_t *restrict req, ikev2_sa_args_t *restrict);
@@ -251,8 +257,6 @@ ikev2_rekey_child_sa_init(ikev2_sa_t *restrict sa, parsedmsg_t *restrict pmsg)
 	return;
 
 fail:
-	/* XXX: Do we need to reply to SADB_EXPIRE messages if we failed? */
-	pfkey_send_error(pmsg->pmsg_samsg, ENOMEM);
 	ikev2_sa_args_free(args);
 }
 #endif
@@ -381,7 +385,7 @@ ikev2_create_child_sa_resp(pkt_t *restrict req)
 		    "sending NO_PROPOSAL_CHOSEN", BUNYAN_T_END);
 
 		VERIFY(ikev2_add_notify(resp, IKEV2_N_NO_PROPOSAL_CHOSEN));
-		(void) ikev2_send_resp(resp);
+		ikev2_send_resp(resp);
 		return;
 	}
 
@@ -396,7 +400,7 @@ ikev2_create_child_sa_resp(pkt_t *restrict req)
 	csa->i2a_dh = i2sa->i2sa_rule->rule_p2_dh;
 
 	if (ikev2_create_child_sa_resp_common(req, resp, csa))
-		(void) ikev2_send_resp(resp);
+		ikev2_send_resp(resp);
 	else
 		ikev2_pkt_free(resp);
 
@@ -654,18 +658,6 @@ ikev2_create_child_sa_init_resp_common(ikev2_sa_t *restrict i2sa,
 		goto fail;
 	}
 
-	/*
-	 * This is not fatal -- the traffic we wanted to send can be sent,
-	 * but the responder has chosen a subset of our policy, so we want
-	 * to log this.
-	 */
-	if (pkt_get_notify(resp,
-	    IKEV2_N_ADDITIONAL_TS_POSSIBLE, NULL) != NULL) {
-		(void) bunyan_warn(log,
-		    "Policy mismatch with peer", BUNYAN_T_END);
-		/* XXX: Log more details */
-	}
-
 	if (csa->i2a_is_auth)
 		check_natt_addrs(resp, TRANSPORT_MODE(csa));
 
@@ -679,6 +671,18 @@ ikev2_create_child_sa_init_resp_common(ikev2_sa_t *restrict i2sa,
 		goto fail;
 	}
 
+	/*
+	 * This is not fatal -- the traffic we wanted to send can be sent,
+	 * but the responder has chosen a subset of our policy, so we want
+	 * to log this.
+	 */
+	if (narrowed || pkt_get_notify(resp,
+	    IKEV2_N_ADDITIONAL_TS_POSSIBLE, NULL) != NULL) {
+		(void) bunyan_warn(log,
+		    "Policy mismatch with peer", BUNYAN_T_END);
+		/* XXX: Log more details */
+	}
+
 	ikev2_set_child_type(csa->i2a_child, B_TRUE, match.ism_satype);
 	ikev2_save_child_results(csa->i2a_child, &match);
 	ikev2_save_child_ts(csa->i2a_child, &ts_i, &ts_r);
@@ -1216,7 +1220,7 @@ ikev2_sa_select_prop(sadb_x_ecomb_t *restrict ecomb, ikev2_dh_t dh,
 
 				m->ism_encr = xfid;
 				m->ism_encr_saltlen =
-				    alg->sadb_x_algdesc_reserved;
+				    alg->sadb_x_algdesc_saltbits;
 				if (m->ism_encr_keylen == 0)
 					m->ism_encr_keylen = ed->ed_keydefault;
 
diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_common.c b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_common.c
index 4f98508ebe..b06644e4b0 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_common.c
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_common.c
@@ -279,7 +279,7 @@ ikev2_sa_match_prop(config_xf_t *restrict rxf,
 	m->ism_lifetime_secs = rxf->xf_lifetime_secs;
 
 	if (m->ism_spi != 0) {
-		(void) snprintf(spibuf, sizeof (spibuf), "0x" PRIx64,
+		(void) snprintf(spibuf, sizeof (spibuf), "0x%016" PRIx64,
 		    m->ism_spi);
 	} else {
 		(void) strlcpy(spibuf, "0x0", sizeof (spibuf));
@@ -301,7 +301,7 @@ ikev2_sa_match_prop(config_xf_t *restrict rxf,
 
 	/*
 	 * For non-combined mode encryption mechanisms, we must also have
-	 * an integrity mechism selected.
+	 * an integrity mechanism selected.
 	 */
 	if (rxf->xf_auth != IKEV2_XF_AUTH_NONE)
 		m->ism_have |= SEEN(IKEV2_XF_AUTH);
@@ -886,13 +886,13 @@ ikev2_sa_args_new(boolean_t create_children)
 	if (create_children) {
 		in = ikev2_child_sa_alloc(B_TRUE);
 		out = ikev2_child_sa_alloc(B_FALSE);
-	}
 
-	if (create_children && (in == NULL || out == NULL)) {
-		ikev2_child_sa_free(NULL, in);
-		ikev2_child_sa_free(NULL, out);
-		umem_free(args, sizeof (*args));
-		return (NULL);
+		if (in == NULL || out == NULL) {
+			ikev2_child_sa_free(NULL, in);
+			ikev2_child_sa_free(NULL, out);
+			umem_free(args, sizeof (*args));
+			return (NULL);
+		}
 	}
 
 	args->i2a_child[CSA_IN].csa_child = in;
diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_cookie.c b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_cookie.c
index 37c3416bea..ea5e961477 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_cookie.c
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_cookie.c
@@ -92,6 +92,16 @@ uint_t ikev2_cookie_secret_grace = COOKIE_GRACE;
  * complex, and we MUST support responding to a responder who sends us
  * cookies -- so it's not really doing much harm.
  */
+static periodic_id_t cookie_timer_id;
+static hrtime_t grace_ns;	/* Only set at startup */
+
+/*
+ * i2c_lock protectes i2c_enabled as well as the secret (current and previous),
+ * as well as i2c_version.
+ */
+static rwlock_t i2c_lock = DEFAULTRWLOCK;
+static uint8_t i2c_version;
+static boolean_t i2c_enabled;
 static struct secret_s {
 	uint8_t s_val[COOKIE_SECRET_LEN];
 	hrtime_t s_birth;
@@ -100,13 +110,6 @@ static struct secret_s {
 #define	SECRET_BIRTH(v) i2c_secret[(v) & 0x1].s_birth
 #define	SECRET_AGE(v) (gethrtime() - SECRET_BIRTH(v))
 
-static hrtime_t grace_ns;	/* Only set at startup */
-
-static rwlock_t i2c_lock = DEFAULTRWLOCK;
-static volatile uint8_t i2c_version;
-static boolean_t i2c_enabled;
-static periodic_id_t cookie_timer_id;
-
 static void cookie_update_secret(void *);
 
 void
diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_ike_auth.c b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_ike_auth.c
index 52cd2310e6..c2a0ee82d8 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_ike_auth.c
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_ike_auth.c
@@ -76,6 +76,13 @@ ikev2_ike_auth_init(ikev2_sa_t *restrict sa)
 
 	/* XXX: CERT */
 
+	/*
+	 * Since we should be the only IKE instance running on this host/zone,
+	 * if we are initiating a new IKE SA, it means we have no existing
+	 * IKE SA for this peer.  Inform the peer of this in case we restarted
+	 * due to a crash or such.  This allows the peer to clear out any
+	 * leftover IKE SA it might have for us.
+	 */
 	if (!ikev2_add_notify(req, IKEV2_N_INITIAL_CONTACT))
 		goto fail;
 
@@ -153,7 +160,7 @@ ikev2_ike_auth_resp(pkt_t *req)
 
 		/* This is the 2nd payload (after SK) -- it should always fit */
 		VERIFY(ikev2_add_notify(resp, IKEV2_N_INVALID_SYNTAX));
-		(void) ikev2_send_resp(resp);
+		ikev2_send_resp(resp);
 		return;
 	}
 
@@ -196,8 +203,8 @@ ikev2_ike_auth_resp(pkt_t *req)
 	    BUNYAN_T_END);
 
 	/*
-	 * XXX: Check for INITIAL_CONTACT, if there delete any existing
-	 * IPsec SAs between the two hosts.
+	 * XXX: Check for INITIAL_CONTACT, if there are, delete any existing
+	 * IPsec SAs between the two hosts based on the authenticated ID.
 	 */
 
 	/* The initiator may optionally request we send a specific ID */
@@ -237,11 +244,7 @@ ikev2_ike_auth_resp(pkt_t *req)
 	if (!ikev2_create_child_sa_resp_auth(req, resp))
 		goto fail;
 
-	if (!ikev2_send_resp(resp)) {
-		resp = NULL;
-		goto fail;
-	}
-
+	ikev2_send_resp(resp);
 	ikev2_sa_args_free(sa->sa_init_args);
 	sa->sa_init_args = NULL;
 	return;
@@ -263,7 +266,7 @@ authfail:
 	sa->flags |= I2SA_CONDEMNED;
 
 	VERIFY(ikev2_add_notify(resp, IKEV2_N_AUTHENTICATION_FAILED));
-	(void) ikev2_send_resp(resp);
+	ikev2_send_resp(resp);
 }
 
 /*
@@ -298,7 +301,8 @@ ikev2_ike_auth_init_resp(ikev2_sa_t *restrict sa, pkt_t *restrict resp,
 	 * delete the IKE SA, or if we (the initiator) reject the IKE SA
 	 * (as opposed to the responder), it all must be done in a separate
 	 * exchange.  All of that is to say, that this is the only notification
-	 * we check for doing the authentication portion of the processing.
+	 * we check for before doing the authentication portion of the
+	 * processing.
 	 */
 	if (pkt_get_notify(resp, IKEV2_N_AUTHENTICATION_FAILED, NULL) != NULL) {
 		(void) bunyan_warn(log,
@@ -520,6 +524,7 @@ calc_auth(ikev2_sa_t *restrict sa, boolean_t initiator,
 	case IKEV2_AUTH_ECDSA_384:
 	case IKEV2_AUTH_ECDSA_512:
 	case IKEV2_AUTH_GSPM:
+		/* TODO: Hook in certificate authentication support */
 		(void) bunyan_info(log,
 		    "IKE SA Authentication method not yet implemented",
 		    BUNYAN_T_END);
@@ -608,7 +613,8 @@ add_id(pkt_t *pkt, const config_id_t *id, boolean_t initiator)
 {
 	ikev2_sa_t *sa = pkt->pkt_sa;
 	config_auth_id_t cid_type = 0;
-	ikev2_id_type_t id_type = IKEV2_ID_FQDN; /* set default to quiet GCC */
+	/* arbitrary default value to quiet GCC */
+	ikev2_id_type_t id_type = IKEV2_ID_FQDN;
 	const void *ptr = NULL;
 	size_t idlen = 0;
 
diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_pkt.c b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_pkt.c
index f5dc360408..9b28f33836 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_pkt.c
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_pkt.c
@@ -141,7 +141,7 @@ ikev2_pkt_new_response(const pkt_t *init)
 	return (pkt);
 }
 
-/* Allocate a ikev2_pkt_t for an inbound datagram in raw */
+/* Allocate a ikev2_pkt_t for an inbound datagram in buf */
 pkt_t *
 ikev2_pkt_new_inbound(void *restrict buf, size_t buflen)
 {
@@ -795,9 +795,9 @@ add_iv(pkt_t *restrict pkt)
 {
 	ikev2_sa_t *sa = pkt->pkt_sa;
 	const encr_data_t *ed = encr_data(sa->encr);
-	size_t len = ed->ed_blocklen;
+	size_t ivlen = ed->ed_ivlen;
 
-	if (pkt_write_left(pkt) < len)
+	if (pkt_write_left(pkt) < ivlen)
 		return (B_FALSE);
 
 	switch (ed->ed_mode) {
@@ -810,7 +810,7 @@ add_iv(pkt_t *restrict pkt)
 		 * requirements.
 		 */
 		VERIFY(put32(pkt, msgid));
-		pkt->pkt_ptr += (len - sizeof (msgid));
+		pkt->pkt_ptr += (ivlen - sizeof (msgid));
 		return (B_TRUE);
 	}
 	case MODE_CTR:
@@ -835,6 +835,13 @@ add_iv(pkt_t *restrict pkt)
 	CK_RV rc = CKR_OK;
 	uint32_t msgid = ntohl(pkt_header(pkt)->msgid);
 
+	/*
+	 * The IV length should always <= the block size of the related
+	 * mechanism.  This also means we only ever need to compute a single
+	 * block when generating the IV value.
+	 */
+	VERIFY3U(ivlen, <=, blocklen);
+
 	if (sa->flags & I2SA_INITIATOR)
 		key = sa->sk_ei;
 	else
@@ -873,9 +880,9 @@ add_iv(pkt_t *restrict pkt)
 		return (B_FALSE);
 	}
 
-	bcopy(buf, pkt->pkt_ptr, MIN(len, blocklen));
+	bcopy(buf, pkt->pkt_ptr, ivlen);
 	explicit_bzero(buf, blocklen);
-	pkt->pkt_ptr += MIN(len, blocklen);
+	pkt->pkt_ptr += ivlen;
 	return (B_TRUE);
 }
 
@@ -1181,7 +1188,7 @@ ikev2_pkt_done(pkt_t *pkt)
 	if (pkt_write_left(pkt) < padlen + 1 + icvlen) {
 		(void) bunyan_info(log, "Not enough space for packet",
 		    BUNYAN_T_END);
-		goto done;
+		return (B_FALSE);
 	}
 
 	/*
@@ -1207,8 +1214,6 @@ ikev2_pkt_done(pkt_t *pkt)
 	ok = pkt_done(pkt);
 	ok &= ikev2_pkt_encryptdecrypt(pkt, B_TRUE);
 	ok &= ikev2_pkt_signverify(pkt, B_TRUE);
-
-done:
 	return (ok);
 }
 
diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_pkt.h b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_pkt.h
index 919b73f565..a7a32f7009 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_pkt.h
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_pkt.h
@@ -35,11 +35,11 @@ struct sockaddr;
 #define	I2P_INITIATOR(p) (pkt_header(p)->flags & IKEV2_FLAG_INITIATOR)
 
 #define	INBOUND_LOCAL_SPI(hdr) \
-	(((hdr)->flags == IKEV2_FLAG_INITIATOR) ? \
+	((((hdr)->flags & IKEV2_FLAG_INITIATOR) != 0) ? \
 	    (hdr)->responder_spi : (hdr)->initiator_spi)
 
 #define	INBOUND_REMOTE_SPI(hdr) \
-	(((hdr)->flags == IKEV2_FLAG_INITIATOR) ? \
+	((((hdr)->flags & IKEV2_FLAG_INITIATOR) != 0) ? \
 	    (hdr)->initiator_spi : (hdr)->responder_spi)
 
 typedef struct ikev2_pkt_ts_state {
@@ -92,7 +92,7 @@ boolean_t ikev2_add_vendor(pkt_t *restrict, const void *restrict,
 
 /*
  * The usage of 'struct sockaddr *' vs 'struct sockaddr_storage *'
- * reflects the assumptions about the side of the struct pointed to --
+ * reflects the assumptions about the size of the struct pointed to --
  * functions that take 'struct sockaddr' are assumed to be sized according
  * to the value of sa_family (i.e. AF_INET implies 'struct sockaddr_in'
  * while AF_INET6 implies 'struct sockaddr_in6'), while 'sockaddr_storage'
diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_pkt_check.c b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_pkt_check.c
index f0df4a6f1c..cb3d3edb8d 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_pkt_check.c
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_pkt_check.c
@@ -657,7 +657,11 @@ ikev2_pkt_check_informational(pkt_t *pkt)
 	if (!pkt->pkt_decrypted)
 		return (ikev2_pkt_check_predecrypt(pkt));
 
-	/* Can have a bunch of things, so don't worry about it */
+	/*
+	 * There's no well defined list of things that can appear in an
+	 * informational exchange, so we rely on the processing there to
+	 * handle them properly.
+	 */
 	return (B_TRUE);
 }
 #undef	PAYIDX
@@ -688,6 +692,7 @@ ikev2_pkt_check_payloads(pkt_t *pkt)
 		break;
 	}
 
+	abort();
 	/*NOTREACHED*/
 	return (B_FALSE);
 }
diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_proto.c b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_proto.c
index 8f5b8cb232..64fc1f9277 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_proto.c
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_proto.c
@@ -247,9 +247,9 @@ fail:
 }
 
 /*
- * Take a parsed pfkey message, and either locate an existing IKE SA and
- * add to it's queue, or create a larval IKE SA and kickoff the
- * IKE_SA_INIT exchange.
+ * Take a parsed pfkey message (either an SADB_ACQUIRE or SADB_EXPIRE message),
+ * and either locate an existing IKE SA and add to it's queue, or create a
+ * larval IKE SA and kickoff the IKE_SA_INIT exchange.
  */
 void
 ikev2_pfkey(parsedmsg_t *pmsg)
@@ -528,6 +528,15 @@ ikev2_send_req(pkt_t *restrict req, ikev2_send_cb_t cb, void *restrict arg)
 		return (B_FALSE);
 	}
 
+	/*
+	 * If we receive a valid response to this request,
+	 * ikev2_handle_response() will consume req and invoke the given
+	 * callback (w/ both arg and the response packet as arguments).
+	 * Success will also cancel the retransmit counter.  If this request
+	 * times out, ikev2_timeout() will consume req and invoke the given
+	 * callback w/ arg and a NULL response packet (to indicate the request
+	 * was a failure and allow the callback to do any necessary cleanup).
+	 */
 	i2req->i2r_pkt = req;
 	i2req->i2r_msgid = ntohl(pkt_header(req)->msgid);
 	i2req->i2r_cb = cb;
@@ -543,7 +552,7 @@ ikev2_send_req(pkt_t *restrict req, ikev2_send_cb_t cb, void *restrict arg)
 }
 
 /* Send a response */
-boolean_t
+void
 ikev2_send_resp(pkt_t *restrict resp)
 {
 	ikev2_sa_t *i2sa = resp->pkt_sa;
@@ -554,12 +563,6 @@ ikev2_send_resp(pkt_t *restrict resp)
 	VERIFY(MUTEX_HELD(&i2sa->i2sa_lock));
 	VERIFY(I2P_RESPONSE(resp));
 
-	if (!ikev2_send_common(resp, SSTOSA(&i2sa->laddr),
-	    SSTOSA(&i2sa->raddr), I2SA_IS_NAT(i2sa))) {
-		ikev2_pkt_free(resp);
-		return (B_FALSE);
-	}
-
         /*
 	 * Normally, we save the last response packet we've sent in order to
 	 * re-send the last response in case the remote system retransmits
@@ -587,7 +590,13 @@ ikev2_send_resp(pkt_t *restrict resp)
 		i2sa->last_resp_sent = resp;
 	}
 
-	return (B_TRUE);
+	/*
+	 * Ignore if we fail.  In the hope that it might be a transitory
+	 * problem, let the retransmit or P1 timer (if appropriate)
+	 * determine if the repsonse fails or not.
+	 */
+	(void) ikev2_send_common(resp, SSTOSA(&i2sa->laddr),
+	    SSTOSA(&i2sa->raddr), I2SA_IS_NAT(i2sa));
 }
 
 /* Used for sending responses outside of an IKEv2 SA */
diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_proto.h b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_proto.h
index bbf3d6a0c2..0ead73221c 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_proto.h
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_proto.h
@@ -40,7 +40,7 @@ typedef void (*ikev2_send_cb_t)(struct ikev2_sa_s *restrict,
     struct pkt_s *restrict, void *restrict);
 boolean_t ikev2_send_req(struct pkt_s *restrict, ikev2_send_cb_t,
     void *restrict);
-boolean_t ikev2_send_resp(struct pkt_s *restrict);
+void ikev2_send_resp(struct pkt_s *restrict);
 boolean_t ikev2_send_resp_addr(struct pkt_s *restrict,
     const struct sockaddr *restrict,
     const struct sockaddr *restrict);
diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_sa.c b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_sa.c
index ca66084f4a..79fb853a16 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_sa.c
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_sa.c
@@ -270,8 +270,7 @@ ikev2_sa_alloc(pkt_t *restrict init_pkt,
 	 * we generate a duplicate (or even far, far, rarer chance 0 is
 	 * returned), just retry until we pick a value that's not in use.
 	 */
-	NOTE(CONSTCOND)
-	while (1) {
+	for (;;) {
 		uint64_t spi = 0;
 
 		arc4random_buf(&spi, sizeof (spi));
diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_sa.h b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_sa.h
index e2ac525455..773adae262 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_sa.h
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_sa.h
@@ -100,7 +100,8 @@ typedef enum i2sa_evt {
  * ikev2_pkt.h for the _SPI() macros.)  It should be allocated with a umem
  * cache.
  *
- * Because of the distinct sets of lookup keys, it requires two linkages.
+ * Because of the distinct sets of lookup keys (local SPI, remote SPI,
+ * address), it requires three linkages.
  */
 struct ikev2_sa_s {
 	/*
diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_sa_init.c b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_sa_init.c
index 208f0418dc..4c70f9b904 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_sa_init.c
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ikev2_sa_init.c
@@ -173,7 +173,7 @@ ikev2_sa_init_resp(pkt_t *pkt)
 	if (sa->i2sa_rule == NULL) {
 		/* This is the 2nd payload, it should fit */
 		VERIFY(ikev2_add_notify(resp, IKEV2_N_NO_PROPOSAL_CHOSEN));
-		(void) ikev2_send_resp(resp);
+		ikev2_send_resp(resp);
 		return;
 	}
 
@@ -189,7 +189,7 @@ ikev2_sa_init_resp(pkt_t *pkt)
 
 	if (!ikev2_sa_match_rule(sa->i2sa_rule, pkt, &sa_result, B_FALSE)) {
 		VERIFY(ikev2_add_notify(resp, IKEV2_N_NO_PROPOSAL_CHOSEN));
-		(void) ikev2_send_resp(resp);
+		ikev2_send_resp(resp);
 		return;
 	}
 
@@ -207,10 +207,11 @@ ikev2_sa_init_resp(pkt_t *pkt)
 	 */
 	sa_args->i2a_dh = sa_result.ism_dh;
 	if (ikev2_get_dhgrp(pkt) != sa_args->i2a_dh) {
-		if (ikev2_invalid_ke(resp, sa_args->i2a_dh))
-			goto send;
-		else
+		if (!ikev2_invalid_ke(resp, sa_args->i2a_dh))
 			goto fail;
+
+		ikev2_send_resp(resp);
+		return;
 	}
 
 	if (!check_nats(pkt))
@@ -239,7 +240,7 @@ ikev2_sa_init_resp(pkt_t *pkt)
 		goto fail;
 
 	/*
-	 * While premissible, we do not currently reuse DH exponentials.  Since
+	 * While permissible, we do not currently reuse DH exponentials.  Since
 	 * generating them is a potentially an expensive operation, we wait
 	 * until necessary to create them.
 	 */
@@ -273,11 +274,7 @@ ikev2_sa_init_resp(pkt_t *pkt)
 	    !ikev2_save_init_pkt(sa_args, resp))
 		goto fail;
 
-send:
-	if (!ikev2_send_resp(resp)) {
-		resp = NULL;
-		goto fail;
-	}
+	ikev2_send_resp(resp);
 	return;
 
 fail:
diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/pfkey.c b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/pfkey.c
index bba09a5f88..328c548ec0 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/pfkey.c
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/pfkey.c
@@ -355,7 +355,7 @@ pfkey_inbound(int s)
 	sadb_log(BUNYAN_L_DEBUG, "SADB message received", samsg);
 
 	/*
-	 * XXX KEBE SAYS for now don't print the full inbound message.  An
+	 * For now don't print the full inbound message.  An
 	 * "ipseckey monitor" instance is useful here.
 	 */
 
diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/pkt.c b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/pkt.c
index 89c2b883cd..3a7b4bf64c 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/pkt.c
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/pkt.c
@@ -323,6 +323,14 @@ pkt_add_index(pkt_t *pkt, uint8_t type, uint8_t *buf, uint16_t buflen)
 	return (B_TRUE);
 }
 
+/*
+ * Add a notify index to pkt.  spi, doi, proto, type, buf/buflen are the
+ * values from the notification payload.  Note: for IKEV2, doi is always
+ * 0 (as the field doesn't exist in IKEv2).
+ *
+ * Returns B_TRUE if the index was added, B_FALSE if it could not be added
+ * (i.e. no memory).
+ */
 boolean_t
 pkt_add_nindex(pkt_t *pkt, uint64_t spi, uint32_t doi, uint8_t proto,
     uint16_t type, uint8_t *buf, size_t buflen)
@@ -783,14 +791,14 @@ pay_to_idx(pkt_t *pkt, pkt_payload_t *pay)
 pkt_payload_t *
 pkt_get_payload(pkt_t *pkt, uint8_t type, pkt_payload_t *start)
 {
-	size_t idx = (start == NULL) ? 0 : pay_to_idx(pkt, start);
+	size_t idx = 0;
 
 	/*
 	 * If we're searching for the next payload of 'type', we want to
-	 * being searching after 'start'.
+	 * begin searching with the first payload after 'start'.
 	 */
 	if (start != NULL)
-		idx++;
+		idx = pay_to_idx(pkt, start) + 1;
 
 	for (size_t i = idx; i < pkt->pkt_payload_count; i++) {
 		pkt_payload_t *pay = pkt_payload(pkt, i);
@@ -829,28 +837,28 @@ notify_to_idx(pkt_t *pkt, pkt_notify_t *n)
  * NULL, return the first notify payload of type 'type'.  This allows one
  * to iterate through multiple instances of a given notify type using something
  * such as:
- * 	pkt_notify_t *n;
- * 	...
- * 	for (n = pkt_get_notify(pkt, IKEV2_N_NAT_DETECTION_SOURCE_IP, NULL);
- * 	    n != NULL;
- * 	    n = pkt_get_notify(pkt, IKEV2_N_NAT_DETECTION_SOURCE_IP, n)) {
- * 		....
- * 	}
+ *	pkt_notify_t *n;
+ *	...
+ *	for (n = pkt_get_notify(pkt, IKEV2_N_NAT_DETECTION_SOURCE_IP, NULL);
+ *	    n != NULL;
+ *	    n = pkt_get_notify(pkt, IKEV2_N_NAT_DETECTION_SOURCE_IP, n)) {
+ *		....
+ *	}
  *
- * It is a fatal error to pass in a notify in 'start' that does not exist
- * in pkt.
+ * It is a bug to pass in a non-NULL notify pointer in 'start' that does not
+ * exist in pkt.
  */
 pkt_notify_t *
 pkt_get_notify(pkt_t *pkt, uint16_t type, pkt_notify_t *start)
 {
-	size_t idx = notify_to_idx(pkt, start);
+	size_t idx = 0;
 
 	/*
-	 * If we're looking for the next instance of 'type', we need to
-	 * begin our search after the previous value returned (start).
+	 * If we're searching for the next notification of 'type', begin with
+	 * the first notify payload after start.
 	 */
 	if (start != NULL)
-		idx++;
+		idx = notify_to_idx(pkt, start) + 1;
 
 	for (uint16_t i = idx; i < pkt->pkt_notify_count; i++) {
 		pkt_notify_t *n = pkt_notify(pkt, i);
diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/preshared.c b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/preshared.c
index 63d9a9a53a..49f3979a88 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/preshared.c
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/preshared.c
@@ -22,12 +22,13 @@
  * Copyright 2010 Sun Microsystems, Inc.  All rights reserved.
  * Use is subject to license terms.
  *
- * Copyright (c) 2017, Joyent, Inc.
+ * Copyright (c) 2018, Joyent, Inc.
  */
 
 #include <sys/types.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
 #include <strings.h>
 #include <errno.h>
 #include <ctype.h>
@@ -698,12 +699,7 @@ parsekey(char *input, uint_t *keybuflen, uint_t *lbits, char **errp)
 static boolean_t
 check_if_v6(char *abuf)
 {
-	char *cp;
-	for (cp = abuf; *cp != CHR_NULL; cp++) {
-		if (*cp == CHR_COLON)
-			return (B_TRUE);
-	}
-	return (B_FALSE);
+	return (strchr(abuf, CHR_COLON) != NULL ? B_TRUE : B_FALSE);
 }
 
 static boolean_t
diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/prf.c b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/prf.c
index 165ffdd127..87cdcc97f3 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/prf.c
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/prf.c
@@ -11,6 +11,7 @@
 
 /*
  * Copyright 2014 Jason King.
+ * Copyright (c) 2018, Joyent, Inc.
  */
 #include <bunyan.h>
 #include <limits.h>
@@ -36,7 +37,7 @@
  * generate a seed value.   The prfp+ function starts with this seed value
  * and iteratively uses the previous values to generate new blocks of keying
  * material.  For child SAs (either AH, ESP, or an IKE SA rekey), some
- * additional inputs are mixed in.  RFC7206 secions 2.13 and 2.17 go into
+ * additional inputs are mixed in.  RFC7296 secions 2.13 and 2.17 go into
  * the complete details of what inputs are used when.
  *
  * As both the PRF and prfp+ functions use multiple disparate inputs that
diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ts.c b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ts.c
index 605d59d8bb..d8610944d7 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ts.c
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ts.c
@@ -571,10 +571,7 @@ range_contains(const range_t *restrict r1, const range_t *restrict r2)
 	if ((r1->ra_proto != 0) && (r1->ra_proto != r2->ra_proto))
 		return (B_FALSE);
 
-	if (!range_contains_addr(r1, r2))
-		return (B_FALSE);
-
-	return (range_contains_port(r1, r2));
+	return (range_contains_addr(r1, r2) && range_contains_port(r1, r2));
 }
 
 static int
diff --git a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ts.h b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ts.h
index 57545ae292..7e310ecbb4 100644
--- a/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ts.h
+++ b/usr/src/cmd/cmd-inet/usr.lib/in.ikev2/common/ts.h
@@ -10,7 +10,7 @@
  */
 
 /*
- * Copyright 2017 Joyent, Inc.
+ * Copyright (c) 2018, Joyent, Inc.
  */
 
 #ifndef _TS_H
@@ -38,7 +38,7 @@ enum bunyan_level;
  * sadb_address_t except that the total size of this structure is fixed.
  */
 typedef struct ts_s {
-	uint8_t ts_proto;
+	uint8_t ts_proto;		/* TCP, UDP, etc. */
 	uint8_t	ts_prefix;
 	union {
 		struct sockaddr_in	tsu_sin;