Commit f3fd56fc authored by Tomasz Zawadzki's avatar Tomasz Zawadzki Committed by Jim Harris
Browse files

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: default avatarTomasz Zawadzki <tomasz.zawadzki@intel.com>
Signed-off-by: default avatarJim Harris <james.r.harris@intel.com>
Change-Id: I2605293daf171633a45132d7b5532fdfc9128aff
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/6319


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarZiye Yang <ziye.yang@intel.com>
Reviewed-by: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
Reviewed-by: default avatarPaul Luse <paul.e.luse@intel.com>
parent ae92e7f5

lib/iscsi/param.c

+10 −0
Original line number Diff line number Diff line
@@ -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

+38 −0
Original line number Diff line number Diff line
@@ -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

+8 −0
Original line number Diff line number Diff line
@@ -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");