[PATCH v2 0/3] Rework the struct scsi_device inquiry information

[PATCH v2 0/3] Rework the struct scsi_device inquiry information

[Bart Van Assche]

Hi Martin,

The vendor, model and rev members in struct scsi_device are fixed-length
strings that are not NUL-terminated. This patch converts these members into
NUL-terminated character arrays. This makes it less error-prone to deal with
these structure members. The patches in this series have been implemented such
that the number of lines changed and the risk for regressions is minimized.

Please consider this patch series for the next merge window.

Thanks,

Bart.

Changes compared to v1:
 - Added symbolic constants for the offsets in patch 1/3.
 - Added a new patch 2/3.
 - Converted one strscpy() call into a memcpy() call in patch 3/3 because the
   strscpy() call triggers a KASAN read-out-of-bounds complaint.

Bart Van Assche (3):
  scsi: core, target: Add INQUIRY-related constants into
    <scsi/scsi_common.h>
  scsi: core: Use the INQUIRY-related constants
  scsi: core: Convert INQUIRY information

 drivers/hwmon/drivetemp.c         |  5 +----
 drivers/scsi/scsi_scan.c          | 33 ++++++++++++++++++++-----------
 include/scsi/scsi_common.h        |  8 ++++++++
 include/scsi/scsi_device.h        |  7 ++++---
 include/target/target_core_base.h |  5 +----
 5 files changed, 36 insertions(+), 22 deletions(-)

[PATCH v2 1/3] scsi: core, target: Add INQUIRY-related constants into <scsi/scsi_common.h>

[Bart Van Assche]

Move three constants from <target/target_core_base.h> into
<scsi/scsi_common.h>. Add three new constants in the scsi_common.h header
file. This patch prepares for using these constants in the SCSI core.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/scsi/scsi_common.h        | 8 ++++++++
 include/target/target_core_base.h | 5 +----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h
index fb58715fac86..00c8a16d3cd2 100644
--- a/include/scsi/scsi_common.h
+++ b/include/scsi/scsi_common.h
@@ -10,6 +10,14 @@
 #include <uapi/linux/pr.h>
 #include <scsi/scsi_proto.h>
 
+/* From the standard INQUIRY data description in SPC-6. */
+#define INQUIRY_VENDOR_OFFSET	8
+#define INQUIRY_VENDOR_LEN	8
+#define INQUIRY_MODEL_OFFSET	16
+#define INQUIRY_MODEL_LEN	16
+#define INQUIRY_REVISION_OFFSET	32
+#define INQUIRY_REVISION_LEN	4
+
 enum scsi_pr_type {
 	SCSI_PR_WRITE_EXCLUSIVE			= 0x01,
 	SCSI_PR_EXCLUSIVE_ACCESS		= 0x03,
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 9a0e9f9e1ec4..002b0fc57587 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -8,6 +8,7 @@
 #include <linux/percpu-refcount.h>
 #include <linux/semaphore.h>     /* struct semaphore */
 #include <linux/completion.h>
+#include <scsi/scsi_common.h>
 
 #define TARGET_CORE_VERSION		"v5.0"
 
@@ -46,10 +47,6 @@
 /* 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 */

[PATCH v2 2/3] scsi: core: Use the INQUIRY-related constants

[Bart Van Assche]

Use symbolic names instead of numeric constants to access the vendor and
model information.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_scan.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index a35a5f777d16..b0473f4595c1 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -728,9 +728,13 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	}
 
 	if (result == 0) {
-		scsi_sanitize_inquiry_string(&inq_result[8], 8);
-		scsi_sanitize_inquiry_string(&inq_result[16], 16);
-		scsi_sanitize_inquiry_string(&inq_result[32], 4);
+		scsi_sanitize_inquiry_string(&inq_result[INQUIRY_VENDOR_OFFSET],
+					     INQUIRY_VENDOR_LEN);
+		scsi_sanitize_inquiry_string(&inq_result[INQUIRY_MODEL_OFFSET],
+					     INQUIRY_MODEL_LEN);
+		scsi_sanitize_inquiry_string(
+			&inq_result[INQUIRY_REVISION_OFFSET],
+			INQUIRY_REVISION_LEN);
 
 		response_len = inq_result[4] + 5;
 		if (response_len > 255)
@@ -743,8 +747,9 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 		 * corresponding bit fields in scsi_device, so bflags
 		 * need not be passed as an argument.
 		 */
-		*bflags = scsi_get_device_flags(sdev, &inq_result[8],
-				&inq_result[16]);
+		*bflags = scsi_get_device_flags(sdev,
+				&inq_result[INQUIRY_VENDOR_OFFSET],
+				&inq_result[INQUIRY_MODEL_OFFSET]);
 
 		/* When the first pass succeeds we gain information about
 		 * what larger transfer lengths might work. */

[PATCH v2 3/3] scsi: core: Convert INQUIRY information

[Bart Van Assche]

Currently the vendor, model, and revision members of struct scsi_device
are pointers to fixed-length strings that are not NUL-terminated.
Fixed-precision format specifiers (e.g., "%.8s") are required whenever
they are printed and strncmp() must be used to compare these fields.
This is error-prone.

Convert these fields to fixed-size character arrays within struct
scsi_device. Remove an !sdev->model check because sdev->model is now
guaranteed not to be NULL.

This patch fixes a bug in the qla2xxx driver. It makes the following
code safe:

		if (state_flags & BIT_4)
			scmd_printk(KERN_WARNING, cp,
			    "Unsupported device '%s' found.\n",
			    cp->device->vendor);

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/hwmon/drivetemp.c  |  5 +----
 drivers/scsi/scsi_scan.c   | 18 ++++++++++++------
 include/scsi/scsi_device.h |  7 ++++---
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/drivetemp.c b/drivers/hwmon/drivetemp.c
index 002e0660a0b8..efe8b229bdbe 100644
--- a/drivers/hwmon/drivetemp.c
+++ b/drivers/hwmon/drivetemp.c
@@ -306,13 +306,10 @@ static bool drivetemp_sct_avoid(struct drivetemp_data *st)
 	struct scsi_device *sdev = st->sdev;
 	unsigned int ctr;
 
-	if (!sdev->model)
-		return false;
-
 	/*
 	 * The "model" field contains just the raw SCSI INQUIRY response
 	 * "product identification" field, which has a width of 16 bytes.
-	 * This field is space-filled, but is NOT NULL-terminated.
+	 * This field is space-filled and NUL-terminated.
 	 */
 	for (ctr = 0; ctr < ARRAY_SIZE(sct_avoid_models); ctr++)
 		if (!strncmp(sdev->model, sct_avoid_models[ctr],
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index b0473f4595c1..7e60e3a4bca6 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -292,9 +292,9 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	if (!sdev)
 		goto out;
 
-	sdev->vendor = scsi_null_device_strs;
-	sdev->model = scsi_null_device_strs;
-	sdev->rev = scsi_null_device_strs;
+	strscpy(sdev->vendor, scsi_null_device_strs);
+	strscpy(sdev->model, scsi_null_device_strs);
+	strscpy(sdev->rev, scsi_null_device_strs);
 	sdev->host = shost;
 	sdev->queue_ramp_up_period = SCSI_DEFAULT_RAMP_UP_PERIOD;
 	sdev->id = starget->id;
@@ -910,9 +910,15 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	if (sdev->inquiry == NULL)
 		return SCSI_SCAN_NO_RESPONSE;
 
-	sdev->vendor = (char *) (sdev->inquiry + 8);
-	sdev->model = (char *) (sdev->inquiry + 16);
-	sdev->rev = (char *) (sdev->inquiry + 32);
+	strscpy(sdev->vendor, sdev->inquiry + INQUIRY_VENDOR_OFFSET);
+	strscpy(sdev->model, sdev->inquiry + INQUIRY_MODEL_OFFSET);
+	/*
+	 * memcpy() instead of strscpy() because strscpy() would read past
+	 * the end of sdev->inquiry if its length is exactly 36 bytes.
+	 */
+	memcpy(sdev->rev, sdev->inquiry + INQUIRY_REVISION_OFFSET,
+	       INQUIRY_REVISION_LEN);
+	sdev->rev[INQUIRY_REVISION_LEN] = '\0';
 
 	sdev->is_ata = strncmp(sdev->vendor, "ATA     ", 8) == 0;
 	if (sdev->is_ata) {
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 9c2a7bbe5891..029f5115b2ea 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -7,6 +7,7 @@
 #include <linux/workqueue.h>
 #include <linux/blk-mq.h>
 #include <scsi/scsi.h>
+#include <scsi/scsi_common.h>
 #include <linux/atomic.h>
 #include <linux/sbitmap.h>
 
@@ -137,9 +138,9 @@ struct scsi_device {
 	struct mutex inquiry_mutex;
 	unsigned char inquiry_len;	/* valid bytes in 'inquiry' */
 	unsigned char * inquiry;	/* INQUIRY response data */
-	const char * vendor;		/* [back_compat] point into 'inquiry' ... */
-	const char * model;		/* ... after scan; point to static string */
-	const char * rev;		/* ... "nullnullnullnull" before scan */
+	char vendor[INQUIRY_VENDOR_LEN + 1];
+	char model[INQUIRY_MODEL_LEN + 1];
+	char rev[INQUIRY_REVISION_LEN + 1];
 
 #define SCSI_DEFAULT_VPD_LEN	255	/* default SCSI VPD page size (max) */
 	struct scsi_vpd __rcu *vpd_pg0;