From c3e8d0c9454835074ebcc6971618b2e71bb948c4 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 5 Nov 2013 14:18:07 +0100 Subject: [PATCH 1/7] reconnect: do not initialize iscsi to old_iscsi, use old_iscsi if appropriate Makes it clearer that logging has to be done on the existing context, since the "iscsi" pointer will not survive iscsi_reconnect. Signed-off-by: Paolo Bonzini --- lib/connect.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/connect.c b/lib/connect.c index 271ce2b..50f00e1 100644 --- a/lib/connect.c +++ b/lib/connect.c @@ -231,23 +231,23 @@ void iscsi_defer_reconnect(struct iscsi_context *iscsi) int iscsi_reconnect(struct iscsi_context *old_iscsi) { - struct iscsi_context *iscsi = old_iscsi; + struct iscsi_context *iscsi; int retry = 0; /* if there is already a deferred reconnect do not try again */ - if (iscsi->reconnect_deferred) { - ISCSI_LOG(iscsi, 2, "reconnect initiated, but reconnect is already deferred"); + if (old_iscsi->reconnect_deferred) { + ISCSI_LOG(old_iscsi, 2, "reconnect initiated, but reconnect is already deferred"); return -1; } - ISCSI_LOG(iscsi, 2, "reconnect initiated"); + ISCSI_LOG(old_iscsi, 2, "reconnect initiated"); /* This is mainly for tests, where we do not want to automatically reconnect but rather want the commands to fail with an error if the target drops the session. */ - if (iscsi->no_auto_reconnect) { - iscsi_defer_reconnect(iscsi); + if (old_iscsi->no_auto_reconnect) { + iscsi_defer_reconnect(old_iscsi); return 0; } @@ -302,7 +302,7 @@ try_again: if (backoff > 30) { backoff = 30; } - ISCSI_LOG(iscsi, 1, "reconnect try %d failed, waiting %d seconds", retry, backoff); + ISCSI_LOG(old_iscsi, 1, "reconnect try %d failed, waiting %d seconds", retry, backoff); iscsi_destroy_context(iscsi); sleep(backoff); retry++; @@ -370,11 +370,11 @@ try_again: iscsi->mallocs+=old_iscsi->mallocs; iscsi->frees+=old_iscsi->frees; - ISCSI_LOG(iscsi, 2, "reconnect was successful"); - memcpy(old_iscsi, iscsi, sizeof(struct iscsi_context)); free(iscsi); + ISCSI_LOG(old_iscsi, 2, "reconnect was successful"); + old_iscsi->is_reconnecting = 0; old_iscsi->last_reconnect = time(NULL); From 1acce4a94892ffc8fb75945fc2d2ee691b66a524 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 5 Nov 2013 14:18:51 +0100 Subject: [PATCH 2/7] log failures (typically malloc) of iscsi_create_context during reconnect Signed-off-by: Paolo Bonzini --- lib/connect.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/connect.c b/lib/connect.c index 50f00e1..4d4ea0e 100644 --- a/lib/connect.c +++ b/lib/connect.c @@ -258,6 +258,11 @@ int iscsi_reconnect(struct iscsi_context *old_iscsi) try_again: iscsi = iscsi_create_context(old_iscsi->initiator_name); + if (!iscsi) { + ISCSI_LOG(old_iscsi, 2, "failed to create new context for reconnection"); + return -1; + } + iscsi->is_reconnecting = 1; iscsi_set_targetname(iscsi, old_iscsi->target_name); From dbaa0b4ea6a4ca11d279a4264457ccb1ad4f67ad Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 5 Nov 2013 14:20:01 +0100 Subject: [PATCH 3/7] exit after malloc failure when allocating sense data blob Signed-off-by: Paolo Bonzini --- lib/iscsi-command.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/iscsi-command.c b/lib/iscsi-command.c index 90d3fdb..f86ccaa 100644 --- a/lib/iscsi-command.c +++ b/lib/iscsi-command.c @@ -422,6 +422,7 @@ iscsi_process_scsi_reply(struct iscsi_context *iscsi, struct iscsi_pdu *pdu, if (task->datain.data == NULL) { iscsi_set_error(iscsi, "failed to allocate blob for " "sense data"); + break; } memcpy(task->datain.data, in->data, task->datain.size); From fce94c81a3191a2ba884dcc7a5822b5776004b4f Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 5 Nov 2013 14:20:24 +0100 Subject: [PATCH 4/7] do not test arrays against NULL Signed-off-by: Paolo Bonzini --- lib/login.c | 4 ++-- src/iscsi-inq.c | 2 +- src/iscsi-ls.c | 2 +- src/iscsi-readcapacity16.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/login.c b/lib/login.c index d3bce65..aebb084 100644 --- a/lib/login.c +++ b/lib/login.c @@ -780,7 +780,7 @@ iscsi_login_async(struct iscsi_context *iscsi, iscsi_command_cb cb, } /* optional alias */ - if (iscsi->alias) { + if (iscsi->alias[0]) { if (iscsi_login_add_alias(iscsi, pdu) != 0) { iscsi_free_pdu(iscsi, pdu); return -1; @@ -1079,7 +1079,7 @@ iscsi_process_login_reply(struct iscsi_context *iscsi, struct iscsi_pdu *pdu, size -= len + 1; } - if (status == SCSI_STATUS_REDIRECT && iscsi->target_address) { + if (status == SCSI_STATUS_REDIRECT && iscsi->target_address[0]) { ISCSI_LOG(iscsi, 2, "target requests redirect to %s",iscsi->target_address); pdu->callback(iscsi, SCSI_STATUS_REDIRECT, NULL, pdu->private_data); diff --git a/src/iscsi-inq.c b/src/iscsi-inq.c index 56e4903..ebc6e62 100644 --- a/src/iscsi-inq.c +++ b/src/iscsi-inq.c @@ -326,7 +326,7 @@ int main(int argc, char *argv[]) iscsi_set_session_type(iscsi, ISCSI_SESSION_NORMAL); iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C); - if (iscsi_url->user != NULL) { + if (iscsi_url->user[0]) { if (iscsi_set_initiator_username_pwd(iscsi, iscsi_url->user, iscsi_url->passwd) != 0) { fprintf(stderr, "Failed to set initiator username and password\n"); exit(10); diff --git a/src/iscsi-ls.c b/src/iscsi-ls.c index eb0912c..a7f9013 100644 --- a/src/iscsi-ls.c +++ b/src/iscsi-ls.c @@ -425,7 +425,7 @@ int main(int argc, char *argv[]) iscsi_set_session_type(iscsi, ISCSI_SESSION_DISCOVERY); - if (iscsi_url->user != NULL) { + if (iscsi_url->user[0]) { state.username = iscsi_url->user; state.password = iscsi_url->passwd; if (iscsi_set_initiator_username_pwd(iscsi, iscsi_url->user, iscsi_url->passwd) != 0) { diff --git a/src/iscsi-readcapacity16.c b/src/iscsi-readcapacity16.c index 8749088..7a2adf7 100644 --- a/src/iscsi-readcapacity16.c +++ b/src/iscsi-readcapacity16.c @@ -149,7 +149,7 @@ int main(int argc, char *argv[]) iscsi_set_session_type(iscsi, ISCSI_SESSION_NORMAL); iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C); - if (iscsi_url->user != NULL) { + if (iscsi_url->user[0]) { if (iscsi_set_initiator_username_pwd(iscsi, iscsi_url->user, iscsi_url->passwd) != 0) { fprintf(stderr, "Failed to set initiator username and password\n"); exit(10); From bb0e59055aeb69b0b035e2c2f848d8e82c96eade Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 5 Nov 2013 14:21:45 +0100 Subject: [PATCH 5/7] handle bad iscsi->fd in iscsi_service Just do nothing if the file descriptor is invalid. Signed-off-by: Paolo Bonzini --- lib/socket.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/socket.c b/lib/socket.c index da8ff9d..1d40584 100644 --- a/lib/socket.c +++ b/lib/socket.c @@ -721,6 +721,9 @@ iscsi_service_reconnect_if_loggedin(struct iscsi_context *iscsi) int iscsi_service(struct iscsi_context *iscsi, int revents) { + if (iscsi->fd < 0) + return 0; + if (revents & POLLERR) { int err = 0; socklen_t err_size = sizeof(err); @@ -755,7 +758,7 @@ iscsi_service(struct iscsi_context *iscsi, int revents) return iscsi_service_reconnect_if_loggedin(iscsi); } - if (iscsi->is_connected == 0 && iscsi->fd != -1 && revents&POLLOUT) { + if (iscsi->is_connected == 0 && revents&POLLOUT) { int err = 0; socklen_t err_size = sizeof(err); struct sockaddr_in local; From bfde49756524bd3748234dc6dfa8015e37176e9b Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 5 Nov 2013 14:23:58 +0100 Subject: [PATCH 6/7] rework login and discovery code to avoid strlen beyond end of data Checking for the presence of the NUL character should be done without accessing beyond the PDU datain. Use memchr instead of strlen, and compute the length only if a NUL character is actually there. Signed-off-by: Paolo Bonzini --- lib/discovery.c | 31 ++++++++++++++++++++----------- lib/login.c | 34 ++++++++++++++++++++++------------ 2 files changed, 42 insertions(+), 23 deletions(-) diff --git a/lib/discovery.c b/lib/discovery.c index 52b7762..e12a15a 100644 --- a/lib/discovery.c +++ b/lib/discovery.c @@ -118,25 +118,34 @@ iscsi_process_text_reply(struct iscsi_context *iscsi, struct iscsi_pdu *pdu, pdu->private_data); return -1; } + if (size == 0) { + iscsi_set_error(iscsi, "size == 0 when parsing " + "discovery data"); + pdu->callback(iscsi, SCSI_STATUS_ERROR, NULL, + pdu->private_data); + return -1; + } - while (size > 0) { + do { + unsigned char *end; int len; - len = strlen((char *)ptr); - - if (len == 0) { - break; - } - - if (len > size) { - iscsi_set_error(iscsi, "len > size when parsing " - "discovery data %d>%d", len, size); + end = memchr(ptr, 0, size); + if (end == NULL) { + iscsi_set_error(iscsi, "NUL not found after offset %ld " + "when parsing discovery data", + ptr - in->data); pdu->callback(iscsi, SCSI_STATUS_ERROR, NULL, pdu->private_data); iscsi_free_discovery_addresses(iscsi, targets); return -1; } + len = end - ptr; + if (len == 0) { + break; + } + /* parse the strings */ if (!strncmp((char *)ptr, "TargetName=", 11)) { struct iscsi_discovery_address *target; @@ -187,7 +196,7 @@ iscsi_process_text_reply(struct iscsi_context *iscsi, struct iscsi_pdu *pdu, ptr += len + 1; size -= len + 1; - } + } while (size > 0); pdu->callback(iscsi, SCSI_STATUS_GOOD, targets, pdu->private_data); iscsi_free_discovery_addresses(iscsi, targets); diff --git a/lib/login.c b/lib/login.c index aebb084..9a7347f 100644 --- a/lib/login.c +++ b/lib/login.c @@ -981,30 +981,40 @@ iscsi_process_login_reply(struct iscsi_context *iscsi, struct iscsi_pdu *pdu, if (iscsi_serial32_compare(expcmdsn, iscsi->expcmdsn) > 0) { iscsi->expcmdsn = expcmdsn; } - + + if (size == 0) { + iscsi_set_error(iscsi, "size == 0 when parsing " + "login data"); + pdu->callback(iscsi, SCSI_STATUS_ERROR, NULL, + pdu->private_data); + return -1; + } + /* XXX here we should parse the data returned in case the target * renegotiated some some parameters. * we should also do proper handshaking if the target is not yet * prepared to transition to the next stage */ - while (size > 0) { + do { + char *end; int len; - len = strlen(ptr); - - if (len == 0) { - break; - } - - if (len > size) { - iscsi_set_error(iscsi, "len > size when parsing " - "login data %d>%d", len, size); + end = memchr(ptr, 0, size); + if (end == NULL) { + iscsi_set_error(iscsi, "NUL not found after offset %ld " + "when parsing login data", + (unsigned char *)ptr - in->data); pdu->callback(iscsi, SCSI_STATUS_ERROR, NULL, pdu->private_data); return -1; } + len = end - ptr; + if (len == 0) { + break; + } + /* parse the strings */ if (!strncmp(ptr, "TargetAddress=", 14)) { strncpy(iscsi->target_address,ptr+14,MAX_STRING_SIZE); @@ -1077,7 +1087,7 @@ iscsi_process_login_reply(struct iscsi_context *iscsi, struct iscsi_pdu *pdu, ptr += len + 1; size -= len + 1; - } + } while (size > 0); if (status == SCSI_STATUS_REDIRECT && iscsi->target_address[0]) { ISCSI_LOG(iscsi, 2, "target requests redirect to %s",iscsi->target_address); From 87ee6456217debfbb9a0180933ed84281e45a705 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 5 Nov 2013 14:24:28 +0100 Subject: [PATCH 7/7] check for a target being there before processing TargetAddress Otherwise we access a NULL pointer. RFC3270 appendix D confirms that TargetName must always come before TargetAddress. Signed-off-by: Paolo Bonzini --- lib/discovery.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/discovery.c b/lib/discovery.c index e12a15a..8301bea 100644 --- a/lib/discovery.c +++ b/lib/discovery.c @@ -175,6 +175,14 @@ iscsi_process_text_reply(struct iscsi_context *iscsi, struct iscsi_pdu *pdu, target->next = targets; targets = target; } else if (!strncmp((char *)ptr, "TargetAddress=", 14)) { + if (targets == NULL || targets->target_address != NULL) { + iscsi_set_error(iscsi, "Invalid discovery " + "reply"); + pdu->callback(iscsi, SCSI_STATUS_ERROR, NULL, + pdu->private_data); + iscsi_free_discovery_addresses(iscsi, targets); + return -1; + } targets->target_address = iscsi_strdup(iscsi, (char *)ptr+14); if (targets->target_address == NULL) { iscsi_set_error(iscsi, "Failed to allocate "