target: consistently null-terminate t10_wwn strings

PATCH Applied David Disseldorp 2 participants 2 messages Dec 5, 2018 – Dec 6, 2018 SCSI target

Share & tools

David Disseldorp

author

2018-12-05 12:18 UTC #0 lore raw

PATCH v7 2/5 target: consistently null-terminate t10_wwn strings

In preparation for supporting user provided vendor strings, add an extra byte to the vendor, model and revision arrays in struct t10_wwn. This ensures that the full INQUIRY data can be carried in the arrays along with a null-terminator.

Change a number of array readers and writers so that they account for explicit null-termination:
- The pscsi_set_inquiry_info() and emulate_model_alias_store() codepaths don't currently explicitly null-terminate; fix this.
- Existing t10_wwn field dumps use for-loops which step over null-terminators for right-padding.
+ Use printf with width specifiers instead.

Signed-off-by David Disseldorp <[email protected]>

---
 drivers/target/target_core_configfs.c | 16 +++++++----
 drivers/target/target_core_device.c   | 46 ++++++++++----------------------
 drivers/target/target_core_pscsi.c    | 50 +++++++++++------------------------
 drivers/target/target_core_spc.c      |  7 ++---
 drivers/target/target_core_stat.c     | 32 +++++-----------------
 include/target/target_core_base.h     | 14 +++++++---
 6 files changed, 63 insertions(+), 102 deletions(-)

drivers/target/target_core_configfs.c +11 -5

index f6b1549f4142..8277bcf81d6e 100644
@@ -613,12 +613,17 @@ static void dev_set_t10_wwn_model_alias(struct se_device *dev)
 	const char *configname;
 
 	configname = config_item_name(&dev->dev_group.cg_item);
-	if (strlen(configname) >= 16) {
+	if (strlen(configname) >= INQUIRY_MODEL_LEN) {
 		pr_warn("dev[%p]: Backstore name '%s' is too long for "
-			"INQUIRY_MODEL, truncating to 16 bytes\n", dev,
+			"INQUIRY_MODEL, truncating to 15 characters\n", dev,
 			configname);
 	}
-	snprintf(&dev->t10_wwn.model[0], 16, "%s", configname);
+	/*
+	 * XXX We can't use sizeof(dev->t10_wwn.model) (INQUIRY_MODEL_LEN + 1)
+	 * here without potentially breaking existing setups, so continue to
+	 * truncate one byte shorter than what can be carried in INQUIRY.
+	 */
+	strlcpy(dev->t10_wwn.model, configname, INQUIRY_MODEL_LEN);
 }
 
 static ssize_t emulate_model_alias_store(struct config_item *item,

@@ -640,11 +645,12 @@ static ssize_t emulate_model_alias_store(struct config_item *item,
 	if (ret < 0)
 		return ret;
 
+	BUILD_BUG_ON(sizeof(dev->t10_wwn.model) != INQUIRY_MODEL_LEN + 1);
 	if (flag) {
 		dev_set_t10_wwn_model_alias(dev);
 	} else {
-		strncpy(&dev->t10_wwn.model[0],
-			dev->transport->inquiry_prod, 16);
+		strlcpy(dev->t10_wwn.model, dev->transport->inquiry_prod,
+			sizeof(dev->t10_wwn.model));
 	}
 	da->emulate_model_alias = flag;
 	return count;

drivers/target/target_core_device.c +14 ~~-32~~

61 lines not on this page · raw message

drivers/target/target_core_pscsi.c +16 ~~-34~~

72 lines not on this page · raw message

drivers/target/target_core_spc.c +4 -3

index c37dd36ec77d..8ffe712cb44d 100644
@@ -113,12 +113,13 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char *buf)
 	 * unused bytes at the end of the field (i.e., highest offset) and the
 	 * unused bytes shall be filled with ASCII space characters (20h).
 	 */
-	memset(&buf[8], 0x20, 8 + 16 + 4);
+	memset(&buf[8], 0x20,
+	       INQUIRY_VENDOR_LEN + INQUIRY_MODEL_LEN + INQUIRY_REVISION_LEN);
 	memcpy(&buf[8], "LIO-ORG", sizeof("LIO-ORG") - 1);
 	memcpy(&buf[16], dev->t10_wwn.model,
-	       strnlen(dev->t10_wwn.model, 16));
+	       strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
 	memcpy(&buf[32], dev->t10_wwn.revision,
-	       strnlen(dev->t10_wwn.revision, 4));
+	       strnlen(dev->t10_wwn.revision, INQUIRY_REVISION_LEN));
 	buf[4] = 31; /* Set additional length to 31 */
 
 	return 0;

drivers/target/target_core_stat.c +7 ~~-25~~

index f0db91ebd735..87fd2b11fe3b 100644
@@ -246,43 +246,25 @@ static ssize_t target_stat_lu_lu_name_show(struct config_item *item, char *page)
 static ssize_t target_stat_lu_vend_show(struct config_item *item, char *page)
 {
 	struct se_device *dev = to_stat_lu_dev(item);
-	int i;
-	char str[sizeof(dev->t10_wwn.vendor)+1];
 
-	/* scsiLuVendorId */
-	for (i = 0; i < sizeof(dev->t10_wwn.vendor); i++)
-		str[i] = ISPRINT(dev->t10_wwn.vendor[i]) ?
-			dev->t10_wwn.vendor[i] : ' ';
-	str[i] = '\0';
-	return snprintf(page, PAGE_SIZE, "%s\n", str);
+	return snprintf(page, PAGE_SIZE, "%-" __stringify(INQUIRY_VENDOR_LEN)
+			"s\n", dev->t10_wwn.vendor);
 }
 
 static ssize_t target_stat_lu_prod_show(struct config_item *item, char *page)
 {
 	struct se_device *dev = to_stat_lu_dev(item);
-	int i;
-	char str[sizeof(dev->t10_wwn.model)+1];
 
-	/* scsiLuProductId */
-	for (i = 0; i < sizeof(dev->t10_wwn.model); i++)
-		str[i] = ISPRINT(dev->t10_wwn.model[i]) ?
-			dev->t10_wwn.model[i] : ' ';
-	str[i] = '\0';
-	return snprintf(page, PAGE_SIZE, "%s\n", str);
+	return snprintf(page, PAGE_SIZE, "%-" __stringify(INQUIRY_MODEL_LEN)
+			"s\n", dev->t10_wwn.model);
 }
 
 static ssize_t target_stat_lu_rev_show(struct config_item *item, char *page)
 {
 	struct se_device *dev = to_stat_lu_dev(item);
-	int i;
-	char str[sizeof(dev->t10_wwn.revision)+1];
-
-	/* scsiLuRevisionId */
-	for (i = 0; i < sizeof(dev->t10_wwn.revision); i++)
-		str[i] = ISPRINT(dev->t10_wwn.revision[i]) ?
-			dev->t10_wwn.revision[i] : ' ';
-	str[i] = '\0';
-	return snprintf(page, PAGE_SIZE, "%s\n", str);
+
+	return snprintf(page, PAGE_SIZE, "%-" __stringify(INQUIRY_REVISION_LEN)
+			"s\n", dev->t10_wwn.revision);
 }
 
 static ssize_t target_stat_lu_dev_type_show(struct config_item *item, char *page)

include/target/target_core_base.h +11 -3

index e3bdb0550a59..497853a09fee 100644
@@ -46,6 +46,10 @@
 /* Used by transport_get_inquiry_vpd_device_ident() */
 #define INQUIRY_VPD_DEVICE_IDENTIFIER_LEN	254
 
+#define INQUIRY_VENDOR_LEN			8
+#define INQUIRY_MODEL_LEN			16
+#define INQUIRY_REVISION_LEN			4
+
 /* Attempts before moving from SHORT to LONG */
 #define PYX_TRANSPORT_WINDOW_CLOSED_THRESHOLD	3
 #define PYX_TRANSPORT_WINDOW_CLOSED_WAIT_SHORT	3  /* In milliseconds */

@@ -314,9 +318,13 @@ struct t10_vpd {
 };
 
 struct t10_wwn {
-	char vendor[8];
-	char model[16];
-	char revision[4];
+	/*
+	 * SCSI left aligned strings may not be null terminated. +1 to ensure a
+	 * null terminator is always present.
+	 */
+	char vendor[INQUIRY_VENDOR_LEN + 1];
+	char model[INQUIRY_MODEL_LEN + 1];
+	char revision[INQUIRY_REVISION_LEN + 1];
 	char unit_serial[INQUIRY_VPD_SERIAL_LEN];
 	spinlock_t t10_vpd_lock;
 	struct se_device *t10_dev;

Signature · 1 line

2.13.7

Roman Bolshakov

Reviewed-by on v7 2/5

2018-12-06 12:54 UTC #1 lore raw

Quoted 14 lines · David Disseldorp

In preparation for supporting user provided vendor strings, add an extra byte to the vendor, model and revision arrays in struct t10_wwn. This ensures that the full INQUIRY data can be carried in the arrays along with a null-terminator.
Change a number of array readers and writers so that they account for explicit null-termination:
- The pscsi_set_inquiry_info() and emulate_model_alias_store() codepaths don't currently explicitly null-terminate; fix this.
- Existing t10_wwn field dumps use for-loops which step over null-terminators for right-padding.
+ Use printf with width specifiers instead.
Signed-off-by David Disseldorp <[email protected]>

Quoted 9 lines

---
 drivers/target/target_core_configfs.c | 16 +++++++----
 drivers/target/target_core_device.c   | 46 ++++++++++----------------------
 drivers/target/target_core_pscsi.c    | 50 +++++++++++------------------------
 drivers/target/target_core_spc.c      |  7 ++---
 drivers/target/target_core_stat.c     | 32 +++++-----------------
 include/target/target_core_base.h     | 14 +++++++---
 6 files changed, 63 insertions(+), 102 deletions(-)

Reviewed-by Roman Bolshakov <[email protected]>

Thank you,
Roman Bolshakov