Commit af9c9a41 authored by Daniel Verkamp's avatar Daniel Verkamp
Browse files

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: default avatarDaniel Verkamp <daniel.verkamp@intel.com>
Reviewed-on: https://review.gerrithub.io/406990


Tested-by: default avatarSPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
parent 97934c52

lib/scsi/dev.c

+9 −1
Original line number Diff line number Diff line
@@ -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;

lib/scsi/scsi_bdev.c

+1 −1
Original line number Diff line number Diff line
@@ -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';

test/unit/lib/scsi/dev.c/dev_ut.c

+19 −0
Original line number Diff line number Diff line
@@ -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

test/unit/lib/scsi/scsi_bdev.c/scsi_bdev_ut.c

+1 −10
Original line number Diff line number Diff line
@@ -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');

}



/*