Fix several problems in the new iSCSI stack; this includes interoperability

fix for LIO (Linux target), removing possibility for the target to avoid mutual
CHAP by choosing to skip authentication altogether, and fixing truncated error
messages in iscsictl(8) output.  This also fixes several of the problems found
with Coverity.

Note that this change requires world rebuild.

Coverity CID:	1088038, 1087998, 1087990, 1088004, 1088044, 1088041, 1088040
Approved by:	re (blanket)
Sponsored by:	FreeBSD Foundation

6 files changed, 117 insertions, 74 deletions

diff --git a/sys/dev/iscsi/iscsi.c b/sys/dev/iscsi/iscsi.c
index 0a956e8..0b3d565 100644
--- a/sys/dev/iscsi/iscsi.c
+++ b/sys/dev/iscsi/iscsi.c
@@ -1513,8 +1513,13 @@ iscsi_ioctl_session_add(struct iscsi_softc *sc, struct iscsi_session_add *isa)
 	memcpy(&is->is_conf, &isa->isa_conf, sizeof(is->is_conf));
 
 	if (is->is_conf.isc_initiator[0] == '\0' ||
-	    is->is_conf.isc_target == '\0' ||
-	    is->is_conf.isc_target_addr == '\0') {
+	    is->is_conf.isc_target_addr[0] == '\0') {
+		free(is, M_ISCSI);
+		return (EINVAL);
+	}
+
+	if ((is->is_conf.isc_discovery != 0 && is->is_conf.isc_target[0] != 0) ||
+	    (is->is_conf.isc_discovery == 0 && is->is_conf.isc_target[0] == 0)) {
 		free(is, M_ISCSI);
 		return (EINVAL);
 	}
@@ -1525,11 +1530,22 @@ iscsi_ioctl_session_add(struct iscsi_softc *sc, struct iscsi_session_add *isa)
 	 * Prevent duplicates.
 	 */
 	TAILQ_FOREACH(is2, &sc->sc_sessions, is_next) {
-		if (strcmp(is2->is_conf.isc_target,
-		    is->is_conf.isc_target) == 0) {
-			sx_xunlock(&sc->sc_lock);
-			return (EBUSY);
-		}
+		if (!!is->is_conf.isc_discovery !=
+		    !!is2->is_conf.isc_discovery)
+			continue;
+
+		if (strcmp(is->is_conf.isc_target_addr,
+		    is2->is_conf.isc_target_addr) != 0)
+			continue;
+
+		if (is->is_conf.isc_discovery == 0 &&
+		    strcmp(is->is_conf.isc_target,
+		    is2->is_conf.isc_target) != 0)
+			continue;
+
+		sx_xunlock(&sc->sc_lock);
+		free(is, M_ISCSI);
+		return (EBUSY);
 	}
 
 	is->is_conn = icl_conn_new();
@@ -1936,9 +1952,9 @@ iscsi_action(struct cam_sim *sim, union ccb *ccb)
 		cpi->max_lun = 255;
 		//cpi->initiator_id = 0; /* XXX */
 		cpi->initiator_id = 64; /* XXX */
-		strncpy(cpi->sim_vid, "FreeBSD", SIM_IDLEN);
-		strncpy(cpi->hba_vid, "iSCSI", HBA_IDLEN);
-		strncpy(cpi->dev_name, cam_sim_name(sim), DEV_IDLEN);
+		strlcpy(cpi->sim_vid, "FreeBSD", SIM_IDLEN);
+		strlcpy(cpi->hba_vid, "iSCSI", HBA_IDLEN);
+		strlcpy(cpi->dev_name, cam_sim_name(sim), DEV_IDLEN);
 		cpi->unit_number = cam_sim_unit(sim);
 		cpi->bus_id = cam_sim_bus(sim);
 		cpi->base_transfer_speed = 150000; /* XXX */
diff --git a/sys/dev/iscsi/iscsi_ioctl.h b/sys/dev/iscsi/iscsi_ioctl.h
index 9e27844..61c48cc 100644
--- a/sys/dev/iscsi/iscsi_ioctl.h
+++ b/sys/dev/iscsi/iscsi_ioctl.h
@@ -43,7 +43,7 @@
 #define	ISCSI_ADDR_LEN		47	/* INET6_ADDRSTRLEN + '\0' */
 #define	ISCSI_ALIAS_LEN		256	/* XXX: Where did it come from? */
 #define	ISCSI_SECRET_LEN	17	/* 16 + '\0' */
-#define	ISCSI_REASON_LEN	32
+#define	ISCSI_REASON_LEN	64
 
 #define	ISCSI_DIGEST_NONE	0
 #define	ISCSI_DIGEST_CRC32C	1
diff --git a/usr.sbin/ctld/ctld.c b/usr.sbin/ctld/ctld.c
index 34fd650..a5472ab 100644
--- a/usr.sbin/ctld/ctld.c
+++ b/usr.sbin/ctld/ctld.c
@@ -348,12 +348,10 @@ portal_group_new(struct conf *conf, const char *name)
 {
 	struct portal_group *pg;
 
-	if (name != NULL) {
-		pg = portal_group_find(conf, name);
-		if (pg != NULL) {
-			log_warnx("duplicated portal-group \"%s\"", name);
-			return (NULL);
-		}
+	pg = portal_group_find(conf, name);
+	if (pg != NULL) {
+		log_warnx("duplicated portal-group \"%s\"", name);
+		return (NULL);
 	}
 
 	pg = calloc(1, sizeof(*pg));
diff --git a/usr.sbin/ctld/kernel.c b/usr.sbin/ctld/kernel.c
index 869d0a0..0e00204 100644
--- a/usr.sbin/ctld/kernel.c
+++ b/usr.sbin/ctld/kernel.c
@@ -628,7 +628,7 @@ kernel_port_on(void)
 	struct ctl_port_entry entry;
 	int error;
 
-	bzero(&entry, sizeof(&entry));
+	bzero(&entry, sizeof(entry));
 
 	entry.port_type = CTL_PORT_ISCSI;
 	entry.targ_port = -1;
@@ -648,7 +648,7 @@ kernel_port_off(void)
 	struct ctl_port_entry entry;
 	int error;
 
-	bzero(&entry, sizeof(&entry));
+	bzero(&entry, sizeof(entry));
 
 	entry.port_type = CTL_PORT_ISCSI;
 	entry.targ_port = -1;
diff --git a/usr.sbin/iscsid/iscsid.c b/usr.sbin/iscsid/iscsid.c
index 0547465..c22d7fd 100644
--- a/usr.sbin/iscsid/iscsid.c
+++ b/usr.sbin/iscsid/iscsid.c
@@ -156,7 +156,7 @@ connection_new(unsigned int session_id, const struct iscsi_session_conf *conf,
 	struct addrinfo *from_ai, *to_ai;
 	const char *from_addr, *to_addr;
 #ifdef ICL_KERNEL_PROXY
-	struct iscsi_daemon_connect *idc;
+	struct iscsi_daemon_connect idc;
 #endif
 	int error;
 
@@ -195,25 +195,22 @@ connection_new(unsigned int session_id, const struct iscsi_session_conf *conf,
 
 #ifdef ICL_KERNEL_PROXY
 
-	idc = calloc(1, sizeof(*idc));
-	if (idc == NULL)
-		log_err(1, "calloc");
-
-	idc->idc_session_id = conn->conn_session_id;
+	memset(&idc, 0, sizeof(idc));
+	idc.idc_session_id = conn->conn_session_id;
 	if (conn->conn_conf.isc_iser)
-		idc->idc_iser = 1;
-	idc->idc_domain = to_ai->ai_family;
-	idc->idc_socktype = to_ai->ai_socktype;
-	idc->idc_protocol = to_ai->ai_protocol;
+		idc.idc_iser = 1;
+	idc.idc_domain = to_ai->ai_family;
+	idc.idc_socktype = to_ai->ai_socktype;
+	idc.idc_protocol = to_ai->ai_protocol;
 	if (from_ai != NULL) {
-		idc->idc_from_addr = from_ai->ai_addr;
-		idc->idc_from_addrlen = from_ai->ai_addrlen;
+		idc.idc_from_addr = from_ai->ai_addr;
+		idc.idc_from_addrlen = from_ai->ai_addrlen;
 	}
-	idc->idc_to_addr = to_ai->ai_addr;
-	idc->idc_to_addrlen = to_ai->ai_addrlen;
+	idc.idc_to_addr = to_ai->ai_addr;
+	idc.idc_to_addrlen = to_ai->ai_addrlen;
 
 	log_debugx("connecting to %s using ICL kernel proxy", to_addr);
-	error = ioctl(iscsi_fd, ISCSIDCONNECT, idc);
+	error = ioctl(iscsi_fd, ISCSIDCONNECT, &idc);
 	if (error != 0) {
 		fail(conn, strerror(errno));
 		log_err(1, "failed to connect to %s using ICL kernel proxy",
@@ -257,31 +254,29 @@ connection_new(unsigned int session_id, const struct iscsi_session_conf *conf,
 static void
 handoff(struct connection *conn)
 {
-	struct iscsi_daemon_handoff *idh;
+	struct iscsi_daemon_handoff idh;
 	int error;
 
 	log_debugx("handing off connection to the kernel");
 
-	idh = calloc(1, sizeof(*idh));
-	if (idh == NULL)
-		log_err(1, "calloc");
-	idh->idh_session_id = conn->conn_session_id;
+	memset(&idh, 0, sizeof(idh));
+	idh.idh_session_id = conn->conn_session_id;
 #ifndef ICL_KERNEL_PROXY
-	idh->idh_socket = conn->conn_socket;
+	idh.idh_socket = conn->conn_socket;
 #endif
-	strlcpy(idh->idh_target_alias, conn->conn_target_alias,
-	    sizeof(idh->idh_target_alias));
-	memcpy(idh->idh_isid, conn->conn_isid, sizeof(idh->idh_isid));
-	idh->idh_statsn = conn->conn_statsn;
-	idh->idh_header_digest = conn->conn_header_digest;
-	idh->idh_data_digest = conn->conn_data_digest;
-	idh->idh_initial_r2t = conn->conn_initial_r2t;
-	idh->idh_immediate_data = conn->conn_immediate_data;
-	idh->idh_max_data_segment_length = conn->conn_max_data_segment_length;
-	idh->idh_max_burst_length = conn->conn_max_burst_length;
-	idh->idh_first_burst_length = conn->conn_first_burst_length;
-
-	error = ioctl(conn->conn_iscsi_fd, ISCSIDHANDOFF, idh);
+	strlcpy(idh.idh_target_alias, conn->conn_target_alias,
+	    sizeof(idh.idh_target_alias));
+	memcpy(idh.idh_isid, conn->conn_isid, sizeof(idh.idh_isid));
+	idh.idh_statsn = conn->conn_statsn;
+	idh.idh_header_digest = conn->conn_header_digest;
+	idh.idh_data_digest = conn->conn_data_digest;
+	idh.idh_initial_r2t = conn->conn_initial_r2t;
+	idh.idh_immediate_data = conn->conn_immediate_data;
+	idh.idh_max_data_segment_length = conn->conn_max_data_segment_length;
+	idh.idh_max_burst_length = conn->conn_max_burst_length;
+	idh.idh_first_burst_length = conn->conn_first_burst_length;
+
+	error = ioctl(conn->conn_iscsi_fd, ISCSIDHANDOFF, &idh);
 	if (error != 0)
 		log_err(1, "ISCSIDHANDOFF");
 }
@@ -289,17 +284,14 @@ handoff(struct connection *conn)
 void
 fail(const struct connection *conn, const char *reason)
 {
-	struct iscsi_daemon_fail *idf;
+	struct iscsi_daemon_fail idf;
 	int error;
 
-	idf = calloc(1, sizeof(*idf));
-	if (idf == NULL)
-		log_err(1, "calloc");
-
-	idf->idf_session_id = conn->conn_session_id;
-	strlcpy(idf->idf_reason, reason, sizeof(idf->idf_reason));
+	memset(&idf, 0, sizeof(idf));
+	idf.idf_session_id = conn->conn_session_id;
+	strlcpy(idf.idf_reason, reason, sizeof(idf.idf_reason));
 
-	error = ioctl(conn->conn_iscsi_fd, ISCSIDFAIL, idf);
+	error = ioctl(conn->conn_iscsi_fd, ISCSIDFAIL, &idf);
 	if (error != 0)
 		log_err(1, "ISCSIDFAIL");
 }
@@ -402,7 +394,7 @@ set_timeout(int timeout)
 }
 
 static void
-handle_request(int iscsi_fd, struct iscsi_daemon_request *request, int timeout)
+handle_request(int iscsi_fd, const struct iscsi_daemon_request *request, int timeout)
 {
 	struct connection *conn;
 
@@ -468,7 +460,7 @@ main(int argc, char **argv)
 	struct pidfh *pidfh;
 	pid_t pid, otherpid;
 	const char *pidfile_path = DEFAULT_PIDFILE;
-	struct iscsi_daemon_request *request;
+	struct iscsi_daemon_request request;
 
 	while ((ch = getopt(argc, argv, "P:dl:m:t:")) != -1) {
 		switch (ch) {
@@ -533,11 +525,8 @@ main(int argc, char **argv)
 	for (;;) {
 		log_debugx("waiting for request from the kernel");
 
-		request = calloc(1, sizeof(*request));
-		if (request == NULL)
-			log_err(1, "calloc");
-
-		error = ioctl(iscsi_fd, ISCSIDWAIT, request);
+		memset(&request, 0, sizeof(request));
+		error = ioctl(iscsi_fd, ISCSIDWAIT, &request);
 		if (error != 0) {
 			if (errno == EINTR) {
 				nchildren -= wait_for_children(false);
@@ -573,7 +562,7 @@ main(int argc, char **argv)
 		}
 
 		pidfile_close(pidfh);
-		handle_request(iscsi_fd, request, timeout);
+		handle_request(iscsi_fd, &request, timeout);
 	}
 
 	return (0);
diff --git a/usr.sbin/iscsid/login.c b/usr.sbin/iscsid/login.c
index 2ea47cb..2ae36e8 100644
--- a/usr.sbin/iscsid/login.c
+++ b/usr.sbin/iscsid/login.c
@@ -504,23 +504,37 @@ login_negotiate(struct connection *conn)
 	login_set_csg(request, BHSLR_STAGE_OPERATIONAL_NEGOTIATION);
 	login_set_nsg(request, BHSLR_STAGE_FULL_FEATURE_PHASE);
 	request_keys = keys_new();
+
+	/*
+	 * The following keys are irrelevant for discovery sessions.
+	 */
 	if (conn->conn_conf.isc_discovery == 0) {
 		if (conn->conn_conf.isc_header_digest != 0)
 			keys_add(request_keys, "HeaderDigest", "CRC32C");
+		else
+			keys_add(request_keys, "HeaderDigest", "None");
 		if (conn->conn_conf.isc_data_digest != 0)
 			keys_add(request_keys, "DataDigest", "CRC32C");
+		else
+			keys_add(request_keys, "DataDigest", "None");
 
 		keys_add(request_keys, "ImmediateData", "Yes");
 		keys_add_int(request_keys, "MaxBurstLength",
 		    ISCSI_MAX_DATA_SEGMENT_LENGTH);
 		keys_add_int(request_keys, "FirstBurstLength",
 		    ISCSI_MAX_DATA_SEGMENT_LENGTH);
+		keys_add(request_keys, "InitialR2T", "Yes");
+	} else {
+		keys_add(request_keys, "HeaderDigest", "None");
+		keys_add(request_keys, "DataDigest", "None");
 	}
-	keys_add(request_keys, "InitialR2T", "Yes");
+
 	keys_add_int(request_keys, "MaxRecvDataSegmentLength",
 	    ISCSI_MAX_DATA_SEGMENT_LENGTH);
 	keys_add(request_keys, "DefaultTime2Wait", "0");
 	keys_add(request_keys, "DefaultTime2Retain", "0");
+	keys_add(request_keys, "ErrorRecoveryLevel", "0");
+	keys_add(request_keys, "MaxOutstandingR2T", "1");
 	keys_save(request_keys, request);
 	keys_delete(request_keys);
 	request_keys = NULL;
@@ -776,10 +790,26 @@ login(struct connection *conn)
 	bhslr->bhslr_flags |= BHSLR_FLAGS_TRANSIT;
 
 	request_keys = keys_new();
-	if (conn->conn_conf.isc_user[0] == '\0')
+	if (conn->conn_conf.isc_mutual_user[0] != '\0') {
+		keys_add(request_keys, "AuthMethod", "CHAP");
+	} else if (conn->conn_conf.isc_user[0] != '\0') {
+		/*
+		 * Give target a chance to skip authentication if it
+		 * doesn't feel like it.
+		 *
+		 * None is first, CHAP second; this is to work around
+		 * what seems to be LIO (Linux target) bug: otherwise,
+		 * if target is configured with no authentication,
+		 * and we are configured to authenticate, the target
+		 * will erroneously respond with AuthMethod=CHAP
+		 * instead of AuthMethod=None, and will subsequently
+		 * fail the connection.  This usually happens with
+		 * Discovery sessions, which default to no authentication.
+		 */
+		keys_add(request_keys, "AuthMethod", "None,CHAP");
+	} else {
 		keys_add(request_keys, "AuthMethod", "None");
-	else
-		keys_add(request_keys, "AuthMethod", "CHAP,None");
+	}
 	keys_add(request_keys, "InitiatorName",
 	    conn->conn_conf.isc_initiator);
 	if (conn->conn_conf.isc_initiator_alias[0] != '\0') {
@@ -825,9 +855,14 @@ login(struct connection *conn)
 	bhslr2 = (struct iscsi_bhs_login_response *)response->pdu_bhs;
 	if ((bhslr2->bhslr_flags & BHSLR_FLAGS_TRANSIT) != 0 &&
 	    login_nsg(response) == BHSLR_STAGE_OPERATIONAL_NEGOTIATION) {
+		if (conn->conn_conf.isc_mutual_user[0] != '\0') {
+			log_errx(1, "target requested transition "
+			    "to operational negotiation, but we require "
+			    "mutual CHAP");
+		}
+
 		log_debugx("target requested transition "
 		    "to operational negotiation");
-
 		keys_delete(response_keys);
 		pdu_delete(response);
 		login_negotiate(conn);
@@ -838,6 +873,11 @@ login(struct connection *conn)
 	if (auth_method == NULL)
 		log_errx(1, "received response without AuthMethod");
 	if (strcmp(auth_method, "None") == 0) {
+		if (conn->conn_conf.isc_mutual_user[0] != '\0') {
+			log_errx(1, "target does not require authantication, "
+			    "but we require mutual CHAP");
+		}
+
 		log_debugx("target does not require authentication");
 		keys_delete(response_keys);
 		pdu_delete(response);