[PATCH 00/10] scsi: Replace {v}snprintf() variants with safer alternatives

[PATCH 00/10] scsi: Replace {v}snprintf() variants with safer alternatives

[Lee Jones]

Note: We're also taking the time to obay our new .editorconfig overlord!

For a far better description of the problem than I could author, see
Jon's write-up on LWN [1] and/or Alex's on the Kernel Self Protection
Project [1].

[0] https://lwn.net/Articles/69419/
[1] https://github.com/KSPP/linux/issues/105

Lee Jones (10):
  scsi: 3w-xxxx: Trivial: Remove trailing whitespace
  scsi: 53c700: Trivial: Remove trailing whitespace
  scsi: NCR5380: Replace snprintf() with the safer scnprintf() variant
  scsi: aacraid: linit: Remove snprintf() from sysfs call-backs and
    replace with sysfs_emit()
  scsi: aacraid: linit: Replace snprintf() with the safer scnprintf()
    variant
  scsi: aha1542: Replace snprintf() with the safer scnprintf() variant
  scsi: aic7xxx: aicasm: Trivial: Remove trailing whitespace
  scsi: aic7xxx: aicasm: Replace snprintf() with the safer scnprintf()
    variant
  scsi: aic94xx: Remove snprintf() from sysfs call-backs and replace
    with sysfs_emit()
  scsi: arcmsr: Remove snprintf() from sysfs call-backs and replace with
    sysfs_emit()

 drivers/scsi/3w-xxxx.c               |   2 +-
 drivers/scsi/53c700.c                | 102 +++++++++++++--------------
 drivers/scsi/NCR5380.c               |  16 ++---
 drivers/scsi/aacraid/linit.c         |  40 +++++------
 drivers/scsi/aha1542.c               |   2 +-
 drivers/scsi/aic7xxx/aicasm/aicasm.c |  16 ++---
 drivers/scsi/aic94xx/aic94xx_init.c  |  11 ++-
 drivers/scsi/arcmsr/arcmsr_attr.c    |  40 +++--------
 8 files changed, 101 insertions(+), 128 deletions(-)

Cc: Adam Radford <aradford@gmail.com>
Cc: Adaptec OEM Raid Solutions <aacraid@microsemi.com>
Cc: Andre Hedrick <andre@suse.com>
Cc: de Melo <acme@conectiva.com.br>
Cc: drew@colorado.edu
Cc: Finn Thain <fthain@linux-m68k.org>
Cc: Hannes Reinecke <hare@suse.com>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: Joel Jacobson <linux@3ware.com>
Cc: John Garry <john.g.garry@oracle.com>
Cc: Lee Jones <lee@kernel.org>
Cc: linux-scsi@vger.kernel.org
Cc: Luben Tuikov <luben_tuikov@adaptec.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Michael Schmitz <schmitzmic@gmail.com>
Cc: "PMC-Sierra, Inc" <aacraid@pmc-sierra.com>
Cc: Richard Hirst <rhirst@linuxcare.com>
Cc: support@areca.com.tw
Cc: Tnx to <Thomas_Roesch@m2.maus.de>
-- 
2.43.0.594.gd9cf4e227d-goog

[PATCH 03/10] scsi: NCR5380: Replace snprintf() with the safer scnprintf() variant

[Lee Jones]

There is a general misunderstanding amongst engineers that {v}snprintf()
returns the length of the data *actually* encoded into the destination
array.  However, as per the C99 standard {v}snprintf() really returns
the length of the data that *would have been* written if there were
enough space for it.  This misunderstanding has led to buffer-overruns
in the past.  It's generally considered safer to use the {v}scnprintf()
variants in their place (or even sprintf() in simple cases).  So let's
do that.

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Signed-off-by: Lee Jones <lee@kernel.org>
---
Cc: Finn Thain <fthain@linux-m68k.org>
Cc: Michael Schmitz <schmitzmic@gmail.com>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: drew@colorado.edu
Cc: Tnx to <Thomas_Roesch@m2.maus.de>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/NCR5380.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index cea3a79d538e4..ea565e843c765 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -421,14 +421,14 @@ static int NCR5380_init(struct Scsi_Host *instance, int flags)
 	if (!hostdata->work_q)
 		return -ENOMEM;
 
-	snprintf(hostdata->info, sizeof(hostdata->info),
-		"%s, irq %d, io_port 0x%lx, base 0x%lx, can_queue %d, cmd_per_lun %d, sg_tablesize %d, this_id %d, flags { %s%s%s}",
-		instance->hostt->name, instance->irq, hostdata->io_port,
-		hostdata->base, instance->can_queue, instance->cmd_per_lun,
-		instance->sg_tablesize, instance->this_id,
-		hostdata->flags & FLAG_DMA_FIXUP     ? "DMA_FIXUP "     : "",
-		hostdata->flags & FLAG_NO_PSEUDO_DMA ? "NO_PSEUDO_DMA " : "",
-		hostdata->flags & FLAG_TOSHIBA_DELAY ? "TOSHIBA_DELAY " : "");
+	scnprintf(hostdata->info, sizeof(hostdata->info),
+		 "%s, irq %d, io_port 0x%lx, base 0x%lx, can_queue %d, cmd_per_lun %d, sg_tablesize %d, this_id %d, flags { %s%s%s}",
+		 instance->hostt->name, instance->irq, hostdata->io_port,
+		 hostdata->base, instance->can_queue, instance->cmd_per_lun,
+		 instance->sg_tablesize, instance->this_id,
+		 hostdata->flags & FLAG_DMA_FIXUP     ? "DMA_FIXUP "     : "",
+		 hostdata->flags & FLAG_NO_PSEUDO_DMA ? "NO_PSEUDO_DMA " : "",
+		 hostdata->flags & FLAG_TOSHIBA_DELAY ? "TOSHIBA_DELAY " : "");
 
 	NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
 	NCR5380_write(MODE_REG, MR_BASE);
-- 
2.43.0.594.gd9cf4e227d-goog

[PATCH 04/10] scsi: aacraid: linit: Remove snprintf() from sysfs call-backs and replace with sysfs_emit()

[Lee Jones]

Since snprintf() has the documented, but still rather strange trait of
returning the length of the data that *would have been* written to the
array if space were available, rather than the arguably more useful
length of data *actually* written, it is usually considered wise to use
something else instead in order to avoid confusion.

In the case of sysfs call-backs, new wrappers exist that do just that.

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Signed-off-by: Lee Jones <lee@kernel.org>
---
Cc: Adaptec OEM Raid Solutions <aacraid@microsemi.com>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: "PMC-Sierra, Inc" <aacraid@pmc-sierra.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/aacraid/linit.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index 68f4dbcfff492..69526e2bd2e78 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -558,11 +558,9 @@ static ssize_t aac_show_raid_level(struct device *dev, struct device_attribute *
 	struct scsi_device *sdev = to_scsi_device(dev);
 	struct aac_dev *aac = (struct aac_dev *)(sdev->host->hostdata);
 	if (sdev_channel(sdev) != CONTAINER_CHANNEL)
-		return snprintf(buf, PAGE_SIZE, sdev->no_uld_attach
-		  ? "Hidden\n" :
+		return sysfs_emit(buf, sdev->no_uld_attach ? "Hidden\n" :
 		  ((aac->jbod && (sdev->type == TYPE_DISK)) ? "JBOD\n" : ""));
-	return snprintf(buf, PAGE_SIZE, "%s\n",
-	  get_container_type(aac->fsa_dev[sdev_id(sdev)].type));
+	return sysfs_emit(buf, "%s\n", get_container_type(aac->fsa_dev[sdev_id(sdev)].type));
 }
 
 static struct device_attribute aac_raid_level_attr = {
@@ -1195,10 +1193,9 @@ static ssize_t aac_show_model(struct device *device,
 			++cp;
 		while (*cp == ' ')
 			++cp;
-		len = snprintf(buf, PAGE_SIZE, "%s\n", cp);
+		len = sysfs_emit(buf, "%s\n", cp);
 	} else
-		len = snprintf(buf, PAGE_SIZE, "%s\n",
-		  aac_drivers[dev->cardtype].model);
+		len = sysfs_emit(buf, "%s\n", aac_drivers[dev->cardtype].model);
 	return len;
 }
 
@@ -1214,12 +1211,11 @@ static ssize_t aac_show_vendor(struct device *device,
 		char *cp = sup_adap_info->adapter_type_text;
 		while (*cp && *cp != ' ')
 			++cp;
-		len = snprintf(buf, PAGE_SIZE, "%.*s\n",
+		len = sysfs_emit(buf, "%.*s\n",
 			(int)(cp - (char *)sup_adap_info->adapter_type_text),
 					sup_adap_info->adapter_type_text);
 	} else
-		len = snprintf(buf, PAGE_SIZE, "%s\n",
-			aac_drivers[dev->cardtype].vname);
+		len = sysfs_emit(buf, "%s\n", aac_drivers[dev->cardtype].vname);
 	return len;
 }
 
@@ -1230,7 +1226,7 @@ static ssize_t aac_show_flags(struct device *cdev,
 	struct aac_dev *dev = (struct aac_dev*)class_to_shost(cdev)->hostdata;
 
 	if (nblank(dprintk(x)))
-		len = snprintf(buf, PAGE_SIZE, "dprintk\n");
+		len = sysfs_emit(buf, "dprintk\n");
 #ifdef AAC_DETAILED_STATUS_INFO
 	len += scnprintf(buf + len, PAGE_SIZE - len,
 			 "AAC_DETAILED_STATUS_INFO\n");
@@ -1258,7 +1254,7 @@ static ssize_t aac_show_kernel_version(struct device *device,
 	int len, tmp;
 
 	tmp = le32_to_cpu(dev->adapter_info.kernelrev);
-	len = snprintf(buf, PAGE_SIZE, "%d.%d-%d[%d]\n",
+	len = sysfs_emit(buf, "%d.%d-%d[%d]\n",
 	  tmp >> 24, (tmp >> 16) & 0xff, tmp & 0xff,
 	  le32_to_cpu(dev->adapter_info.kernelbuild));
 	return len;
@@ -1272,7 +1268,7 @@ static ssize_t aac_show_monitor_version(struct device *device,
 	int len, tmp;
 
 	tmp = le32_to_cpu(dev->adapter_info.monitorrev);
-	len = snprintf(buf, PAGE_SIZE, "%d.%d-%d[%d]\n",
+	len = sysfs_emit(buf, "%d.%d-%d[%d]\n",
 	  tmp >> 24, (tmp >> 16) & 0xff, tmp & 0xff,
 	  le32_to_cpu(dev->adapter_info.monitorbuild));
 	return len;
@@ -1286,7 +1282,7 @@ static ssize_t aac_show_bios_version(struct device *device,
 	int len, tmp;
 
 	tmp = le32_to_cpu(dev->adapter_info.biosrev);
-	len = snprintf(buf, PAGE_SIZE, "%d.%d-%d[%d]\n",
+	len = sysfs_emit(buf, "%d.%d-%d[%d]\n",
 	  tmp >> 24, (tmp >> 16) & 0xff, tmp & 0xff,
 	  le32_to_cpu(dev->adapter_info.biosbuild));
 	return len;
@@ -1296,7 +1292,7 @@ static ssize_t aac_show_driver_version(struct device *device,
 					struct device_attribute *attr,
 					char *buf)
 {
-	return snprintf(buf, PAGE_SIZE, "%s\n", aac_driver_version);
+	return sysfs_emit(buf, "%s\n", aac_driver_version);
 }
 
 static ssize_t aac_show_serial_number(struct device *device,
@@ -1322,15 +1318,13 @@ static ssize_t aac_show_serial_number(struct device *device,
 static ssize_t aac_show_max_channel(struct device *device,
 				    struct device_attribute *attr, char *buf)
 {
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-	  class_to_shost(device)->max_channel);
+	return sysfs_emit(buf, "%d\n", class_to_shost(device)->max_channel);
 }
 
 static ssize_t aac_show_max_id(struct device *device,
 			       struct device_attribute *attr, char *buf)
 {
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-	  class_to_shost(device)->max_id);
+	return sysfs_emit(buf, "%d\n", class_to_shost(device)->max_id);
 }
 
 static ssize_t aac_store_reset_adapter(struct device *device,
@@ -1360,7 +1354,7 @@ static ssize_t aac_show_reset_adapter(struct device *device,
 	tmp = aac_adapter_check_health(dev);
 	if ((tmp == 0) && dev->in_reset)
 		tmp = -EBUSY;
-	len = snprintf(buf, PAGE_SIZE, "0x%x\n", tmp);
+	len = sysfs_emit(buf, "0x%x\n", tmp);
 	return len;
 }
 
-- 
2.43.0.594.gd9cf4e227d-goog

[PATCH 05/10] scsi: aacraid: linit: Replace snprintf() with the safer scnprintf() variant

[Lee Jones]

There is a general misunderstanding amongst engineers that {v}snprintf()
returns the length of the data *actually* encoded into the destination
array.  However, as per the C99 standard {v}snprintf() really returns
the length of the data that *would have been* written if there were
enough space for it.  This misunderstanding has led to buffer-overruns
in the past.  It's generally considered safer to use the {v}scnprintf()
variants in their place (or even sprintf() in simple cases).  So let's
do that.

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Signed-off-by: Lee Jones <lee@kernel.org>
---
Cc: Adaptec OEM Raid Solutions <aacraid@microsemi.com>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: "PMC-Sierra, Inc" <aacraid@pmc-sierra.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/aacraid/linit.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index 69526e2bd2e78..0e47d9c4cff23 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -583,7 +583,7 @@ static ssize_t aac_show_unique_id(struct device *dev,
 	if (sdev_channel(sdev) == CONTAINER_CHANNEL)
 		memcpy(sn, aac->fsa_dev[sdev_id(sdev)].identifier, sizeof(sn));
 
-	return snprintf(buf, 16 * 2 + 2,
+	return scnprintf(buf, 16 * 2 + 2,
 		"%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X\n",
 		sn[0], sn[1], sn[2], sn[3],
 		sn[4], sn[5], sn[6], sn[7],
@@ -1302,13 +1302,13 @@ static ssize_t aac_show_serial_number(struct device *device,
 	int len = 0;
 
 	if (le32_to_cpu(dev->adapter_info.serial[0]) != 0xBAD0)
-		len = snprintf(buf, 16, "%06X\n",
+		len = scnprintf(buf, 16, "%06X\n",
 		  le32_to_cpu(dev->adapter_info.serial[0]));
 	if (len &&
 	  !memcmp(&dev->supplement_adapter_info.mfg_pcba_serial_no[
 	    sizeof(dev->supplement_adapter_info.mfg_pcba_serial_no)-len],
 	  buf, len-1))
-		len = snprintf(buf, 16, "%.*s\n",
+		len = scnprintf(buf, 16, "%.*s\n",
 		  (int)sizeof(dev->supplement_adapter_info.mfg_pcba_serial_no),
 		  dev->supplement_adapter_info.mfg_pcba_serial_no);
 
-- 
2.43.0.594.gd9cf4e227d-goog

[PATCH 06/10] scsi: aha1542: Replace snprintf() with the safer scnprintf() variant

[Lee Jones]

There is a general misunderstanding amongst engineers that {v}snprintf()
returns the length of the data *actually* encoded into the destination
array.  However, as per the C99 standard {v}snprintf() really returns
the length of the data that *would have been* written if there were
enough space for it.  This misunderstanding has led to buffer-overruns
in the past.  It's generally considered safer to use the {v}scnprintf()
variants in their place (or even sprintf() in simple cases).  So let's
do that.

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Signed-off-by: Lee Jones <lee@kernel.org>
---
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/aha1542.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/aha1542.c b/drivers/scsi/aha1542.c
index 9503996c63256..b5ec7887801a5 100644
--- a/drivers/scsi/aha1542.c
+++ b/drivers/scsi/aha1542.c
@@ -772,7 +772,7 @@ static struct Scsi_Host *aha1542_hw_init(const struct scsi_host_template *tpnt,
 		goto unregister;
 
 	if (sh->dma_channel != 0xFF)
-		snprintf(dma_info, sizeof(dma_info), "DMA %d", sh->dma_channel);
+		scnprintf(dma_info, sizeof(dma_info), "DMA %d", sh->dma_channel);
 	shost_printk(KERN_INFO, sh, "Adaptec AHA-1542 (SCSI-ID %d) at IO 0x%x, IRQ %d, %s\n",
 				sh->this_id, base_io, sh->irq, dma_info);
 	if (aha1542->bios_translation == BIOS_TRANSLATION_25563)
-- 
2.43.0.594.gd9cf4e227d-goog

[PATCH 07/10] scsi: aic7xxx: aicasm: Trivial: Remove trailing whitespace

[Lee Jones]

Removing out of need rather than want.  I wish to make proper changes to
this file, but my editor is insistent on removing whitespace on save.
Rather than go down the rabbit hole of debugging my editor right now,
I'd rather stay productive.  So, out it comes!

EDIT: Found it.  Looks like 5a602de99797b ("Add .editorconfig file for
basic formatting") now forces editors to trim whitespace.

Signed-off-by: Lee Jones <lee@kernel.org>
---
Cc: Hannes Reinecke <hare@suse.com>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/aic7xxx/aicasm/aicasm.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/aic7xxx/aicasm/aicasm.c b/drivers/scsi/aic7xxx/aicasm/aicasm.c
index cd692a4c5f857..8d995186e557a 100644
--- a/drivers/scsi/aic7xxx/aicasm/aicasm.c
+++ b/drivers/scsi/aic7xxx/aicasm/aicasm.c
@@ -132,7 +132,7 @@ main(int argc, char *argv[])
 	/* Set Sentinal scope node */
 	sentinal = scope_alloc();
 	sentinal->type = SCOPE_ROOT;
-	
+
 	includes_search_curdir = 1;
 	appname = *argv;
 	regfile = NULL;
@@ -553,7 +553,7 @@ output_listing(char *ifilename)
 
 		if (func_values == NULL)
 			stop("Could not malloc", EX_OSERR);
-		
+
 		func_values[0] = 0; /* FALSE func */
 		func_count--;
 
@@ -561,13 +561,13 @@ output_listing(char *ifilename)
 		 * Ask the user to fill in the return values for
 		 * the rest of the functions.
 		 */
-		
-		
+
+
 		for (cur_func = SLIST_FIRST(&patch_functions);
 		     cur_func != NULL && SLIST_NEXT(cur_func, links) != NULL;
 		     cur_func = SLIST_NEXT(cur_func, links), func_count--) {
 			int input;
-			
+
 			fprintf(stdout, "\n(%s)\n", cur_func->symbol->name);
 			fprintf(stdout,
 				"Enter the return value for "
@@ -751,7 +751,7 @@ cs_alloc()
 	if (new_cs == NULL)
 		stop("Unable to malloc critical_section object", EX_SOFTWARE);
 	memset(new_cs, 0, sizeof(*new_cs));
-	
+
 	TAILQ_INSERT_TAIL(&cs_tailq, new_cs, links);
 	return new_cs;
 }
@@ -766,7 +766,7 @@ scope_alloc()
 		stop("Unable to malloc scope object", EX_SOFTWARE);
 	memset(new_scope, 0, sizeof(*new_scope));
 	TAILQ_INIT(&new_scope->inner_scope);
-	
+
 	if (SLIST_FIRST(&scope_stack) != NULL) {
 		TAILQ_INSERT_TAIL(&SLIST_FIRST(&scope_stack)->inner_scope,
 				  new_scope, scope_links);
-- 
2.43.0.594.gd9cf4e227d-goog

[PATCH 08/10] scsi: aic7xxx: aicasm: Replace snprintf() with the safer scnprintf() variant

[Lee Jones]

There is a general misunderstanding amongst engineers that {v}snprintf()
returns the length of the data *actually* encoded into the destination
array.  However, as per the C99 standard {v}snprintf() really returns
the length of the data that *would have been* written if there were
enough space for it.  This misunderstanding has led to buffer-overruns
in the past.  It's generally considered safer to use the {v}scnprintf()
variants in their place (or even sprintf() in simple cases).  So let's
do that.

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Signed-off-by: Lee Jones <lee@kernel.org>
---
Cc: Hannes Reinecke <hare@suse.com>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Lee Jones <lee@kernel.org>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/aic7xxx/aicasm/aicasm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/aic7xxx/aicasm/aicasm.c b/drivers/scsi/aic7xxx/aicasm/aicasm.c
index 8d995186e557a..bc758e78d3c06 100644
--- a/drivers/scsi/aic7xxx/aicasm/aicasm.c
+++ b/drivers/scsi/aic7xxx/aicasm/aicasm.c
@@ -331,7 +331,7 @@ back_patch()
 			if (cur_instr->patch_label->type != LABEL) {
 				char buf[255];
 
-				snprintf(buf, sizeof(buf),
+				scnprintf(buf, sizeof(buf),
 					 "Undefined label %s",
 					 cur_instr->patch_label->name);
 				stop(buf, EX_DATAERR);
-- 
2.43.0.594.gd9cf4e227d-goog

[PATCH 09/10] scsi: aic94xx: Remove snprintf() from sysfs call-backs and replace with sysfs_emit()

[Lee Jones]

Since snprintf() has the documented, but still rather strange trait of
returning the length of the data that *would have been* written to the
array if space were available, rather than the arguably more useful
length of data *actually* written, it is usually considered wise to use
something else instead in order to avoid confusion.

In the case of sysfs call-backs, new wrappers exist that do just that.

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Signed-off-by: Lee Jones <lee@kernel.org>
---
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: John Garry <john.g.garry@oracle.com>
Cc: Luben Tuikov <luben_tuikov@adaptec.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/aic94xx/aic94xx_init.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/aic94xx/aic94xx_init.c b/drivers/scsi/aic94xx/aic94xx_init.c
index 8a3340d8d7ad4..c976e4b77c71e 100644
--- a/drivers/scsi/aic94xx/aic94xx_init.c
+++ b/drivers/scsi/aic94xx/aic94xx_init.c
@@ -264,8 +264,7 @@ static ssize_t asd_show_dev_rev(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
 	struct asd_ha_struct *asd_ha = dev_to_asd_ha(dev);
-	return snprintf(buf, PAGE_SIZE, "%s\n",
-			asd_dev_rev[asd_ha->revision_id]);
+	return sysfs_emit(buf, "%s\n", asd_dev_rev[asd_ha->revision_id]);
 }
 static DEVICE_ATTR(aic_revision, S_IRUGO, asd_show_dev_rev, NULL);
 
@@ -273,7 +272,7 @@ static ssize_t asd_show_dev_bios_build(struct device *dev,
 				       struct device_attribute *attr,char *buf)
 {
 	struct asd_ha_struct *asd_ha = dev_to_asd_ha(dev);
-	return snprintf(buf, PAGE_SIZE, "%d\n", asd_ha->hw_prof.bios.bld);
+	return sysfs_emit(buf, "%d\n", asd_ha->hw_prof.bios.bld);
 }
 static DEVICE_ATTR(bios_build, S_IRUGO, asd_show_dev_bios_build, NULL);
 
@@ -281,7 +280,7 @@ static ssize_t asd_show_dev_pcba_sn(struct device *dev,
 				    struct device_attribute *attr, char *buf)
 {
 	struct asd_ha_struct *asd_ha = dev_to_asd_ha(dev);
-	return snprintf(buf, PAGE_SIZE, "%s\n", asd_ha->hw_prof.pcba_sn);
+	return sysfs_emit(buf, "%s\n", asd_ha->hw_prof.pcba_sn);
 }
 static DEVICE_ATTR(pcba_sn, S_IRUGO, asd_show_dev_pcba_sn, NULL);
 
@@ -452,7 +451,7 @@ static ssize_t asd_show_update_bios(struct device *dev,
 	if (asd_ha->bios_status != FLASH_IN_PROGRESS)
 		asd_ha->bios_status = FLASH_OK;
 
-	return snprintf(buf, PAGE_SIZE, "status=%x %s\n",
+	return sysfs_emit(buf, "status=%x %s\n",
 			flash_error_table[i].err_code,
 			flash_error_table[i].reason);
 }
@@ -937,7 +936,7 @@ static int asd_scan_finished(struct Scsi_Host *shost, unsigned long time)
 
 static ssize_t version_show(struct device_driver *driver, char *buf)
 {
-	return snprintf(buf, PAGE_SIZE, "%s\n", ASD_DRIVER_VERSION);
+	return sysfs_emit(buf, "%s\n", ASD_DRIVER_VERSION);
 }
 static DRIVER_ATTR_RO(version);
 
-- 
2.43.0.594.gd9cf4e227d-goog

[PATCH 10/10] scsi: arcmsr: Remove snprintf() from sysfs call-backs and replace with sysfs_emit()

[Lee Jones]

Since snprintf() has the documented, but still rather strange trait of
returning the length of the data that *would have been* written to the
array if space were available, rather than the arguably more useful
length of data *actually* written, it is usually considered wise to use
something else instead in order to avoid confusion.

In the case of sysfs call-backs, new wrappers exist that do just that.

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Signed-off-by: Lee Jones <lee@kernel.org>
---
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: support@areca.com.tw
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/arcmsr/arcmsr_attr.c | 40 ++++++++-----------------------
 1 file changed, 10 insertions(+), 30 deletions(-)

diff --git a/drivers/scsi/arcmsr/arcmsr_attr.c b/drivers/scsi/arcmsr/arcmsr_attr.c
index baeb5e7956902..c430c835503be 100644
--- a/drivers/scsi/arcmsr/arcmsr_attr.c
+++ b/drivers/scsi/arcmsr/arcmsr_attr.c
@@ -258,9 +258,7 @@ static ssize_t
 arcmsr_attr_host_driver_version(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-	return snprintf(buf, PAGE_SIZE,
-			"%s\n",
-			ARCMSR_DRIVER_VERSION);
+	return sysfs_emit(buf, "%s\n", ARCMSR_DRIVER_VERSION);
 }
 
 static ssize_t
@@ -270,9 +268,7 @@ arcmsr_attr_host_driver_posted_cmd(struct device *dev,
 	struct Scsi_Host *host = class_to_shost(dev);
 	struct AdapterControlBlock *acb =
 		(struct AdapterControlBlock *) host->hostdata;
-	return snprintf(buf, PAGE_SIZE,
-			"%4d\n",
-			atomic_read(&acb->ccboutstandingcount));
+	return sysfs_emit(buf, "%4d\n",	atomic_read(&acb->ccboutstandingcount));
 }
 
 static ssize_t
@@ -282,9 +278,7 @@ arcmsr_attr_host_driver_reset(struct device *dev,
 	struct Scsi_Host *host = class_to_shost(dev);
 	struct AdapterControlBlock *acb =
 		(struct AdapterControlBlock *) host->hostdata;
-	return snprintf(buf, PAGE_SIZE,
-			"%4d\n",
-			acb->num_resets);
+	return sysfs_emit(buf, "%4d\n", acb->num_resets);
 }
 
 static ssize_t
@@ -294,9 +288,7 @@ arcmsr_attr_host_driver_abort(struct device *dev,
 	struct Scsi_Host *host = class_to_shost(dev);
 	struct AdapterControlBlock *acb =
 		(struct AdapterControlBlock *) host->hostdata;
-	return snprintf(buf, PAGE_SIZE,
-			"%4d\n",
-			acb->num_aborts);
+	return sysfs_emit(buf, "%4d\n", acb->num_aborts);
 }
 
 static ssize_t
@@ -306,9 +298,7 @@ arcmsr_attr_host_fw_model(struct device *dev, struct device_attribute *attr,
 	struct Scsi_Host *host = class_to_shost(dev);
 	struct AdapterControlBlock *acb =
 		(struct AdapterControlBlock *) host->hostdata;
-	return snprintf(buf, PAGE_SIZE,
-			"%s\n",
-			acb->firm_model);
+	return sysfs_emit(buf, "%s\n", acb->firm_model);
 }
 
 static ssize_t
@@ -319,9 +309,7 @@ arcmsr_attr_host_fw_version(struct device *dev,
 	struct AdapterControlBlock *acb =
 			(struct AdapterControlBlock *) host->hostdata;
 
-	return snprintf(buf, PAGE_SIZE,
-			"%s\n",
-			acb->firm_version);
+	return sysfs_emit(buf, "%s\n", acb->firm_version);
 }
 
 static ssize_t
@@ -332,9 +320,7 @@ arcmsr_attr_host_fw_request_len(struct device *dev,
 	struct AdapterControlBlock *acb =
 		(struct AdapterControlBlock *) host->hostdata;
 
-	return snprintf(buf, PAGE_SIZE,
-			"%4d\n",
-			acb->firm_request_len);
+	return sysfs_emit(buf, "%4d\n", acb->firm_request_len);
 }
 
 static ssize_t
@@ -345,9 +331,7 @@ arcmsr_attr_host_fw_numbers_queue(struct device *dev,
 	struct AdapterControlBlock *acb =
 		(struct AdapterControlBlock *) host->hostdata;
 
-	return snprintf(buf, PAGE_SIZE,
-			"%4d\n",
-			acb->firm_numbers_queue);
+	return sysfs_emit(buf, "%4d\n",	acb->firm_numbers_queue);
 }
 
 static ssize_t
@@ -358,9 +342,7 @@ arcmsr_attr_host_fw_sdram_size(struct device *dev,
 	struct AdapterControlBlock *acb =
 		(struct AdapterControlBlock *) host->hostdata;
 
-	return snprintf(buf, PAGE_SIZE,
-			"%4d\n",
-			acb->firm_sdram_size);
+	return sysfs_emit(buf, "%4d\n", acb->firm_sdram_size);
 }
 
 static ssize_t
@@ -371,9 +353,7 @@ arcmsr_attr_host_fw_hd_channels(struct device *dev,
 	struct AdapterControlBlock *acb =
 		(struct AdapterControlBlock *) host->hostdata;
 
-	return snprintf(buf, PAGE_SIZE,
-			"%4d\n",
-			acb->firm_hd_channels);
+	return sysfs_emit(buf, "%4d\n", acb->firm_hd_channels);
 }
 
 static DEVICE_ATTR(host_driver_version, S_IRUGO, arcmsr_attr_host_driver_version, NULL);
-- 
2.43.0.594.gd9cf4e227d-goog

Re: [PATCH 03/10] scsi: NCR5380: Replace snprintf() with the safer scnprintf() variant

[Geert Uytterhoeven]

Hi Lee,

Thanks for your patch!

On Thu, Feb 8, 2024 at 9:48 AM Lee Jones <lee@kernel.org> wrote:
> There is a general misunderstanding amongst engineers that {v}snprintf()
> returns the length of the data *actually* encoded into the destination
> array.  However, as per the C99 standard {v}snprintf() really returns
> the length of the data that *would have been* written if there were
> enough space for it.  This misunderstanding has led to buffer-overruns
> in the past.  It's generally considered safer to use the {v}scnprintf()
> variants in their place (or even sprintf() in simple cases).  So let's
> do that.

Confused... The return value is not used at all?

> --- a/drivers/scsi/NCR5380.c
> +++ b/drivers/scsi/NCR5380.c
> @@ -421,14 +421,14 @@ static int NCR5380_init(struct Scsi_Host *instance, int flags)
>         if (!hostdata->work_q)
>                 return -ENOMEM;
>
> -       snprintf(hostdata->info, sizeof(hostdata->info),
> -               "%s, irq %d, io_port 0x%lx, base 0x%lx, can_queue %d, cmd_per_lun %d, sg_tablesize %d, this_id %d, flags { %s%s%s}",
> -               instance->hostt->name, instance->irq, hostdata->io_port,
> -               hostdata->base, instance->can_queue, instance->cmd_per_lun,
> -               instance->sg_tablesize, instance->this_id,
> -               hostdata->flags & FLAG_DMA_FIXUP     ? "DMA_FIXUP "     : "",
> -               hostdata->flags & FLAG_NO_PSEUDO_DMA ? "NO_PSEUDO_DMA " : "",
> -               hostdata->flags & FLAG_TOSHIBA_DELAY ? "TOSHIBA_DELAY " : "");
> +       scnprintf(hostdata->info, sizeof(hostdata->info),
> +                "%s, irq %d, io_port 0x%lx, base 0x%lx, can_queue %d, cmd_per_lun %d, sg_tablesize %d, this_id %d, flags { %s%s%s}",
> +                instance->hostt->name, instance->irq, hostdata->io_port,
> +                hostdata->base, instance->can_queue, instance->cmd_per_lun,
> +                instance->sg_tablesize, instance->this_id,
> +                hostdata->flags & FLAG_DMA_FIXUP     ? "DMA_FIXUP "     : "",
> +                hostdata->flags & FLAG_NO_PSEUDO_DMA ? "NO_PSEUDO_DMA " : "",
> +                hostdata->flags & FLAG_TOSHIBA_DELAY ? "TOSHIBA_DELAY " : "");
>
>         NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
>         NCR5380_write(MODE_REG, MR_BASE);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 03/10] scsi: NCR5380: Replace snprintf() with the safer scnprintf() variant

[Lee Jones]

On Thu, 08 Feb 2024, Geert Uytterhoeven wrote:

> Hi Lee,
> 
> Thanks for your patch!
> 
> On Thu, Feb 8, 2024 at 9:48 AM Lee Jones <lee@kernel.org> wrote:
> > There is a general misunderstanding amongst engineers that {v}snprintf()
> > returns the length of the data *actually* encoded into the destination
> > array.  However, as per the C99 standard {v}snprintf() really returns
> > the length of the data that *would have been* written if there were
> > enough space for it.  This misunderstanding has led to buffer-overruns
> > in the past.  It's generally considered safer to use the {v}scnprintf()
> > variants in their place (or even sprintf() in simple cases).  So let's
> > do that.
> 
> Confused... The return value is not used at all?

Future proofing.  The idea of the effort is to rid the use entirely.

 - Usage is inside a sysfs handler passing PAGE_SIZE as the size
   - s/snprintf/sysfs_emit/
 - Usage is inside a sysfs handler passing a bespoke value as the size
   - s/snprintf/scnprintf/
 - Return value used, but does *not* care about overflow
   - s/snprintf/scnprintf/
 - Return value used, caller *does* care about overflow
   - s/snprintf/seq_buf/
 - Return value not used
   - s/snprintf/scnprintf/

This is the final case.

> > --- a/drivers/scsi/NCR5380.c
> > +++ b/drivers/scsi/NCR5380.c
> > @@ -421,14 +421,14 @@ static int NCR5380_init(struct Scsi_Host *instance, int flags)
> >         if (!hostdata->work_q)
> >                 return -ENOMEM;
> >
> > -       snprintf(hostdata->info, sizeof(hostdata->info),
> > -               "%s, irq %d, io_port 0x%lx, base 0x%lx, can_queue %d, cmd_per_lun %d, sg_tablesize %d, this_id %d, flags { %s%s%s}",
> > -               instance->hostt->name, instance->irq, hostdata->io_port,
> > -               hostdata->base, instance->can_queue, instance->cmd_per_lun,
> > -               instance->sg_tablesize, instance->this_id,
> > -               hostdata->flags & FLAG_DMA_FIXUP     ? "DMA_FIXUP "     : "",
> > -               hostdata->flags & FLAG_NO_PSEUDO_DMA ? "NO_PSEUDO_DMA " : "",
> > -               hostdata->flags & FLAG_TOSHIBA_DELAY ? "TOSHIBA_DELAY " : "");
> > +       scnprintf(hostdata->info, sizeof(hostdata->info),
> > +                "%s, irq %d, io_port 0x%lx, base 0x%lx, can_queue %d, cmd_per_lun %d, sg_tablesize %d, this_id %d, flags { %s%s%s}",
> > +                instance->hostt->name, instance->irq, hostdata->io_port,
> > +                hostdata->base, instance->can_queue, instance->cmd_per_lun,
> > +                instance->sg_tablesize, instance->this_id,
> > +                hostdata->flags & FLAG_DMA_FIXUP     ? "DMA_FIXUP "     : "",
> > +                hostdata->flags & FLAG_NO_PSEUDO_DMA ? "NO_PSEUDO_DMA " : "",
> > +                hostdata->flags & FLAG_TOSHIBA_DELAY ? "TOSHIBA_DELAY " : "");
> >
> >         NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
> >         NCR5380_write(MODE_REG, MR_BASE);

-- 
Lee Jones [李琼斯]

Re: [PATCH 00/10] scsi: Replace {v}snprintf() variants with safer alternatives

[Bart Van Assche]

On 2/8/24 00:44, Lee Jones wrote:
> Cc: Andre Hedrick <andre@suse.com>

Please take a look at https://lwn.net/Articles/508222/.

Thanks,

Bart.

Re: [PATCH 00/10] scsi: Replace {v}snprintf() variants with safer alternatives

[Lee Jones]

On Thu, 08 Feb 2024, Bart Van Assche wrote:

> On 2/8/24 00:44, Lee Jones wrote:
> > Cc: Andre Hedrick <andre@suse.com>
> 
> Please take a look at https://lwn.net/Articles/508222/.

get_maintainer.pl pulled it from here:

https://github.com/torvalds/linux/blob/master/drivers/scsi/3w-xxxx.c#L11

I like to involve the people who take the time to list themselves as
authors.  I guess these are likely to go out of date at one point or
another, especially in such a long standing subsystem such as SCSI.  Not
as big of an issue in NFC!

-- 
Lee Jones [李琼斯]

Re: [PATCH 07/10] scsi: aic7xxx: aicasm: Trivial: Remove trailing whitespace

[Kees Cook]

On Thu, Feb 08, 2024 at 08:44:19AM +0000, Lee Jones wrote:
> Removing out of need rather than want.  I wish to make proper changes to
> this file, but my editor is insistent on removing whitespace on save.
> Rather than go down the rabbit hole of debugging my editor right now,
> I'd rather stay productive.  So, out it comes!
> 
> EDIT: Found it.  Looks like 5a602de99797b ("Add .editorconfig file for
> basic formatting") now forces editors to trim whitespace.

If there is a v2, you can "squash" this. :)

> 
> Signed-off-by: Lee Jones <lee@kernel.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 04/10] scsi: aacraid: linit: Remove snprintf() from sysfs call-backs and replace with sysfs_emit()

[Kees Cook]

On Thu, Feb 08, 2024 at 08:44:16AM +0000, Lee Jones wrote:
> Since snprintf() has the documented, but still rather strange trait of
> returning the length of the data that *would have been* written to the
> array if space were available, rather than the arguably more useful
> length of data *actually* written, it is usually considered wise to use
> something else instead in order to avoid confusion.
> 
> In the case of sysfs call-backs, new wrappers exist that do just that.
> 
> Link: https://lwn.net/Articles/69419/
> Link: https://github.com/KSPP/linux/issues/105
> Signed-off-by: Lee Jones <lee@kernel.org>

Yeah, better to use sysfs_emit().

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 05/10] scsi: aacraid: linit: Replace snprintf() with the safer scnprintf() variant

[Kees Cook]

On Thu, Feb 08, 2024 at 08:44:17AM +0000, Lee Jones wrote:
> There is a general misunderstanding amongst engineers that {v}snprintf()
> returns the length of the data *actually* encoded into the destination
> array.  However, as per the C99 standard {v}snprintf() really returns
> the length of the data that *would have been* written if there were
> enough space for it.  This misunderstanding has led to buffer-overruns
> in the past.  It's generally considered safer to use the {v}scnprintf()
> variants in their place (or even sprintf() in simple cases).  So let's
> do that.
> 
> Link: https://lwn.net/Articles/69419/
> Link: https://github.com/KSPP/linux/issues/105
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
> Cc: Adaptec OEM Raid Solutions <aacraid@microsemi.com>
> Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: "PMC-Sierra, Inc" <aacraid@pmc-sierra.com>
> Cc: linux-scsi@vger.kernel.org
> ---
>  drivers/scsi/aacraid/linit.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
> index 69526e2bd2e78..0e47d9c4cff23 100644
> --- a/drivers/scsi/aacraid/linit.c
> +++ b/drivers/scsi/aacraid/linit.c
> @@ -583,7 +583,7 @@ static ssize_t aac_show_unique_id(struct device *dev,
>  	if (sdev_channel(sdev) == CONTAINER_CHANNEL)
>  		memcpy(sn, aac->fsa_dev[sdev_id(sdev)].identifier, sizeof(sn));
>  
> -	return snprintf(buf, 16 * 2 + 2,
> +	return scnprintf(buf, 16 * 2 + 2,
>  		"%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X\n",
>  		sn[0], sn[1], sn[2], sn[3],
>  		sn[4], sn[5], sn[6], sn[7],

I'm confused about this conversion and the original size argument. Isn't
this also a sysfs entry? Size should be PAGE_SIZE, or it should be
replaced with sysfs_emit().

-Kees

> @@ -1302,13 +1302,13 @@ static ssize_t aac_show_serial_number(struct device *device,
>  	int len = 0;
>  
>  	if (le32_to_cpu(dev->adapter_info.serial[0]) != 0xBAD0)
> -		len = snprintf(buf, 16, "%06X\n",
> +		len = scnprintf(buf, 16, "%06X\n",
>  		  le32_to_cpu(dev->adapter_info.serial[0]));
>  	if (len &&
>  	  !memcmp(&dev->supplement_adapter_info.mfg_pcba_serial_no[
>  	    sizeof(dev->supplement_adapter_info.mfg_pcba_serial_no)-len],
>  	  buf, len-1))
> -		len = snprintf(buf, 16, "%.*s\n",
> +		len = scnprintf(buf, 16, "%.*s\n",
>  		  (int)sizeof(dev->supplement_adapter_info.mfg_pcba_serial_no),
>  		  dev->supplement_adapter_info.mfg_pcba_serial_no);
>  
> -- 
> 2.43.0.594.gd9cf4e227d-goog
> 
> 

-- 
Kees Cook

Re: [PATCH 06/10] scsi: aha1542: Replace snprintf() with the safer scnprintf() variant

[Kees Cook]

On Thu, Feb 08, 2024 at 08:44:18AM +0000, Lee Jones wrote:
> There is a general misunderstanding amongst engineers that {v}snprintf()
> returns the length of the data *actually* encoded into the destination
> array.  However, as per the C99 standard {v}snprintf() really returns
> the length of the data that *would have been* written if there were
> enough space for it.  This misunderstanding has led to buffer-overruns
> in the past.  It's generally considered safer to use the {v}scnprintf()
> variants in their place (or even sprintf() in simple cases).  So let's
> do that.

It may be better to do a global replace of snprintf() where the return
value is not used.

$ git grep '\bsnprintf\b' | grep -v = | wc -l
6400

There's plenty of work to do to remove just the ones taking the result:

$ git grep ' = snprintf\b' | wc -l
764

Regardless,

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> 
> Link: https://lwn.net/Articles/69419/
> Link: https://github.com/KSPP/linux/issues/105
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
> Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: linux-scsi@vger.kernel.org
> ---
>  drivers/scsi/aha1542.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/aha1542.c b/drivers/scsi/aha1542.c
> index 9503996c63256..b5ec7887801a5 100644
> --- a/drivers/scsi/aha1542.c
> +++ b/drivers/scsi/aha1542.c
> @@ -772,7 +772,7 @@ static struct Scsi_Host *aha1542_hw_init(const struct scsi_host_template *tpnt,
>  		goto unregister;
>  
>  	if (sh->dma_channel != 0xFF)
> -		snprintf(dma_info, sizeof(dma_info), "DMA %d", sh->dma_channel);
> +		scnprintf(dma_info, sizeof(dma_info), "DMA %d", sh->dma_channel);
>  	shost_printk(KERN_INFO, sh, "Adaptec AHA-1542 (SCSI-ID %d) at IO 0x%x, IRQ %d, %s\n",
>  				sh->this_id, base_io, sh->irq, dma_info);
>  	if (aha1542->bios_translation == BIOS_TRANSLATION_25563)
> -- 
> 2.43.0.594.gd9cf4e227d-goog
> 
> 

-- 
Kees Cook

Re: [PATCH 08/10] scsi: aic7xxx: aicasm: Replace snprintf() with the safer scnprintf() variant

[Kees Cook]

On Thu, Feb 08, 2024 at 08:44:20AM +0000, Lee Jones wrote:
> There is a general misunderstanding amongst engineers that {v}snprintf()
> returns the length of the data *actually* encoded into the destination
> array.  However, as per the C99 standard {v}snprintf() really returns
> the length of the data that *would have been* written if there were
> enough space for it.  This misunderstanding has led to buffer-overruns
> in the past.  It's generally considered safer to use the {v}scnprintf()
> variants in their place (or even sprintf() in simple cases).  So let's
> do that.
> 
> Link: https://lwn.net/Articles/69419/
> Link: https://github.com/KSPP/linux/issues/105
> Signed-off-by: Lee Jones <lee@kernel.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 09/10] scsi: aic94xx: Remove snprintf() from sysfs call-backs and replace with sysfs_emit()

[Kees Cook]

On Thu, Feb 08, 2024 at 08:44:21AM +0000, Lee Jones wrote:
> Since snprintf() has the documented, but still rather strange trait of
> returning the length of the data that *would have been* written to the
> array if space were available, rather than the arguably more useful
> length of data *actually* written, it is usually considered wise to use
> something else instead in order to avoid confusion.
> 
> In the case of sysfs call-backs, new wrappers exist that do just that.

Actually, a treewide replacement for snprintf(dst, PAGE_SIZE, ... to
sysfs_emit might be workable too. Here's the .cocci file I made quickly:

@replace@
expression DST;
@@

- snprintf(DST, PAGE_SIZE,
+ sysfs_emit(DST,
  ...)

This produced almost 1000 changes:
 118 files changed, 964 insertions(+), 958 deletions(-)

Some need some manual examination, like:

arch/powerpc/platforms/ps3/system-bus.c:modalias_show() does:

        return (len >= PAGE_SIZE) ? (PAGE_SIZE - 1) : len;

which isn't needed any more.

Regardless,

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook

Re: [PATCH 10/10] scsi: arcmsr: Remove snprintf() from sysfs call-backs and replace with sysfs_emit()

[Kees Cook]

On Thu, Feb 08, 2024 at 08:44:22AM +0000, Lee Jones wrote:
> Since snprintf() has the documented, but still rather strange trait of
> returning the length of the data that *would have been* written to the
> array if space were available, rather than the arguably more useful
> length of data *actually* written, it is usually considered wise to use
> something else instead in order to avoid confusion.
> 
> In the case of sysfs call-backs, new wrappers exist that do just that.
> 
> Link: https://lwn.net/Articles/69419/
> Link: https://github.com/KSPP/linux/issues/105
> Signed-off-by: Lee Jones <lee@kernel.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 03/10] scsi: NCR5380: Replace snprintf() with the safer scnprintf() variant

[Kees Cook]

On Thu, Feb 08, 2024 at 08:44:15AM +0000, Lee Jones wrote:
> There is a general misunderstanding amongst engineers that {v}snprintf()
> returns the length of the data *actually* encoded into the destination
> array.  However, as per the C99 standard {v}snprintf() really returns
> the length of the data that *would have been* written if there were
> enough space for it.  This misunderstanding has led to buffer-overruns
> in the past.  It's generally considered safer to use the {v}scnprintf()
> variants in their place (or even sprintf() in simple cases).  So let's
> do that.
> 
> Link: https://lwn.net/Articles/69419/
> Link: https://github.com/KSPP/linux/issues/105
> Signed-off-by: Lee Jones <lee@kernel.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 03/10] scsi: NCR5380: Replace snprintf() with the safer scnprintf() variant

[Finn Thain]

On Thu, 8 Feb 2024, Lee Jones wrote:

> On Thu, 08 Feb 2024, Geert Uytterhoeven wrote:
> 
> > 
> > Confused... The return value is not used at all?
> 
> Future proofing. 
> 

Surely a better way to prevent potential future API abuse is by adding 
checkpatch.pl rules. That way does not generate churn.

James or Martin, if you can find some value in this patch, go ahead and 
apply it. I'm afraid I can't see it.

Re: [PATCH 03/10] scsi: NCR5380: Replace snprintf() with the safer scnprintf() variant

[James Bottomley]

On Thu, 2024-02-08 at 10:29 +0000, Lee Jones wrote:
> On Thu, 08 Feb 2024, Geert Uytterhoeven wrote:
> 
> > Hi Lee,
> > 
> > Thanks for your patch!
> > 
> > On Thu, Feb 8, 2024 at 9:48 AM Lee Jones <lee@kernel.org> wrote:
> > > There is a general misunderstanding amongst engineers that
> > > {v}snprintf()
> > > returns the length of the data *actually* encoded into the
> > > destination
> > > array.  However, as per the C99 standard {v}snprintf() really
> > > returns
> > > the length of the data that *would have been* written if there
> > > were
> > > enough space for it.  This misunderstanding has led to buffer-
> > > overruns
> > > in the past.  It's generally considered safer to use the
> > > {v}scnprintf()
> > > variants in their place (or even sprintf() in simple cases).  So
> > > let's
> > > do that.
> > 
> > Confused... The return value is not used at all?
> 
> Future proofing.  The idea of the effort is to rid the use entirely.
> 
>  - Usage is inside a sysfs handler passing PAGE_SIZE as the size
>    - s/snprintf/sysfs_emit/
>  - Usage is inside a sysfs handler passing a bespoke value as the
> size
>    - s/snprintf/scnprintf/
>  - Return value used, but does *not* care about overflow
>    - s/snprintf/scnprintf/
>  - Return value used, caller *does* care about overflow
>    - s/snprintf/seq_buf/
>  - Return value not used
>    - s/snprintf/scnprintf/
> 
> This is the final case.

To re-ask Geert's question: the last case can't ever lead to a bug or
problem, what value does churning the kernel to change it provide?  As
Finn said, if we want to deprecate it as a future pattern, put it in
checkpatch.

James

Re: [PATCH 03/10] scsi: NCR5380: Replace snprintf() with the safer scnprintf() variant

[Lee Jones]

On Sat, 10 Feb 2024, James Bottomley wrote:

> On Thu, 2024-02-08 at 10:29 +0000, Lee Jones wrote:
> > On Thu, 08 Feb 2024, Geert Uytterhoeven wrote:
> > 
> > > Hi Lee,
> > > 
> > > Thanks for your patch!
> > > 
> > > On Thu, Feb 8, 2024 at 9:48 AM Lee Jones <lee@kernel.org> wrote:
> > > > There is a general misunderstanding amongst engineers that
> > > > {v}snprintf()
> > > > returns the length of the data *actually* encoded into the
> > > > destination
> > > > array.  However, as per the C99 standard {v}snprintf() really
> > > > returns
> > > > the length of the data that *would have been* written if there
> > > > were
> > > > enough space for it.  This misunderstanding has led to buffer-
> > > > overruns
> > > > in the past.  It's generally considered safer to use the
> > > > {v}scnprintf()
> > > > variants in their place (or even sprintf() in simple cases).  So
> > > > let's
> > > > do that.
> > > 
> > > Confused... The return value is not used at all?
> > 
> > Future proofing.  The idea of the effort is to rid the use entirely.
> > 
> >  - Usage is inside a sysfs handler passing PAGE_SIZE as the size
> >    - s/snprintf/sysfs_emit/
> >  - Usage is inside a sysfs handler passing a bespoke value as the
> > size
> >    - s/snprintf/scnprintf/
> >  - Return value used, but does *not* care about overflow
> >    - s/snprintf/scnprintf/
> >  - Return value used, caller *does* care about overflow
> >    - s/snprintf/seq_buf/
> >  - Return value not used
> >    - s/snprintf/scnprintf/
> > 
> > This is the final case.
> 
> To re-ask Geert's question: the last case can't ever lead to a bug or
> problem, what value does churning the kernel to change it provide?  As
> Finn said, if we want to deprecate it as a future pattern, put it in
> checkpatch.

Adding this to checkpatch is a good idea.

What if we also take Kees's suggestion and hit all of these found in
SCSI in one patch to keep the churn down to a minimum?

-- 
Lee Jones [李琼斯]

Re: [PATCH 03/10] scsi: NCR5380: Replace snprintf() with the safer scnprintf() variant

[James Bottomley]

On Mon, 2024-02-19 at 15:23 +0000, Lee Jones wrote:
> On Sat, 10 Feb 2024, James Bottomley wrote:
> 
> > On Thu, 2024-02-08 at 10:29 +0000, Lee Jones wrote:
> > > On Thu, 08 Feb 2024, Geert Uytterhoeven wrote:
> > > 
> > > > Hi Lee,
> > > > 
> > > > Thanks for your patch!
> > > > 
> > > > On Thu, Feb 8, 2024 at 9:48 AM Lee Jones <lee@kernel.org>
> > > > wrote:
> > > > > There is a general misunderstanding amongst engineers that
> > > > > {v}snprintf() returns the length of the data *actually*
> > > > > encoded into the destination array.  However, as per the C99
> > > > > standard {v}snprintf() really returns the length of the data
> > > > > that *would have been* written if there were enough space for
> > > > > it.  This misunderstanding has led to buffer-overruns in the
> > > > > past.  It's generally considered safer to use the
> > > > > {v}scnprintf() variants in their place (or even sprintf() in
> > > > > simple cases).  So let's do that.
> > > > 
> > > > Confused... The return value is not used at all?
> > > 
> > > Future proofing.  The idea of the effort is to rid the use
> > > entirely.
> > > 
> > >  - Usage is inside a sysfs handler passing PAGE_SIZE as the size
> > >    - s/snprintf/sysfs_emit/
> > >  - Usage is inside a sysfs handler passing a bespoke value as the
> > > size
> > >    - s/snprintf/scnprintf/
> > >  - Return value used, but does *not* care about overflow
> > >    - s/snprintf/scnprintf/
> > >  - Return value used, caller *does* care about overflow
> > >    - s/snprintf/seq_buf/
> > >  - Return value not used
> > >    - s/snprintf/scnprintf/
> > > 
> > > This is the final case.
> > 
> > To re-ask Geert's question: the last case can't ever lead to a bug
> > orproblem, what value does churning the kernel to change it
> > provide? As Finn said, if we want to deprecate it as a future
> > pattern, put it in checkpatch.
> 
> Adding this to checkpatch is a good idea.
> 
> What if we also take Kees's suggestion and hit all of these found in
> SCSI in one patch to keep the churn down to a minimum?

That doesn't fix the churn problem because you're still changing the
source.  For ancient drivers, we keep the changes to a minimum to avoid
introducing inadvertent bugs which aren't discovered until months
later.  If there's no actual bug in the driver, there's no reason to
change the code.

Regards,

James

Re: [PATCH 03/10] scsi: NCR5380: Replace snprintf() with the safer scnprintf() variant

[Kees Cook]

On Mon, Feb 19, 2024 at 03:23:12PM +0000, Lee Jones wrote:
> Adding this to checkpatch is a good idea.

Yeah, please do. You can look at the "strncpy -> strscpy" check that is
already in there for an example.

> 
> What if we also take Kees's suggestion and hit all of these found in
> SCSI in one patch to keep the churn down to a minimum?

We don't have to focus on SCSI even. At the end of the next -rc1, I can
send a tree-wide patch (from Coccinelle) that'll convert all snprintf()
uses that don't check a return value into scnprintf(). For example,
this seems to do the trick:

@scnprintf depends on !(file in "tools") && !(file in "samples")@
@@

-snprintf
+scnprintf
 (...);


Results in:

 2252 files changed, 4795 insertions(+), 4795 deletions(-)

-Kees

-- 
Kees Cook

Re: [PATCH 03/10] scsi: NCR5380: Replace snprintf() with the safer scnprintf() variant

[Lee Jones]

On Mon, 19 Feb 2024, Kees Cook wrote:

> On Mon, Feb 19, 2024 at 03:23:12PM +0000, Lee Jones wrote:
> > Adding this to checkpatch is a good idea.
> 
> Yeah, please do. You can look at the "strncpy -> strscpy" check that is
> already in there for an example.
> 
> > 
> > What if we also take Kees's suggestion and hit all of these found in
> > SCSI in one patch to keep the churn down to a minimum?
> 
> We don't have to focus on SCSI even. At the end of the next -rc1, I can

When I've conducted similar work before, I've taken it subsystem by
subsystem.  However, if you're happy to co-ordinate with the big penguin
et al. and get them all with a treewide patch, please go for it.

> send a tree-wide patch (from Coccinelle) that'll convert all snprintf()
> uses that don't check a return value into scnprintf(). For example,
> this seems to do the trick:
> 
> @scnprintf depends on !(file in "tools") && !(file in "samples")@
> @@
> 
> -snprintf
> +scnprintf
>  (...);
> 
> 
> Results in:
> 
>  2252 files changed, 4795 insertions(+), 4795 deletions(-)

Super!

-- 
Lee Jones [李琼斯]

Re: [PATCH 03/10] scsi: NCR5380: Replace snprintf() with the safer scnprintf() variant

[Lee Jones]

On Mon, 19 Feb 2024, James Bottomley wrote:

> On Mon, 2024-02-19 at 15:23 +0000, Lee Jones wrote:
> > On Sat, 10 Feb 2024, James Bottomley wrote:
> > 
> > > On Thu, 2024-02-08 at 10:29 +0000, Lee Jones wrote:
> > > > On Thu, 08 Feb 2024, Geert Uytterhoeven wrote:
> > > > 
> > > > > Hi Lee,
> > > > > 
> > > > > Thanks for your patch!
> > > > > 
> > > > > On Thu, Feb 8, 2024 at 9:48 AM Lee Jones <lee@kernel.org>
> > > > > wrote:
> > > > > > There is a general misunderstanding amongst engineers that
> > > > > > {v}snprintf() returns the length of the data *actually*
> > > > > > encoded into the destination array.  However, as per the C99
> > > > > > standard {v}snprintf() really returns the length of the data
> > > > > > that *would have been* written if there were enough space for
> > > > > > it.  This misunderstanding has led to buffer-overruns in the
> > > > > > past.  It's generally considered safer to use the
> > > > > > {v}scnprintf() variants in their place (or even sprintf() in
> > > > > > simple cases).  So let's do that.
> > > > > 
> > > > > Confused... The return value is not used at all?
> > > > 
> > > > Future proofing.  The idea of the effort is to rid the use
> > > > entirely.
> > > > 
> > > >  - Usage is inside a sysfs handler passing PAGE_SIZE as the size
> > > >    - s/snprintf/sysfs_emit/
> > > >  - Usage is inside a sysfs handler passing a bespoke value as the
> > > > size
> > > >    - s/snprintf/scnprintf/
> > > >  - Return value used, but does *not* care about overflow
> > > >    - s/snprintf/scnprintf/
> > > >  - Return value used, caller *does* care about overflow
> > > >    - s/snprintf/seq_buf/
> > > >  - Return value not used
> > > >    - s/snprintf/scnprintf/
> > > > 
> > > > This is the final case.
> > > 
> > > To re-ask Geert's question: the last case can't ever lead to a bug
> > > orproblem, what value does churning the kernel to change it
> > > provide? As Finn said, if we want to deprecate it as a future
> > > pattern, put it in checkpatch.
> > 
> > Adding this to checkpatch is a good idea.
> > 
> > What if we also take Kees's suggestion and hit all of these found in
> > SCSI in one patch to keep the churn down to a minimum?
> 
> That doesn't fix the churn problem because you're still changing the
> source.  For ancient drivers, we keep the changes to a minimum to avoid
> introducing inadvertent bugs which aren't discovered until months
> later.  If there's no actual bug in the driver, there's no reason to
> change the code.

Okay, no problem.  Would you like me to drop these from the set and
resubmit or are you happy to cherry-pick the remainder?

-- 
Lee Jones [李琼斯]