scsi: 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 <ddiss@suse.de>
Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

6 files changed, 63 insertions, 102 deletions

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index 0e8449be5115..aaf2a785e225 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -620,12 +620,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,
@@ -647,11 +652,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;
diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 15805dec697b..7655c426d6a5 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -720,36 +720,17 @@ void core_dev_free_initiator_node_lun_acl(
 static void scsi_dump_inquiry(struct se_device *dev)
 {
         struct t10_wwn *wwn = &dev->t10_wwn;
-        char buf[17];
-        int i, device_type;
+        int device_type = dev->transport->get_device_type(dev);
+
         /*
          * Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer
          */
-        for (i = 0; i < 8; i++)
-                if (wwn->vendor[i] >= 0x20)
-                        buf[i] = wwn->vendor[i];
-                else
-                        buf[i] = ' ';
-        buf[i] = '\0';
-        pr_debug("  Vendor: %s\n", buf);
-
-        for (i = 0; i < 16; i++)
-                if (wwn->model[i] >= 0x20)
-                        buf[i] = wwn->model[i];
-                else
-                        buf[i] = ' ';
-        buf[i] = '\0';
-        pr_debug("  Model: %s\n", buf);
-
-        for (i = 0; i < 4; i++)
-                if (wwn->revision[i] >= 0x20)
-                        buf[i] = wwn->revision[i];
-                else
-                        buf[i] = ' ';
-        buf[i] = '\0';
-        pr_debug("  Revision: %s\n", buf);
-
-        device_type = dev->transport->get_device_type(dev);
+        pr_debug("  Vendor: %-" __stringify(INQUIRY_VENDOR_LEN) "s\n",
+                wwn->vendor);
+        pr_debug("  Model: %-" __stringify(INQUIRY_MODEL_LEN) "s\n",
+                wwn->model);
+        pr_debug("  Revision: %-" __stringify(INQUIRY_REVISION_LEN) "s\n",
+                wwn->revision);
         pr_debug("  Type:   %s ", scsi_device_type(device_type));
 }
 
@@ -997,11 +978,12 @@ int target_configure_device(struct se_device *dev)
          * passthrough because this is being provided by the backend LLD.
          */
         if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
-                strncpy(&dev->t10_wwn.vendor[0], "LIO-ORG", 8);
-                strncpy(&dev->t10_wwn.model[0],
-                        dev->transport->inquiry_prod, 16);
-                strncpy(&dev->t10_wwn.revision[0],
-                        dev->transport->inquiry_rev, 4);
+                strlcpy(dev->t10_wwn.vendor, "LIO-ORG",
+                        sizeof(dev->t10_wwn.vendor));
+                strlcpy(dev->t10_wwn.model, dev->transport->inquiry_prod,
+                        sizeof(dev->t10_wwn.model));
+                strlcpy(dev->t10_wwn.revision, dev->transport->inquiry_rev,
+                        sizeof(dev->t10_wwn.revision));
         }
 
         scsi_dump_inquiry(dev);
diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index 47d76c862014..c346ad3ee4e0 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -179,20 +179,20 @@ out_free:
 static void
 pscsi_set_inquiry_info(struct scsi_device *sdev, struct t10_wwn *wwn)
 {
-        unsigned char *buf;
-
         if (sdev->inquiry_len < INQUIRY_LEN)
                 return;
-
-        buf = sdev->inquiry;
-        if (!buf)
-                return;
         /*
-         * Use sdev->inquiry from drivers/scsi/scsi_scan.c:scsi_alloc_sdev()
+         * Use sdev->inquiry data from drivers/scsi/scsi_scan.c:scsi_add_lun()
          */
-        memcpy(&wwn->vendor[0], &buf[8], sizeof(wwn->vendor));
-        memcpy(&wwn->model[0], &buf[16], sizeof(wwn->model));
-        memcpy(&wwn->revision[0], &buf[32], sizeof(wwn->revision));
+        BUILD_BUG_ON(sizeof(wwn->vendor) != INQUIRY_VENDOR_LEN + 1);
+        snprintf(wwn->vendor, sizeof(wwn->vendor),
+                 "%." __stringify(INQUIRY_VENDOR_LEN) "s", sdev->vendor);
+        BUILD_BUG_ON(sizeof(wwn->model) != INQUIRY_MODEL_LEN + 1);
+        snprintf(wwn->model, sizeof(wwn->model),
+                 "%." __stringify(INQUIRY_MODEL_LEN) "s", sdev->model);
+        BUILD_BUG_ON(sizeof(wwn->revision) != INQUIRY_REVISION_LEN + 1);
+        snprintf(wwn->revision, sizeof(wwn->revision),
+                 "%." __stringify(INQUIRY_REVISION_LEN) "s", sdev->rev);
 }
 
 static int
@@ -811,7 +811,6 @@ static ssize_t pscsi_show_configfs_dev_params(struct se_device *dev, char *b)
         struct scsi_device *sd = pdv->pdv_sd;
         unsigned char host_id[16];
         ssize_t bl;
-        int i;
 
         if (phv->phv_mode == PHV_VIRTUAL_HOST_ID)
                 snprintf(host_id, 16, "%d", pdv->pdv_host_id);
@@ -824,29 +823,12 @@ static ssize_t pscsi_show_configfs_dev_params(struct se_device *dev, char *b)
                 host_id);
 
         if (sd) {
-                bl += sprintf(b + bl, "        ");
-                bl += sprintf(b + bl, "Vendor: ");
-                for (i = 0; i < 8; i++) {
-                        if (ISPRINT(sd->vendor[i]))   /* printable character? */
-                                bl += sprintf(b + bl, "%c", sd->vendor[i]);
-                        else
-                                bl += sprintf(b + bl, " ");
-                }
-                bl += sprintf(b + bl, " Model: ");
-                for (i = 0; i < 16; i++) {
-                        if (ISPRINT(sd->model[i]))   /* printable character ? */
-                                bl += sprintf(b + bl, "%c", sd->model[i]);
-                        else
-                                bl += sprintf(b + bl, " ");
-                }
-                bl += sprintf(b + bl, " Rev: ");
-                for (i = 0; i < 4; i++) {
-                        if (ISPRINT(sd->rev[i]))   /* printable character ? */
-                                bl += sprintf(b + bl, "%c", sd->rev[i]);
-                        else
-                                bl += sprintf(b + bl, " ");
-                }
-                bl += sprintf(b + bl, "\n");
+                bl += sprintf(b + bl, "        Vendor: %."
+                        __stringify(INQUIRY_VENDOR_LEN) "s", sd->vendor);
+                bl += sprintf(b + bl, " Model: %."
+                        __stringify(INQUIRY_MODEL_LEN) "s", sd->model);
+                bl += sprintf(b + bl, " Rev: %."
+                        __stringify(INQUIRY_REVISION_LEN) "s\n", sd->rev);
         }
         return bl;
 }
diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index e01ba2945a97..5d2993db345b 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -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;
diff --git a/drivers/target/target_core_stat.c b/drivers/target/target_core_stat.c
index b80b3e990821..8d9ceedfd455 100644
--- a/drivers/target/target_core_stat.c
+++ b/drivers/target/target_core_stat.c
@@ -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)
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index bcff24d0a264..69b7b955902c 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -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 */
@@ -315,9 +319,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;