Commit 4201dfaf authored by Dor Deri's avatar Dor Deri Committed by Tomasz Zawadzki
Browse files

nvmf: enforce spec for invalid get log page offset handling 



Ensure controller aborts command with 'Invalid Field in Command' status
when LPOL/LPOU offset exceeds requested log page size, per spec.

NVMe r1.4 specification says “If the host specifies an offset
(i.e., LPOL and LPOU) that is greater than the size of the log page
requested (e.g., a log page containing 100 bytes is requested starting
at offset 200), then the controller shall abort the command with
a status of Invalid Field in Command.”

Fixes: 7a2a588c (nvmf/ctrlr: check offset parameter for get log page command)

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


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

lib/nvmf/ctrlr.c

+46 −12
Original line number Diff line number Diff line
@@ -2346,6 +2346,10 @@ nvmf_get_firmware_slot_log_page(struct iovec *iovs, int iovcnt, uint64_t offset, 
		if (copy_len > 0) {

			spdk_iov_xfer_from_buf(&ix, (const char *)&fw_page + offset, copy_len);

		}

	} else {

		SPDK_ERRLOG("Invalid Get log page firmware slot offset: (%" PRIu64 "), log page size (%zu)\n",

			    offset, sizeof(fw_page));

		return -EINVAL;

	}



	return 0;
@@ -2441,6 +2445,7 @@ nvmf_get_ana_log_page(struct spdk_nvmf_ctrlr *ctrlr, struct iovec *iovs, int iov 
	struct spdk_nvme_ana_group_descriptor ana_desc;

	size_t copy_len, copied_len;

	uint32_t num_anagrp = 0, anagrpid;

	size_t total_page_size = sizeof(ana_hdr);

	struct spdk_nvmf_ns *ns;

	struct spdk_iov_xfer ix;
@@ -2450,6 +2455,31 @@ nvmf_get_ana_log_page(struct spdk_nvmf_ctrlr *ctrlr, struct iovec *iovs, int iov 
		goto done;

	}



	for (anagrpid = 1; anagrpid <= ctrlr->subsys->max_nsid; anagrpid++) {

		if (ctrlr->subsys->ana_group[anagrpid - 1] == 0) {

			continue;

		}



		total_page_size += sizeof(ana_desc);



		if (rgo) {

			continue;

		}



		for (ns = spdk_nvmf_subsystem_get_first_ns(ctrlr->subsys); ns != NULL;

		     ns = spdk_nvmf_subsystem_get_next_ns(ctrlr->subsys, ns)) {

			if (ns->anagrpid == anagrpid) {

				total_page_size += sizeof(uint32_t);

			}

		}

	}



	if (offset >= total_page_size) {

		SPDK_ERRLOG("Invalid Get log page ana offset: (%" PRIu64 "), log page size (%zu)\n",

			    offset, total_page_size);

		return -EINVAL;

	}



	if (offset >= sizeof(ana_hdr)) {

		offset -= sizeof(ana_hdr);

	} else {
@@ -2579,19 +2609,25 @@ nvmf_get_changed_ns_list_log_page(struct spdk_nvmf_ctrlr *ctrlr, 
{

	size_t copy_length;

	struct spdk_iov_xfer ix;

	size_t page_size = sizeof(ctrlr->changed_ns_list);





	spdk_iov_xfer_init(&ix, iovs, iovcnt);



	if (offset < sizeof(ctrlr->changed_ns_list)) {

		copy_length = spdk_min(length, sizeof(ctrlr->changed_ns_list) - offset);

	if (offset < page_size) {

		copy_length = spdk_min(length, page_size - offset);

		if (copy_length) {

			spdk_iov_xfer_from_buf(&ix, (char *)&ctrlr->changed_ns_list + offset, copy_length);

		}

	} else {

		SPDK_ERRLOG("Invalid Get log page changed ns list offset: (%" PRIu64 "), log page size (%zu)\n",

			    offset, page_size);

		return -EINVAL;

	}



	/* Clear log page each time it is read */

	ctrlr->changed_ns_list_count = 0;

	memset(&ctrlr->changed_ns_list, 0, sizeof(ctrlr->changed_ns_list));

	memset(&ctrlr->changed_ns_list, 0, page_size);



	if (!rae) {

		nvmf_ctrlr_unmask_aen(ctrlr, SPDK_NVME_ASYNC_EVENT_NS_ATTR_CHANGE_MASK_BIT);
@@ -2677,6 +2713,10 @@ nvmf_get_cmds_and_effects_log_page(struct spdk_nvmf_ctrlr *ctrlr, struct iovec * 
	if (offset < page_size) {

		copy_len = spdk_min(page_size - offset, length);

		spdk_iov_xfer_from_buf(&ix, (char *)(&cmds_and_effect_log_page) + offset, copy_len);

	} else {

		SPDK_ERRLOG("Invalid Get log page cmds effects offset: (%" PRIu64 "), log page size (%" PRIu32")\n",

			    offset, page_size);

		return -EINVAL;

	}



	return 0;
@@ -2700,7 +2740,9 @@ nvmf_get_reservation_notification_log_page(struct spdk_nvmf_ctrlr *ctrlr, 


	avail_log_len = ctrlr->num_avail_log_pages * unit_log_len;

	if (offset >= avail_log_len) {

		return 0;

		SPDK_ERRLOG("Invalid Get log page reservation notification offset: (%" PRIu64"), log page size (%"

			    PRIu32")\n", offset, avail_log_len);

		return -EINVAL;

	}



	next_pos = 0;
@@ -2789,14 +2831,6 @@ nvmf_ctrlr_get_log_page(struct spdk_nvmf_request *req) 
			SPDK_INFOLOG(nvmf, "Unsupported Get Log Page Identifier for discovery subsystem 0x%02X\n", lid);

			goto invalid_field_log_page;

		}

	} else {

		if (offset > len) {

			SPDK_ERRLOG("Get log page: offset (%" PRIu64 ") > len (%" PRIu64 ")\n",

				    offset, len);

			response->status.sct = SPDK_NVME_SCT_GENERIC;

			response->status.sc = SPDK_NVME_SC_INVALID_FIELD;

			return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE;

		}

	}



	switch (lid) {

lib/nvmf/ctrlr_discovery.c

+7 −0
Original line number Diff line number Diff line
@@ -215,6 +215,13 @@ nvmf_get_discovery_log_page(struct spdk_nvmf_tgt *tgt, const char *hostnqn, stru 


	discovery_log_page = nvmf_generate_discovery_log(tgt, hostnqn, &log_page_size, cmd_source_trid);



	if (offset >= log_page_size) {

		SPDK_ERRLOG("Invalid Get log page discovery offset: (%" PRIu64 "), log page size (%zu)\n",

			    offset, log_page_size);

		free(discovery_log_page);

		return -EINVAL;

	}



	/* Copy the valid part of the discovery log page, if any */

	if (discovery_log_page) {

		for (tmp = iov; tmp < iov + iovcnt; tmp++) {

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

+11 −0
Original line number Diff line number Diff line
@@ -407,6 +407,17 @@ test_get_log_page(void) 
	CU_ASSERT(req.rsp->nvme_cpl.status.sct == SPDK_NVME_SCT_GENERIC);

	CU_ASSERT(req.rsp->nvme_cpl.status.sc == SPDK_NVME_SC_INVALID_FIELD);



	/* Get Log Page with invalid offset (offset exceeds requested log page size) */

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

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

	cmd.nvme_cmd.opc = SPDK_NVME_OPC_GET_LOG_PAGE;

	cmd.nvme_cmd.cdw10_bits.get_log_page.lid = SPDK_NVME_LOG_FIRMWARE_SLOT;

	cmd.nvme_cmd.cdw10_bits.get_log_page.numdl = spdk_nvme_bytes_to_numd(req.length);

	cmd.nvme_cmd.cdw12 = sizeof(struct spdk_nvme_firmware_page) + 16;

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

	CU_ASSERT(req.rsp->nvme_cpl.status.sct == SPDK_NVME_SCT_GENERIC);

	CU_ASSERT(req.rsp->nvme_cpl.status.sc == SPDK_NVME_SC_INVALID_FIELD);



	/* Get Log Page without data buffer */

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

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