From af9c9a415ac7d632251bb46aa6e6cbd1f01a1c61 Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Mon, 9 Apr 2018 10:19:08 -0700 Subject: [PATCH] scsi: validate dev name length Instead of silently truncating overly-long SCSI dev names, add an explicit check and return an error if the name is too long. Since we now calculate the length of name up front, this also allows simplification of the copy into dev->name. This also ensures that dev->name will always be zero-terminated, so we can simplify the strnlen() in spdk_bdev_scsi_pad_scsi_name() to a strlen() and remove the invalid unit test case for padding names longer than SPDK_SCSI_DEV_MAX_NAME. Change-Id: I54de00bac062a142a10c41cfa2aec19d7969dff0 Signed-off-by: Daniel Verkamp Reviewed-on: https://review.gerrithub.io/406990 Tested-by: SPDK Automated Test System Reviewed-by: Ben Walker Reviewed-by: Jim Harris --- lib/scsi/dev.c | 10 +++++++++- lib/scsi/scsi_bdev.c | 2 +- test/unit/lib/scsi/dev.c/dev_ut.c | 19 +++++++++++++++++++ test/unit/lib/scsi/scsi_bdev.c/scsi_bdev_ut.c | 11 +---------- 4 files changed, 30 insertions(+), 12 deletions(-) diff --git a/lib/scsi/dev.c b/lib/scsi/dev.c index aa5f1d5dc..c86eaee8a 100644 --- a/lib/scsi/dev.c +++ b/lib/scsi/dev.c @@ -185,9 +185,17 @@ spdk_scsi_dev_construct(const char *name, const char *bdev_name_list[], void *hotremove_ctx) { struct spdk_scsi_dev *dev; + size_t name_len; bool found_lun_0; int i, rc; + name_len = strlen(name); + if (name_len > sizeof(dev->name) - 1) { + SPDK_ERRLOG("device %s: name longer than maximum allowed length %zu\n", + name, sizeof(dev->name) - 1); + return NULL; + } + if (num_luns == 0) { SPDK_ERRLOG("device %s: no LUNs specified\n", name); return NULL; @@ -219,7 +227,7 @@ spdk_scsi_dev_construct(const char *name, const char *bdev_name_list[], return NULL; } - strncpy(dev->name, name, SPDK_SCSI_DEV_MAX_NAME); + memcpy(dev->name, name, name_len + 1); dev->num_ports = 0; dev->protocol_id = protocol_id; diff --git a/lib/scsi/scsi_bdev.c b/lib/scsi/scsi_bdev.c index ad2b3da39..adefbbe8b 100644 --- a/lib/scsi/scsi_bdev.c +++ b/lib/scsi/scsi_bdev.c @@ -174,7 +174,7 @@ spdk_bdev_scsi_pad_scsi_name(char *dst, const char *name) { size_t len; - len = strnlen(name, SPDK_SCSI_DEV_MAX_NAME); + len = strlen(name); memcpy(dst, name, len); do { dst[len++] = '\0'; diff --git a/test/unit/lib/scsi/dev.c/dev_ut.c b/test/unit/lib/scsi/dev.c/dev_ut.c index 5f83000e3..60c4c0555 100644 --- a/test/unit/lib/scsi/dev.c/dev_ut.c +++ b/test/unit/lib/scsi/dev.c/dev_ut.c @@ -229,6 +229,24 @@ dev_construct_null_lun(void) CU_ASSERT_TRUE(dev == NULL); } +static void +dev_construct_name_too_long(void) +{ + struct spdk_scsi_dev *dev; + const char *bdev_name_list[1] = {"malloc0"}; + int lun_id_list[1] = { 0 }; + char name[SPDK_SCSI_DEV_MAX_NAME + 1 + 1]; + + /* Try to construct a dev with a name that is one byte longer than allowed. */ + memset(name, 'x', sizeof(name) - 1); + name[sizeof(name) - 1] = '\0'; + + dev = spdk_scsi_dev_construct(name, bdev_name_list, lun_id_list, 1, + SPDK_SPC_PROTOCOL_IDENTIFIER_ISCSI, NULL, NULL); + + CU_ASSERT(dev == NULL); +} + static void dev_construct_success(void) { @@ -613,6 +631,7 @@ main(int argc, char **argv) dev_construct_no_lun_zero) == NULL || CU_add_test(suite, "construct - null lun", dev_construct_null_lun) == NULL + || CU_add_test(suite, "construct - name too long", dev_construct_name_too_long) == NULL || CU_add_test(suite, "construct - success", dev_construct_success) == NULL || CU_add_test(suite, "construct - success - LUN zero not first", dev_construct_success_lun_zero_not_first) == NULL diff --git a/test/unit/lib/scsi/scsi_bdev.c/scsi_bdev_ut.c b/test/unit/lib/scsi/scsi_bdev.c/scsi_bdev_ut.c index b2c6b1e98..edcfb1765 100644 --- a/test/unit/lib/scsi/scsi_bdev.c/scsi_bdev_ut.c +++ b/test/unit/lib/scsi/scsi_bdev.c/scsi_bdev_ut.c @@ -542,7 +542,7 @@ inquiry_overflow_test(void) static void scsi_name_padding_test(void) { - char name[SPDK_SCSI_DEV_MAX_NAME + 10]; + char name[SPDK_SCSI_DEV_MAX_NAME + 1]; char buf[SPDK_SCSI_DEV_MAX_NAME + 1]; int written, i; @@ -574,15 +574,6 @@ scsi_name_padding_test(void) CU_ASSERT(written == 256); CU_ASSERT(buf[254] == 'x'); CU_ASSERT(buf[255] == '\0'); - - /* case 4 */ - memset(name, '\0', sizeof(name)); - memset(name, 'x', sizeof(name)); - written = spdk_bdev_scsi_pad_scsi_name(buf, name); - - CU_ASSERT(written == 256); - CU_ASSERT(buf[254] == 'x'); - CU_ASSERT(buf[255] == '\0'); } /*