Commit 47ecfb11 authored by Dor Deri's avatar Dor Deri Committed by Tomasz Zawadzki
Browse files

nvmf: validate NSID for Get and Set features commands 



Add validation logic for the Namespace Identifier (NSID) field in both
Get Features and Set Features commands.
The implementation ensures that NSID values are checked according to the
rules defined in the NVMe spec r1.4, Section 7.8 ("Feature Values")

This commit enforces that behavior by validating NSID and returning the
appropriate error when misused.

Change-Id: I2338b107031661b2664038dc07f0ab61515abe91
Signed-off-by: default avatarDor Deri <dor.deri@dell.com>
Reviewed-on: https://review.spdk.io/c/spdk/spdk/+/26534


Community-CI: Mellanox Build Bot
Reviewed-by: default avatarJim Harris <jim.harris@nvidia.com>
Reviewed-by: default avatarChangpeng Liu <changpeliu@tencent.com>
Tested-by: default avatarSPDK Automated Test System <spdkbot@gmail.com>
parent 2d5bb799

lib/nvmf/ctrlr.c

+51 −0
Original line number Diff line number Diff line
@@ -3602,6 +3602,12 @@ nvmf_ctrlr_get_features(struct spdk_nvmf_request *req) 


	feature = cmd->cdw10_bits.get_features.fid;



	if ((cmd->nsid > ctrlr->subsys->max_nsid) && (cmd->nsid != SPDK_NVME_GLOBAL_NS_TAG)) {

		SPDK_ERRLOG("Get Features command with invalid NSID %u, feature ID 0x%02x\n", cmd->nsid, feature);

		response->status.sc = SPDK_NVME_SC_INVALID_NAMESPACE_OR_FORMAT;

		return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE;

	}



	if (spdk_nvmf_subsystem_is_discovery(ctrlr->subsys)) {

		/*

		 * Features supported by Discovery controller
@@ -3679,6 +3685,36 @@ nvmf_ctrlr_get_features(struct spdk_nvmf_request *req) 
	}

}



static bool

is_feature_ctrlr_scope(uint8_t feature)

{

	switch (feature) {

	case SPDK_NVME_FEAT_ARBITRATION:

	case SPDK_NVME_FEAT_POWER_MANAGEMENT:

	case SPDK_NVME_FEAT_TEMPERATURE_THRESHOLD:

	case SPDK_NVME_FEAT_VOLATILE_WRITE_CACHE:

	case SPDK_NVME_FEAT_NUMBER_OF_QUEUES:

	case SPDK_NVME_FEAT_INTERRUPT_COALESCING:

	case SPDK_NVME_FEAT_INTERRUPT_VECTOR_CONFIGURATION:

	case SPDK_NVME_FEAT_ASYNC_EVENT_CONFIGURATION:

	case SPDK_NVME_FEAT_AUTONOMOUS_POWER_STATE_TRANSITION:

	case SPDK_NVME_FEAT_HOST_MEM_BUFFER:

	case SPDK_NVME_FEAT_TIMESTAMP:

	case SPDK_NVME_FEAT_KEEP_ALIVE_TIMER:

	case SPDK_NVME_FEAT_HOST_CONTROLLED_THERMAL_MANAGEMENT:

	case SPDK_NVME_FEAT_NON_OPERATIONAL_POWER_STATE_CONFIG:

	case SPDK_NVME_FEAT_HOST_BEHAVIOR_SUPPORT:

	case SPDK_NVME_FEAT_IO_COMMAND_SET_PROFILE:

	case SPDK_NVME_FEAT_ENHANCED_CONTROLLER_METADATA:

	case SPDK_NVME_FEAT_CONTROLLER_METADATA:

	case SPDK_NVME_FEAT_SOFTWARE_PROGRESS_MARKER:

	case SPDK_NVME_FEAT_HOST_IDENTIFIER:

		return true;

	default:

		return false;

	}

}



static int

nvmf_ctrlr_set_features(struct spdk_nvmf_request *req)

{
@@ -3700,6 +3736,12 @@ nvmf_ctrlr_set_features(struct spdk_nvmf_request *req) 


	feature = cmd->cdw10_bits.set_features.fid;



	if ((cmd->nsid > ctrlr->subsys->max_nsid) && (cmd->nsid != SPDK_NVME_GLOBAL_NS_TAG)) {

		SPDK_ERRLOG("Set Features command with invalid NSID %u, feature ID 0x%02x\n", cmd->nsid, feature);

		response->status.sc = SPDK_NVME_SC_INVALID_NAMESPACE_OR_FORMAT;

		return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE;

	}



	if (spdk_nvmf_subsystem_is_discovery(ctrlr->subsys)) {

		/*

		 * Features supported by Discovery controller
@@ -3748,6 +3790,15 @@ nvmf_ctrlr_set_features(struct spdk_nvmf_request *req) 
		break;

	}



	if ((cmd->nsid < ctrlr->subsys->max_nsid) && (cmd->nsid != 0)) {

		if (is_feature_ctrlr_scope(feature)) {

			SPDK_ERRLOG("Set Feature Controller scope with valid NSID. feature ID 0x%02x, NSID %u\n",

				    feature, cmd->nsid);

			response->status.sc = SPDK_NVME_SC_FEATURE_NOT_NAMESPACE_SPECIFIC;

			return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE;

		}

	}



	switch (feature) {

	case SPDK_NVME_FEAT_ARBITRATION:

		return nvmf_ctrlr_set_features_arbitration(req);

test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c

+28 −0
Original line number Diff line number Diff line
@@ -1410,6 +1410,15 @@ test_set_get_features(void) 
	CU_ASSERT(rsp.nvme_cpl.status.sct == SPDK_NVME_SCT_GENERIC);

	CU_ASSERT(rsp.nvme_cpl.status.sc == SPDK_NVME_SC_INVALID_FIELD);



	/* Get SPDK_NVME_FEAT_TEMPERATURE_THRESHOLD - invalid NSID */

	cmd.nvme_cmd.opc = SPDK_NVME_OPC_GET_FEATURES;

	cmd.nvme_cmd.cdw10_bits.set_features.fid = SPDK_NVME_FEAT_TEMPERATURE_THRESHOLD;

	cmd.nvme_cmd.nsid = SPDK_COUNTOF(ns_arr) + 1;



	rc = nvmf_ctrlr_get_features(&req);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);

	CU_ASSERT(rsp.nvme_cpl.status.sc == SPDK_NVME_SC_INVALID_NAMESPACE_OR_FORMAT);



	/* Set SPDK_NVME_FEAT_TEMPERATURE_THRESHOLD - valid TMPSEL */

	cmd.nvme_cmd.opc = SPDK_NVME_OPC_SET_FEATURES;

	cmd.nvme_cmd.cdw11 = 0x42;
@@ -1422,6 +1431,7 @@ test_set_get_features(void) 
	cmd.nvme_cmd.opc = SPDK_NVME_OPC_SET_FEATURES;

	cmd.nvme_cmd.cdw11 = 0x42 | 1 << 16 | 1 << 19; /* Set reserved value */

	cmd.nvme_cmd.cdw10_bits.set_features.fid = SPDK_NVME_FEAT_TEMPERATURE_THRESHOLD;

	cmd.nvme_cmd.nsid = 0;



	rc = nvmf_ctrlr_set_features(&req);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);
@@ -1467,6 +1477,24 @@ test_set_get_features(void) 
	rc = nvmf_ctrlr_set_features(&req);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);



	/* Set SPDK_NVME_FEAT_ERROR_RECOVERY - invalid NSID */

	cmd.nvme_cmd.opc = SPDK_NVME_OPC_SET_FEATURES;

	cmd.nvme_cmd.cdw10_bits.set_features.fid = SPDK_NVME_FEAT_ARBITRATION;

	cmd.nvme_cmd.nsid = SPDK_COUNTOF(ns_arr) + 1;



	rc = nvmf_ctrlr_set_features(&req);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);

	CU_ASSERT(rsp.nvme_cpl.status.sc == SPDK_NVME_SC_INVALID_NAMESPACE_OR_FORMAT);



	/* Set SPDK_NVME_FEAT_ARBITRATION - valid NSID for controller scoped feature */

	cmd.nvme_cmd.opc = SPDK_NVME_OPC_SET_FEATURES;

	cmd.nvme_cmd.cdw10_bits.set_features.fid = SPDK_NVME_FEAT_ARBITRATION;

	cmd.nvme_cmd.nsid = 1;



	rc = nvmf_ctrlr_set_features(&req);

	CU_ASSERT(rc == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);

	CU_ASSERT(rsp.nvme_cpl.status.sc == SPDK_NVME_SC_FEATURE_NOT_NAMESPACE_SPECIFIC);



	spdk_bit_array_free(&ctrlr.visible_ns);

}