Commit 8bbc7b69 authored by Pawel Baldysiak's avatar Pawel Baldysiak Committed by Konrad Sztyber
Browse files

nvmf: Block ctrlr-only admin cmds if NSID is set 



According to NVMe spec rev1.3d, Section4, Figure12:
If admin cmd is not using NSID field - host should set it to 0h.
Otherwise - such command should be returned with
INVALID FIELD IN COMMAND error.

With recent change that added passthrough command caps
to nvmf - SPDK engine is forwarding all admin commands
with NSID set to the bdev - even the incorrectly formed ones.
For example - commit firmware with NSID set to 1.

Validate if requested command's opcode is using NSID,
Return appropriate error if not - for request with NSID set.

Fixes issue #3564.

Change-Id: Id2fce050eeaf96ff039073f439d6444ecd55c0b3
Signed-off-by: default avatarPawel Baldysiak <pawel.baldysiak@dell.com>
Signed-off-by: default avatarMarcin Galecki <marcin.galecki@dell.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/25151


Community-CI: Community CI Samsung <spdk.community.ci.samsung@gmail.com>
Community-CI: Mellanox Build Bot
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Community-CI: Broadcom CI <spdk-ci.pdl@broadcom.com>
Reviewed-by: default avatarJim Harris <jim.harris@nvidia.com>
Reviewed-by: default avatarKonrad Sztyber <konrad.sztyber@intel.com>
parent d66a1e46

lib/nvmf/ctrlr.c

+29 −2
Original line number Diff line number Diff line
@@ -3816,6 +3816,30 @@ nvmf_ctrlr_keep_alive(struct spdk_nvmf_request *req) 
	return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE;

}



static bool

is_cmd_ctrlr_specific(struct spdk_nvme_cmd *cmd)

{

	switch (cmd->opc) {

	case SPDK_NVME_OPC_DELETE_IO_SQ:

	case SPDK_NVME_OPC_CREATE_IO_SQ:

	case SPDK_NVME_OPC_DELETE_IO_CQ:

	case SPDK_NVME_OPC_CREATE_IO_CQ:

	case SPDK_NVME_OPC_ABORT:

	case SPDK_NVME_OPC_ASYNC_EVENT_REQUEST:

	case SPDK_NVME_OPC_FIRMWARE_COMMIT:

	case SPDK_NVME_OPC_FIRMWARE_IMAGE_DOWNLOAD:

	case SPDK_NVME_OPC_KEEP_ALIVE:

	case SPDK_NVME_OPC_VIRTUALIZATION_MANAGEMENT:

	case SPDK_NVME_OPC_NVME_MI_SEND:

	case SPDK_NVME_OPC_NVME_MI_RECEIVE:

	case SPDK_NVME_OPC_DOORBELL_BUFFER_CONFIG:

	case SPDK_NVME_OPC_SANITIZE:

		return true;

	default:

		return false;

	}

}



int

nvmf_ctrlr_process_admin_cmd(struct spdk_nvmf_request *req)

{
@@ -3838,8 +3862,11 @@ nvmf_ctrlr_process_admin_cmd(struct spdk_nvmf_request *req) 


	assert(spdk_get_thread() == ctrlr->thread);



	if (cmd->fuse != 0) {

		/* Fused admin commands are not supported. */

	if (cmd->fuse != 0 ||

	    (is_cmd_ctrlr_specific(cmd) && (cmd->nsid != 0))) {

		/* Fused admin commands are not supported.

		 * Commands with controller scope - should be rejected if NSID is set.

		 */

		response->status.sct = SPDK_NVME_SCT_GENERIC;

		response->status.sc = SPDK_NVME_SC_INVALID_FIELD;

		return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE;

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

+11 −3
Original line number Diff line number Diff line
@@ -1021,6 +1021,14 @@ test_get_ns_id_desc_list(void) 
	CU_ASSERT(rsp.nvme_cpl.status.sc == SPDK_NVME_SC_SUCCESS);

	CU_ASSERT(spdk_mem_all_zero(buf, sizeof(buf)));



	/* Valid NSID, but command not using NSID */

	cmd.nvme_cmd.opc = SPDK_NVME_OPC_KEEP_ALIVE;

	memset(&rsp, 0, sizeof(rsp));

	CU_ASSERT(nvmf_ctrlr_process_admin_cmd(&req) == SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE);

	CU_ASSERT(rsp.nvme_cpl.status.sct == SPDK_NVME_SCT_GENERIC);

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

	cmd.nvme_cmd.opc = SPDK_NVME_OPC_IDENTIFY;



	/* Valid NSID, only EUI64 defined */

	ns.opts.eui64[0] = 0x11;

	ns.opts.eui64[7] = 0xFF;
@@ -2123,7 +2131,7 @@ test_multi_async_event_reqs(void) 


	for (i = 0; i < 5; i++) {

		cmd[i].nvme_cmd.opc = SPDK_NVME_OPC_ASYNC_EVENT_REQUEST;

		cmd[i].nvme_cmd.nsid = 1;

		cmd[i].nvme_cmd.nsid = 0;

		cmd[i].nvme_cmd.cid = i;



		req[i].qpair = &qpair;
@@ -2413,7 +2421,7 @@ test_multi_async_events(void) 


	for (i = 0; i < 4; i++) {

		cmd[i].nvme_cmd.opc = SPDK_NVME_OPC_ASYNC_EVENT_REQUEST;

		cmd[i].nvme_cmd.nsid = 1;

		cmd[i].nvme_cmd.nsid = 0;

		cmd[i].nvme_cmd.cid = i;



		req[i].qpair = &qpair;
@@ -2490,7 +2498,7 @@ test_rae(void) 
	req[0].cmd = &cmd[0];

	req[0].rsp = &rsp[0];

	cmd[0].nvme_cmd.opc = SPDK_NVME_OPC_ASYNC_EVENT_REQUEST;

	cmd[0].nvme_cmd.nsid = 1;

	cmd[0].nvme_cmd.nsid = 0;

	cmd[0].nvme_cmd.cid = 0;



	for (i = 1; i < 3; i++) {