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 <tomasz.zawadzki@intel.com>
Signed-off-by: Jim Harris <james.r.harris@intel.com>
Change-Id: I2605293daf171633a45132d7b5532fdfc9128aff
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/6319
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ziye Yang <ziye.yang@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Paul Luse <paul.e.luse@intel.com>

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++) {

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();

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(&params, 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");