From c04b2968a6aaa0fa3bc08a92c51a611c8156c66a Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Tue, 23 Aug 2016 10:10:07 -0700 Subject: [PATCH] nvmf: enforce NQN validity at creation time Move the NQN validation into the subsytem creation function, and fix the allowed size to match the spec. The spec is not clear about the allowed NQN size; for now, interpret it as 223 bytes, including the null terminator (222 bytes of actual NQN plus one terminator byte). Change-Id: If9743ab2fe009d9d852e8b03317d9b38d8af18dc Signed-off-by: Daniel Verkamp --- app/nvmf_tgt/conf.c | 30 ------------------- lib/nvmf/subsystem.c | 30 +++++++++++++++++++ lib/nvmf/subsystem.h | 3 +- test/lib/nvmf/subsystem/subsystem_ut.c | 41 +++++++++++++------------- 4 files changed, 52 insertions(+), 52 deletions(-) diff --git a/app/nvmf_tgt/conf.c b/app/nvmf_tgt/conf.c index af677ba0e..91766919b 100644 --- a/app/nvmf_tgt/conf.c +++ b/app/nvmf_tgt/conf.c @@ -314,32 +314,6 @@ attach_cb(void *cb_ctx, struct spdk_pci_device *dev, struct spdk_nvme_ctrlr *ctr } } -static int -spdk_nvmf_validate_nqn(const char *nqn) -{ - size_t len; - - len = strlen(nqn); - if (len > SPDK_NVMF_NQN_MAX_LEN) { - SPDK_ERRLOG("Invalid NQN \"%s\": length %zu > max %d\n", nqn, len, SPDK_NVMF_NQN_MAX_LEN); - return -1; - } - - if (strncasecmp(nqn, "nqn.", 4) != 0) { - SPDK_ERRLOG("Invalid NQN \"%s\": NQN must begin with \"nqn.\".\n", nqn); - return -1; - } - - /* yyyy-mm. */ - if (!(isdigit(nqn[4]) && isdigit(nqn[5]) && isdigit(nqn[6]) && isdigit(nqn[7]) && - nqn[8] == '-' && isdigit(nqn[9]) && isdigit(nqn[10]) && nqn[11] == '.')) { - SPDK_ERRLOG("Invalid date code in NQN \"%s\"\n", nqn); - return -1; - } - - return 0; -} - static int spdk_nvmf_allocate_lcore(uint64_t mask, uint32_t lcore) { @@ -376,10 +350,6 @@ spdk_nvmf_parse_subsystem(struct spdk_conf_section *sp) return -1; } - if (spdk_nvmf_validate_nqn(nqn) != 0) { - return -1; - } - /* Determine which core to assign to the subsystem */ mask = spdk_app_get_core_mask(); lcore = spdk_conf_section_get_intval(sp, "Core"); diff --git a/lib/nvmf/subsystem.c b/lib/nvmf/subsystem.c index 65853bcd7..a6d574847 100644 --- a/lib/nvmf/subsystem.c +++ b/lib/nvmf/subsystem.c @@ -99,6 +99,32 @@ spdk_nvmf_subsystem_poller(void *arg) spdk_nvmf_subsystem_poll(subsystem); } +static bool +spdk_nvmf_valid_nqn(const char *nqn) +{ + size_t len; + + len = strlen(nqn); + if (len >= SPDK_NVMF_NQN_MAX_LEN) { + SPDK_ERRLOG("Invalid NQN \"%s\": length %zu > max %d\n", nqn, len, SPDK_NVMF_NQN_MAX_LEN - 1); + return false; + } + + if (strncasecmp(nqn, "nqn.", 4) != 0) { + SPDK_ERRLOG("Invalid NQN \"%s\": NQN must begin with \"nqn.\".\n", nqn); + return false; + } + + /* yyyy-mm. */ + if (!(isdigit(nqn[4]) && isdigit(nqn[5]) && isdigit(nqn[6]) && isdigit(nqn[7]) && + nqn[8] == '-' && isdigit(nqn[9]) && isdigit(nqn[10]) && nqn[11] == '.')) { + SPDK_ERRLOG("Invalid date code in NQN \"%s\"\n", nqn); + return false; + } + + return true; +} + struct spdk_nvmf_subsystem * nvmf_create_subsystem(int num, const char *name, enum spdk_nvmf_subtype subtype, @@ -106,6 +132,10 @@ nvmf_create_subsystem(int num, const char *name, { struct spdk_nvmf_subsystem *subsystem; + if (!spdk_nvmf_valid_nqn(name)) { + return NULL; + } + subsystem = calloc(1, sizeof(struct spdk_nvmf_subsystem)); if (subsystem == NULL) { return NULL; diff --git a/lib/nvmf/subsystem.h b/lib/nvmf/subsystem.h index 8a7a4fb1b..e710c58ab 100644 --- a/lib/nvmf/subsystem.h +++ b/lib/nvmf/subsystem.h @@ -45,7 +45,6 @@ struct spdk_nvmf_subsystem; struct spdk_nvmf_request; struct nvmf_session; -#define MAX_NQN_SIZE 255 #define MAX_VIRTUAL_NAMESPACE 16 enum spdk_nvmf_subsystem_mode { @@ -119,7 +118,7 @@ struct spdk_nvmf_controller { struct spdk_nvmf_subsystem { uint16_t num; uint32_t lcore; - char subnqn[MAX_NQN_SIZE]; + char subnqn[SPDK_NVMF_NQN_MAX_LEN]; enum spdk_nvmf_subsystem_mode mode; enum spdk_nvmf_subtype subtype; struct nvmf_session *session; diff --git a/test/lib/nvmf/subsystem/subsystem_ut.c b/test/lib/nvmf/subsystem/subsystem_ut.c index 98f65dad9..b914fe53f 100644 --- a/test/lib/nvmf/subsystem/subsystem_ut.c +++ b/test/lib/nvmf/subsystem/subsystem_ut.c @@ -109,33 +109,34 @@ spdk_nvmf_session_poll(struct nvmf_session *session) static void nvmf_test_create_subsystem(void) { - - char correct_name[] = "subsystem1"; - char *nqn_name; + char nqn[256]; struct spdk_nvmf_subsystem *subsystem; - subsystem = nvmf_create_subsystem(1, correct_name, SPDK_NVMF_SUBTYPE_NVME, 1); + + strncpy(nqn, "nqn.2016-06.io.spdk:subsystem1", sizeof(nqn)); + subsystem = nvmf_create_subsystem(1, nqn, SPDK_NVMF_SUBTYPE_NVME, 1); SPDK_CU_ASSERT_FATAL(subsystem != NULL); CU_ASSERT_EQUAL(subsystem->num, 1); CU_ASSERT_EQUAL(subsystem->lcore, 1); - CU_ASSERT_STRING_EQUAL(subsystem->subnqn, correct_name); + CU_ASSERT_STRING_EQUAL(subsystem->subnqn, nqn); + nvmf_delete_subsystem(subsystem); - /* test long name */ - nqn_name = malloc(256); - memset(nqn_name, '\0', 256); - memset(nqn_name, 'a', 255); - subsystem = nvmf_create_subsystem(2, nqn_name, SPDK_NVMF_SUBTYPE_NVME, 2); + /* Longest valid name */ + strncpy(nqn, "nqn.2016-06.io.spdk:", sizeof(nqn)); + memset(nqn + strlen(nqn), 'a', 222 - strlen(nqn)); + nqn[222] = '\0'; + CU_ASSERT(strlen(nqn) == 222); + subsystem = nvmf_create_subsystem(2, nqn, SPDK_NVMF_SUBTYPE_NVME, 2); SPDK_CU_ASSERT_FATAL(subsystem != NULL); - CU_ASSERT_STRING_NOT_EQUAL(subsystem->subnqn, nqn_name); - CU_ASSERT_EQUAL(strlen(subsystem->subnqn) + 1, MAX_NQN_SIZE); - free(nqn_name); + CU_ASSERT_STRING_EQUAL(subsystem->subnqn, nqn); + nvmf_delete_subsystem(subsystem); - nqn_name = malloc(255); - memset(nqn_name, '\0', 255); - memset(nqn_name, 'a', 254); - subsystem = nvmf_create_subsystem(2, nqn_name, SPDK_NVMF_SUBTYPE_NVME, 2); - SPDK_CU_ASSERT_FATAL(subsystem != NULL); - CU_ASSERT_STRING_EQUAL(subsystem->subnqn, nqn_name); - free(nqn_name); + /* Name that is one byte longer than allowed */ + strncpy(nqn, "nqn.2016-06.io.spdk:", sizeof(nqn)); + memset(nqn + strlen(nqn), 'a', 223 - strlen(nqn)); + nqn[223] = '\0'; + CU_ASSERT(strlen(nqn) == 223); + subsystem = nvmf_create_subsystem(2, nqn, SPDK_NVMF_SUBTYPE_NVME, 2); + CU_ASSERT(subsystem == NULL); } static void