From f3fd56fc3c73ee7595bbe7c69cf8048fe2577e1a Mon Sep 17 00:00:00 2001 From: Tomasz Zawadzki Date: Mon, 8 Feb 2021 10:28:44 -0500 Subject: [PATCH] lib/iscsi: return immediately from iscsi_parse_params if len is 0 The spec does not disallow TEXT PDUs with no data. In that case, just return immediately from iscsi_parse_params. This avoids a NULL pointer dereference with a TEXT PDU that has no data, but CONTINUE flag is set. Signed-off-by: Tomasz Zawadzki Signed-off-by: Jim Harris Change-Id: I2605293daf171633a45132d7b5532fdfc9128aff Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/6319 Tested-by: SPDK CI Jenkins Reviewed-by: Ziye Yang Reviewed-by: Shuhei Matsumoto Reviewed-by: Ben Walker Reviewed-by: Paul Luse --- lib/iscsi/param.c | 10 +++++++ test/unit/lib/iscsi/iscsi.c/iscsi_ut.c | 38 ++++++++++++++++++++++++++ test/unit/lib/iscsi/param.c/param_ut.c | 8 ++++++ 3 files changed, 56 insertions(+) diff --git a/lib/iscsi/param.c b/lib/iscsi/param.c index 1a353c4e5..ab2c9cd26 100644 --- a/lib/iscsi/param.c +++ b/lib/iscsi/param.c @@ -315,6 +315,16 @@ iscsi_parse_params(struct iscsi_param **params, const uint8_t *data, char *p; int i; + /* Spec does not disallow TEXT PDUs with zero length, just return + * immediately in that case, since there is no param data to parse + * and any existing partial parameter would remain as-is. + */ + if (len == 0) { + return 0; + } + + assert(data != NULL); + /* strip the partial text parameters if previous PDU have C enabled */ if (partial_parameter && *partial_parameter) { for (i = 0; i < len && data[i] != '\0'; i++) { diff --git a/test/unit/lib/iscsi/iscsi.c/iscsi_ut.c b/test/unit/lib/iscsi/iscsi.c/iscsi_ut.c index 35aaa2632..31087dca7 100644 --- a/test/unit/lib/iscsi/iscsi.c/iscsi_ut.c +++ b/test/unit/lib/iscsi/iscsi.c/iscsi_ut.c @@ -1995,6 +1995,43 @@ pdu_hdr_op_data_test(void) g_task_pool_is_empty = false; } +/* Test an ISCSI_OP_TEXT PDU with CONTINUE bit set but + * no data. + */ +static void +empty_text_with_cbit_test(void) +{ + struct spdk_iscsi_sess sess = {}; + struct spdk_iscsi_conn conn = {}; + struct spdk_scsi_dev dev = {}; + struct spdk_iscsi_pdu *req_pdu; + int rc; + + req_pdu = iscsi_get_pdu(&conn); + + sess.ExpCmdSN = 0; + sess.MaxCmdSN = 64; + sess.session_type = SESSION_TYPE_NORMAL; + sess.MaxBurstLength = 1024; + + conn.full_feature = 1; + conn.sess = &sess; + conn.dev = &dev; + conn.state = ISCSI_CONN_STATE_RUNNING; + + memset(&req_pdu->bhs, 0, sizeof(req_pdu->bhs)); + req_pdu->bhs.opcode = ISCSI_OP_TEXT; + req_pdu->bhs.flags = ISCSI_TEXT_CONTINUE; + + rc = iscsi_pdu_hdr_handle(&conn, req_pdu); + CU_ASSERT(rc == 0); + CU_ASSERT(!req_pdu->is_rejected); + rc = iscsi_pdu_payload_handle(&conn, req_pdu); + CU_ASSERT(rc == 0); + + iscsi_put_pdu(req_pdu); +} + int main(int argc, char **argv) { @@ -2026,6 +2063,7 @@ main(int argc, char **argv) CU_ADD_TEST(suite, pdu_hdr_op_task_mgmt_test); CU_ADD_TEST(suite, pdu_hdr_op_nopout_test); CU_ADD_TEST(suite, pdu_hdr_op_data_test); + CU_ADD_TEST(suite, empty_text_with_cbit_test); CU_basic_set_mode(CU_BRM_VERBOSE); CU_basic_run_tests(); diff --git a/test/unit/lib/iscsi/param.c/param_ut.c b/test/unit/lib/iscsi/param.c/param_ut.c index ccf62643f..9b3e8201f 100644 --- a/test/unit/lib/iscsi/param.c/param_ut.c +++ b/test/unit/lib/iscsi/param.c/param_ut.c @@ -264,6 +264,14 @@ parse_valid_test(void) EXPECT_VAL("F", "IIII"); CU_ASSERT_PTR_NULL(partial_parameter); + /* partial parameter: NULL data */ + /* It is technically allowed to have a TEXT PDU with no data, yet + * CONTINUE bit is enabled - make sure we handle that case correctly. + */ + rc = iscsi_parse_params(¶ms, NULL, 0, true, &partial_parameter); + CU_ASSERT(rc == 0); + CU_ASSERT_PTR_NULL(partial_parameter); + /* Second partial parameter is the only parameter */ PARSE("OOOO", true, &partial_parameter); CU_ASSERT_STRING_EQUAL(partial_parameter, "OOOO");