Re: [PATCH v3 2/6] scsi: Protect INQUIRY sysfs attributes with mutex

[Bart Van Assche <bvanassche@acm.org>]

On 5/2/26 6:37 PM, Bart Van Assche wrote:
> Another possibility is to change sdev->vendor, sdev->model and
> sdev->rev from pointers to fixed size strings into '\0'-terminated char
> arrays.

(replying to my own email)

How about the untested patch below?

scsi: core: Convert inquiry information

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 and remove trailing white space at initialization time.

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);

diff --git a/drivers/hwmon/drivetemp.c b/drivers/hwmon/drivetemp.c
index 002e0660a0b8..e71b99f70076 100644
--- a/drivers/hwmon/drivetemp.c
+++ b/drivers/hwmon/drivetemp.c
@@ -306,7 +306,7 @@ static bool drivetemp_sct_avoid(struct 
drivetemp_data *st)
  	struct scsi_device *sdev = st->sdev;
  	unsigned int ctr;

-	if (!sdev->model)
+	if (!sdev->model[0])
  		return false;

  	/*
diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index fcf059bf41e8..f92ad396ac9c 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -7204,7 +7204,7 @@ static void AscAsyncFix(ASC_DVC_VAR *asc_dvc, 
struct scsi_device *sdev)
  	if (asc_dvc->init_sdtr & tid_bits)
  		return;

-	if ((type == TYPE_ROM) && (strncmp(sdev->vendor, "HP ", 3) == 0))
+	if (type == TYPE_ROM && strcmp(sdev->vendor, "HP") == 0)
  		asc_dvc->pci_fix_asyn_xfer_always |= tid_bits;

  	asc_dvc->pci_fix_asyn_xfer |= tid_bits;
diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 4010fdbf813c..fb59ca035843 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -408,7 +408,7 @@ ch_readconfig(scsi_changer *ch)
  				/* should not happen */
  				VPRINTK(KERN_CONT, "Huh? device not found!\n");
  			} else {
-				VPRINTK(KERN_CONT, "name: %8.8s %16.16s %4.4s\n",
+				VPRINTK(KERN_CONT, "name: %8s %16s %4s\n",
  					ch->dt[elem]->vendor,
  					ch->dt[elem]->model,
  					ch->dt[elem]->rev);
diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index a1b116cd4723..bdec827b82a1 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -1262,7 +1262,7 @@ static void hpsa_show_dev_msg(const char *level, 
struct ctlr_info *h,
  	}

  	dev_printk(level, &h->pdev->dev,
-			"scsi %d:%d:%d:%d: %s %s %.8s %.16s %s SSDSmartPathCap%c En%c Exp=%d\n",
+			"scsi %d:%d:%d:%d: %s %s %s %s %s SSDSmartPathCap%c En%c Exp=%d\n",
  			h->scsi_host->host_no, dev->bus, dev->target, dev->lun,
  			description,
  			scsi_device_type(dev->devtype),
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index ef22a4228b85..242bd46df3ba 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;
@@ -857,6 +857,21 @@ static int scsi_probe_lun(struct scsi_device *sdev, 
unsigned char *inq_result,
  	return 0;
  }

+static void strip_trailing_spaces(char *s)
+{
+	size_t size;
+	char *end;
+
+	size = strlen(s);
+	if (!size)
+		return;
+
+	end = s + size - 1;
+	while (end >= s && isspace(*end))
+		end--;
+	*(end + 1) = '\0';
+}
+
  /**
   * scsi_add_lun - allocate and fully initialze a scsi_device
   * @sdev:	holds information to be stored in the new scsi_device
@@ -905,11 +920,17 @@ 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);
-
-	sdev->is_ata = strncmp(sdev->vendor, "ATA     ", 8) == 0;
+	memcpy(sdev->vendor, sdev->inquiry + 8, 8);
+	sdev->vendor[8] = 0;
+	strip_trailing_spaces(sdev->vendor);
+	memcpy(sdev->model, sdev->inquiry + 16, 16);
+	sdev->model[16] = 0;
+	strip_trailing_spaces(sdev->model);
+	memcpy(sdev->rev, sdev->inquiry + 32, 4);
+	sdev->rev[4] = 0;
+	strip_trailing_spaces(sdev->rev);
+
+	sdev->is_ata = strcmp(sdev->vendor, "ATA") == 0;
  	if (sdev->is_ata) {
  		/*
  		 * sata emulation layer device.  This is a hack to work around
@@ -978,7 +999,7 @@ static int scsi_add_lun(struct scsi_device *sdev, 
unsigned char *inq_result,
  	if (inq_result[7] & 0x10)
  		sdev->sdtr = 1;

-	sdev_printk(KERN_NOTICE, sdev, "%s %.8s %.16s %.4s PQ: %d "
+	sdev_printk(KERN_NOTICE, sdev, "%s %s %s %s PQ: %d "
  			"ANSI: %d%s\n", scsi_device_type(sdev->type),
  			sdev->vendor, sdev->model, sdev->rev,
  			sdev->inq_periph_qual, inq_result[2] & 0x07,
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index dfc3559e7e04..3282ad956d1f 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -648,9 +648,9 @@ static DEVICE_ATTR(field, S_IRUGO, 
sdev_show_##field, NULL);
   */
  sdev_rd_attr (type, "%d\n");
  sdev_rd_attr (scsi_level, "%d\n");
-sdev_rd_attr (vendor, "%.8s\n");
-sdev_rd_attr (model, "%.16s\n");
-sdev_rd_attr (rev, "%.4s\n");
+sdev_rd_attr (vendor, "%s\n");
+sdev_rd_attr (model, "%s\n");
+sdev_rd_attr (rev, "%s\n");
  sdev_rd_attr (cdl_supported, "%d\n");

  static ssize_t
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 2b4b2a1a8e44..b6c248e69b3a 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -2507,7 +2507,7 @@ static int sg_proc_seq_show_devstrs(struct 
seq_file *s, void *v)
  	sdp = it ? sg_lookup_dev(it->index) : NULL;
  	scsidp = sdp ? sdp->device : NULL;
  	if (sdp && scsidp && (!atomic_read(&sdp->detaching)))
-		seq_printf(s, "%8.8s\t%16.16s\t%4.4s\n",
+		seq_printf(s, "%8s\t%16s\t%4s\n",
  			   scsidp->vendor, scsidp->model, scsidp->rev);
  	else
  		seq_puts(s, "<no active device>\n");
diff --git a/drivers/target/target_core_pscsi.c 
b/drivers/target/target_core_pscsi.c
index 9ed2818c3028..1718d1620f24 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -169,14 +169,11 @@ pscsi_set_inquiry_info(struct scsi_device *sdev, 
struct t10_wwn *wwn)
  	 * Use sdev->inquiry data from drivers/scsi/scsi_scan.c:scsi_add_lun()
  	 */
  	BUILD_BUG_ON(sizeof(wwn->vendor) != INQUIRY_VENDOR_LEN + 1);
-	snprintf(wwn->vendor, sizeof(wwn->vendor),
-		 "%." __stringify(INQUIRY_VENDOR_LEN) "s", sdev->vendor);
+	strscpy(wwn->vendor, 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);
+	strscpy(wwn->model, 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);
+	strscpy(wwn->revision, sdev->rev);
  }

  static int
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 4805e40ed4d7..70315f7fdafe 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -702,7 +702,7 @@ static void ufshcd_print_host_state(struct ufs_hba *hba)
  	dev_err(hba->dev, "quirks=0x%x, dev. quirks=0x%x\n", hba->quirks,
  		hba->dev_quirks);
  	if (sdev_ufs)
-		dev_err(hba->dev, "UFS dev info: %.8s %.16s rev %.4s\n",
+		dev_err(hba->dev, "UFS dev info: %s %s rev %s\n",
  			sdev_ufs->vendor, sdev_ufs->model, sdev_ufs->rev);

  	ufshcd_print_clk_freqs(hba);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 9c2a7bbe5891..cce4587b4b80 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -137,9 +137,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[9];
+	char model[17];
+	char rev[5];

  #define SCSI_DEFAULT_VPD_LEN	255	/* default SCSI VPD page size (max) */
  	struct scsi_vpd __rcu *vpd_pg0;