[PATCH 0/7] scsi: replace deprecated strncpy

[PATCH 0/7] scsi: replace deprecated strncpy

[Justin Stitt]

This series contains multiple replacements of strncpy throughout the
scsi subsystem.

strncpy() is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces. The details of each replacement will be in their respective
patch.

---
Justin Stitt (7):
      scsi: mpi3mr: replace deprecated strncpy with strscpy
      scsi: mpt3sas: replace deprecated strncpy with strscpy
      scsi: qedf: replace deprecated strncpy with strscpy
      scsi: qla4xxx: replace deprecated strncpy with strscpy
      scsi: devinfo: replace strncpy and manual pad
      scsi: smartpqi: replace deprecated strncpy with strscpy
      scsi: wd33c93: replace deprecated strncpy with strscpy

 drivers/net/ethernet/qlogic/qed/qed_main.c |  2 +-
 drivers/scsi/mpi3mr/mpi3mr_fw.c            |  8 ++++----
 drivers/scsi/mpt3sas/mpt3sas_base.c        |  2 +-
 drivers/scsi/mpt3sas/mpt3sas_transport.c   | 18 +++++++++---------
 drivers/scsi/qedf/qedf_main.c              |  2 +-
 drivers/scsi/qla4xxx/ql4_mbx.c             | 17 ++++++++++++-----
 drivers/scsi/qla4xxx/ql4_os.c              | 14 +++++++-------
 drivers/scsi/scsi_devinfo.c                | 18 ++++++++++--------
 drivers/scsi/smartpqi/smartpqi_init.c      |  5 ++---
 drivers/scsi/wd33c93.c                     |  4 +---
 10 files changed, 48 insertions(+), 42 deletions(-)
---
base-commit: 39133352cbed6626956d38ed72012f49b0421e7b
change-id: 20240222-strncpy-drivers-scsi-mpi3mr-mpi3mr_fw-c-1b130d51bdcc

Best regards,
--
Justin Stitt <justinstitt@google.com>

[PATCH 1/7] scsi: mpi3mr: replace deprecated strncpy with strscpy

[Justin Stitt]

Really, there's no bug with the current code. Let's just ditch strncpy()
all together.

Since strscpy() will not NUL-pad the destination buffer let's
NUL-initialize @personality; just like the others.

Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
 drivers/scsi/mpi3mr/mpi3mr_fw.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
index 528f19f782f2..c3e55eedfa5e 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
@@ -3685,20 +3685,20 @@ static void
 mpi3mr_print_ioc_info(struct mpi3mr_ioc *mrioc)
 {
 	int i = 0, bytes_written = 0;
-	char personality[16];
+	char personality[16] = {0};
 	char protocol[50] = {0};
 	char capabilities[100] = {0};
 	struct mpi3mr_compimg_ver *fwver = &mrioc->facts.fw_ver;
 
 	switch (mrioc->facts.personality) {
 	case MPI3_IOCFACTS_FLAGS_PERSONALITY_EHBA:
-		strncpy(personality, "Enhanced HBA", sizeof(personality));
+		strscpy(personality, "Enhanced HBA", sizeof(personality));
 		break;
 	case MPI3_IOCFACTS_FLAGS_PERSONALITY_RAID_DDR:
-		strncpy(personality, "RAID", sizeof(personality));
+		strscpy(personality, "RAID", sizeof(personality));
 		break;
 	default:
-		strncpy(personality, "Unknown", sizeof(personality));
+		strscpy(personality, "Unknown", sizeof(personality));
 		break;
 	}
 

-- 
2.44.0.rc0.258.g7320e95886-goog

[PATCH 2/7] scsi: mpt3sas: replace deprecated strncpy with strscpy

[Justin Stitt]

The replacement in mpt3sas_base.c is a trivial one because desc is
already zero-initialized meaning there is no functional change here.

For mpt3sas_transport.c, we know edev is zero-initialized as well while
manufacture_reply comes from dma_alloc_coherent(). No functional change
here either.

For all cases, use the more idiomatic strscpy() usage of:
strscpy(dest, src, sizeof(dest))

Signed-off-by: Justin Stitt <justinstitt@google.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c      |  2 +-
 drivers/scsi/mpt3sas/mpt3sas_transport.c | 18 +++++++++---------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 8761bc58d965..c1e421cb8533 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -4774,7 +4774,7 @@ _base_display_ioc_capabilities(struct MPT3SAS_ADAPTER *ioc)
 	char desc[17] = {0};
 	u32 iounit_pg1_flags;
 
-	strncpy(desc, ioc->manu_pg0.ChipName, 16);
+	strscpy(desc, ioc->manu_pg0.ChipName, sizeof(desc));
 	ioc_info(ioc, "%s: FWVersion(%02d.%02d.%02d.%02d), ChipRevision(0x%02x)\n",
 		 desc,
 		 (ioc->facts.FWVersion.Word & 0xFF000000) >> 24,
diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c b/drivers/scsi/mpt3sas/mpt3sas_transport.c
index 421ea511b664..76f9a9177198 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
@@ -458,17 +458,17 @@ _transport_expander_report_manufacture(struct MPT3SAS_ADAPTER *ioc,
 			goto out;
 
 		manufacture_reply = data_out + sizeof(struct rep_manu_request);
-		strncpy(edev->vendor_id, manufacture_reply->vendor_id,
-		     SAS_EXPANDER_VENDOR_ID_LEN);
-		strncpy(edev->product_id, manufacture_reply->product_id,
-		     SAS_EXPANDER_PRODUCT_ID_LEN);
-		strncpy(edev->product_rev, manufacture_reply->product_rev,
-		     SAS_EXPANDER_PRODUCT_REV_LEN);
+		strscpy(edev->vendor_id, manufacture_reply->vendor_id,
+			sizeof(edev->vendor_id));
+		strscpy(edev->product_id, manufacture_reply->product_id,
+			sizeof(edev->product_id));
+		strscpy(edev->product_rev, manufacture_reply->product_rev,
+			sizeof(edev->product_rev));
 		edev->level = manufacture_reply->sas_format & 1;
 		if (edev->level) {
-			strncpy(edev->component_vendor_id,
-			    manufacture_reply->component_vendor_id,
-			     SAS_EXPANDER_COMPONENT_VENDOR_ID_LEN);
+			strscpy(edev->component_vendor_id,
+				manufacture_reply->component_vendor_id,
+				sizeof(edev->component_vendor_id));
 			tmp = (u8 *)&manufacture_reply->component_id;
 			edev->component_id = tmp[0] << 8 | tmp[1];
 			edev->component_revision_id =

-- 
2.44.0.rc0.258.g7320e95886-goog

[PATCH 3/7] scsi: qedf: replace deprecated strncpy with strscpy

[Justin Stitt]

We expect slowpath_params.name to be NUL-terminated based on its future
usage with other string APIs:

|	static int qed_slowpath_start(struct qed_dev *cdev,
|				      struct qed_slowpath_params *params)
...
|	strscpy(drv_version.name, params->name,
|		MCP_DRV_VER_STR_SIZE - 4);

Moreover, NUL-padding is not necessary as the only use for this slowpath
name parameter is to copy into the drv_version.name field.

Also, let's prefer using strscpy(src, dest, sizeof(src)) in two
instances (one of which is outside of the scsi system but it is trivial
and related to this patch).

We can see the drv_version.name size here:
|	struct qed_mcp_drv_version {
|		u32	version;
|		u8	name[MCP_DRV_VER_STR_SIZE - 4];
|	};

Signed-off-by: Justin Stitt <justinstitt@google.com>
---
 drivers/net/ethernet/qlogic/qed/qed_main.c | 2 +-
 drivers/scsi/qedf/qedf_main.c              | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c b/drivers/net/ethernet/qlogic/qed/qed_main.c
index c278f8893042..d39e198fe8db 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_main.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_main.c
@@ -1351,7 +1351,7 @@ static int qed_slowpath_start(struct qed_dev *cdev,
 				      (params->drv_rev << 8) |
 				      (params->drv_eng);
 		strscpy(drv_version.name, params->name,
-			MCP_DRV_VER_STR_SIZE - 4);
+			sizeof(drv_version.name));
 		rc = qed_mcp_send_drv_version(hwfn, hwfn->p_main_ptt,
 					      &drv_version);
 		if (rc) {
diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c
index a58353b7b4e8..fd12439cbaab 100644
--- a/drivers/scsi/qedf/qedf_main.c
+++ b/drivers/scsi/qedf/qedf_main.c
@@ -3468,7 +3468,7 @@ static int __qedf_probe(struct pci_dev *pdev, int mode)
 	slowpath_params.drv_minor = QEDF_DRIVER_MINOR_VER;
 	slowpath_params.drv_rev = QEDF_DRIVER_REV_VER;
 	slowpath_params.drv_eng = QEDF_DRIVER_ENG_VER;
-	strncpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
+	strscpy(slowpath_params.name, "qedf", sizeof(slowpath_params.name));
 	rc = qed_ops->common->slowpath_start(qedf->cdev, &slowpath_params);
 	if (rc) {
 		QEDF_ERR(&(qedf->dbg_ctx), "Cannot start slowpath.\n");

-- 
2.44.0.rc0.258.g7320e95886-goog

[PATCH 4/7] scsi: qla4xxx: replace deprecated strncpy with strscpy

[Justin Stitt]

Replace 3 instances of strncpy in ql4_mbx.c

No bugs exist in the current implementation as some care was taken to
ensure the write length was decreased by one to leave some space for a
NUL-byte. However, instead of using strncpy(dest, src, LEN-1) we can opt
for strscpy(dest, src, sizeof(dest)) which will result in
NUL-termination as well. It should be noted that the entire chap_table
is zero-allocated so the NUL-padding provided by strncpy is not needed.

While here, I noticed that MIN_CHAP_SECRET_LEN was not used anywhere.
Since strscpy gives us the number of bytes copied into the destination
buffer (or an -E2BIG) we can check both for an error during copying and
also for a non-length compliant secret. Add a new jump label so we can
properly clean up our chap_table should we have to abort due to bad
secret.

The third instance in this file involves some more peculiar handling of
strings:
|	uint32_t mbox_cmd[MBOX_REG_COUNT];
|	...
|	memset(&mbox_cmd, 0, sizeof(mbox_cmd));
|	...
|	mbox_cmd[0] = MBOX_CMD_SET_PARAM;
|	if (param == SET_DRVR_VERSION) {
|		mbox_cmd[1] = SET_DRVR_VERSION;
|		strncpy((char *)&mbox_cmd[2], QLA4XXX_DRIVER_VERSION,
|			MAX_DRVR_VER_LEN - 1);

mbox_cmd has a size of 8:
|	#define MBOX_REG_COUNT 8
... and its type width is 4 bytes. Hence, we have 32 bytes to work with
here. The first 4 bytes are used as a flag for the MBOX_CMD_SET_PARAM.
The next 4 bytes are used for SET_DRVR_VERSION. We now have 32-8=24
bytes remaining -- which thankfully is what MAX_DRVR_VER_LEN is equal to
|	#define MAX_DRVR_VER_LEN                    24

... and the thing we're copying into this pseudo-string buffer is
|	#define QLA4XXX_DRIVER_VERSION        "5.04.00-k6"

... which is great because its less than 24 bytes (therefore we aren't
truncating the source).

All to say, there's no bug in the existing implementation (yay!) but we
can clean the code up a bit by using strscpy().

In ql4_os.c, there aren't any strncpy() uses to replace but there are
some existing strscpy() calls that could be made more idiomatic. Where
possible, use strscpy(dest, src, sizeof(dest)). Note that
chap_rec->password has a size of ISCSI_CHAP_AUTH_SECRET_MAX_LEN
|	#define ISCSI_CHAP_AUTH_SECRET_MAX_LEN	256
... while the current strscpy usage uses QL4_CHAP_MAX_SECRET_LEN
|	#define QL4_CHAP_MAX_SECRET_LEN 100
... however since chap_table->secret was set and bounded properly in its
string assignment its probably safe here to switch over to sizeof().

|	struct iscsi_chap_rec {
	...
|		char username[ISCSI_CHAP_AUTH_NAME_MAX_LEN];
|		uint8_t password[ISCSI_CHAP_AUTH_SECRET_MAX_LEN];
	...
|	};

|	strscpy(chap_rec->password, chap_table->secret,
|		QL4_CHAP_MAX_SECRET_LEN);

Signed-off-by: Justin Stitt <justinstitt@google.com>
---
 drivers/scsi/qla4xxx/ql4_mbx.c | 17 ++++++++++++-----
 drivers/scsi/qla4xxx/ql4_os.c  | 14 +++++++-------
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/qla4xxx/ql4_mbx.c b/drivers/scsi/qla4xxx/ql4_mbx.c
index 249f1d7021d4..75125d2021f5 100644
--- a/drivers/scsi/qla4xxx/ql4_mbx.c
+++ b/drivers/scsi/qla4xxx/ql4_mbx.c
@@ -1641,6 +1641,7 @@ int qla4xxx_set_chap(struct scsi_qla_host *ha, char *username, char *password,
 	struct ql4_chap_table *chap_table;
 	uint32_t chap_size = 0;
 	dma_addr_t chap_dma;
+	ssize_t secret_len;
 
 	chap_table = dma_pool_zalloc(ha->chap_dma_pool, GFP_KERNEL, &chap_dma);
 	if (chap_table == NULL) {
@@ -1652,9 +1653,13 @@ int qla4xxx_set_chap(struct scsi_qla_host *ha, char *username, char *password,
 		chap_table->flags |= BIT_6; /* peer */
 	else
 		chap_table->flags |= BIT_7; /* local */
-	chap_table->secret_len = strlen(password);
-	strncpy(chap_table->secret, password, MAX_CHAP_SECRET_LEN - 1);
-	strncpy(chap_table->name, username, MAX_CHAP_NAME_LEN - 1);
+
+	secret_len = strscpy(chap_table->secret, password,
+			     sizeof(chap_table->secret));
+	if (secret_len < MIN_CHAP_SECRET_LEN)
+		goto cleanup_chap_table;
+	chap_table->secret_len = (uint8_t)secret_len;
+	strscpy(chap_table->name, username, sizeof(chap_table->name));
 	chap_table->cookie = cpu_to_le16(CHAP_VALID_COOKIE);
 
 	if (is_qla40XX(ha)) {
@@ -1679,6 +1684,8 @@ int qla4xxx_set_chap(struct scsi_qla_host *ha, char *username, char *password,
 		memcpy((struct ql4_chap_table *)ha->chap_list + idx,
 		       chap_table, sizeof(struct ql4_chap_table));
 	}
+
+cleanup_chap_table:
 	dma_pool_free(ha->chap_dma_pool, chap_table, chap_dma);
 	if (rval != QLA_SUCCESS)
 		ret =  -EINVAL;
@@ -2281,8 +2288,8 @@ int qla4_8xxx_set_param(struct scsi_qla_host *ha, int param)
 	mbox_cmd[0] = MBOX_CMD_SET_PARAM;
 	if (param == SET_DRVR_VERSION) {
 		mbox_cmd[1] = SET_DRVR_VERSION;
-		strncpy((char *)&mbox_cmd[2], QLA4XXX_DRIVER_VERSION,
-			MAX_DRVR_VER_LEN - 1);
+		strscpy((char *)&mbox_cmd[2], QLA4XXX_DRIVER_VERSION,
+			MAX_DRVR_VER_LEN);
 	} else {
 		ql4_printk(KERN_ERR, ha, "%s: invalid parameter 0x%x\n",
 			   __func__, param);
diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
index 675332e49a7b..17cccd14765f 100644
--- a/drivers/scsi/qla4xxx/ql4_os.c
+++ b/drivers/scsi/qla4xxx/ql4_os.c
@@ -799,10 +799,10 @@ static int qla4xxx_get_chap_list(struct Scsi_Host *shost, uint16_t chap_tbl_idx,
 
 		chap_rec->chap_tbl_idx = i;
 		strscpy(chap_rec->username, chap_table->name,
-			ISCSI_CHAP_AUTH_NAME_MAX_LEN);
-		strscpy(chap_rec->password, chap_table->secret,
-			QL4_CHAP_MAX_SECRET_LEN);
-		chap_rec->password_length = chap_table->secret_len;
+			sizeof(chap_rec->username));
+		chap_rec->password_length = strscpy(chap_rec->password,
+						    chap_table->secret,
+						    sizeof(chap_rec->password));
 
 		if (chap_table->flags & BIT_7) /* local */
 			chap_rec->chap_type = CHAP_TYPE_OUT;
@@ -6291,8 +6291,8 @@ static void qla4xxx_get_param_ddb(struct ddb_entry *ddb_entry,
 
 	tddb->tpgt = sess->tpgt;
 	tddb->port = conn->persistent_port;
-	strscpy(tddb->iscsi_name, sess->targetname, ISCSI_NAME_SIZE);
-	strscpy(tddb->ip_addr, conn->persistent_address, DDB_IPADDR_LEN);
+	strscpy(tddb->iscsi_name, sess->targetname, sizeof(tddb->iscsi_name));
+	strscpy(tddb->ip_addr, conn->persistent_address, sizeof(tddb->ip_addr));
 }
 
 static void qla4xxx_convert_param_ddb(struct dev_db_entry *fw_ddb_entry,
@@ -7792,7 +7792,7 @@ static int qla4xxx_sysfs_ddb_logout(struct iscsi_bus_flash_session *fnode_sess,
 	}
 
 	strscpy(flash_tddb->iscsi_name, fnode_sess->targetname,
-		ISCSI_NAME_SIZE);
+		sizeof(flash_tddb->iscsi_name));
 
 	if (!strncmp(fnode_sess->portal_type, PORTAL_TYPE_IPV6, 4))
 		sprintf(flash_tddb->ip_addr, "%pI6", fnode_conn->ipaddress);

-- 
2.44.0.rc0.258.g7320e95886-goog

[PATCH 5/7] scsi: devinfo: replace strncpy and manual pad

[Justin Stitt]

Depending on the state of @compatible, we are going to do different
things with our @to buffer.

When @compatible is true we want a NUL-term'd and NUL-padded destination
buffer. Conversely, if @compatible is false we just want a space-padded
destination buffer (no NUL-term required).

As per:
/**
 * scsi_dev_info_list_add_keyed - add one dev_info list entry.
 * @compatible: if true, null terminate short strings.  Otherwise space pad.
...

Note that we can't easily use `strtomem_pad` here as the size of the @to
buffer is unknown to the compiler due to indirection layers.

Now, the intent of the code is more clear (I probably didn't even need
to add a comment -- that's how clear it is).

Signed-off-by: Justin Stitt <justinstitt@google.com>
---
 drivers/scsi/scsi_devinfo.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index 3fcaf10a9dfe..2d3dbce25629 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -293,14 +293,16 @@ static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,
 	size_t from_length;
 
 	from_length = strlen(from);
-	/* This zero-pads the destination */
-	strncpy(to, from, to_length);
-	if (from_length < to_length && !compatible) {
-		/*
-		 * space pad the string if it is short.
-		 */
-		memset(&to[from_length], ' ', to_length - from_length);
-	}
+
+	/*
+	 * null pad and null terminate if compatible
+	 * otherwise space pad
+	 */
+	if (compatible)
+		strscpy_pad(to, from, to_length);
+	else
+		memcpy_and_pad(to, to_length, from, from_length, ' ');
+
 	if (from_length > to_length)
 		 printk(KERN_WARNING "%s: %s string '%s' is too long\n",
 			__func__, name, from);

-- 
2.44.0.rc0.258.g7320e95886-goog

[PATCH 6/7] scsi: smartpqi: replace deprecated strncpy with strscpy

[Justin Stitt]

buffer->driver_version is sized 32:
|	struct bmic_host_wellness_driver_version {
|	...
|		char	driver_version[32];
... the source string "Linux " + DRIVER_VERISON is sized at 16. There's
really no bug in the existing code since the buffers are sized
appropriately with great care taken to manually NUL-terminate the
destination buffer. Nonetheless, let's make the swap over to strscpy()
for robustness' (and readability's) sake.

Signed-off-by: Justin Stitt <justinstitt@google.com>
---
 drivers/scsi/smartpqi/smartpqi_init.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
index ceff1ec13f9e..bfe6f42e8e96 100644
--- a/drivers/scsi/smartpqi/smartpqi_init.c
+++ b/drivers/scsi/smartpqi/smartpqi_init.c
@@ -1041,9 +1041,8 @@ static int pqi_write_driver_version_to_host_wellness(
 	buffer->driver_version_tag[1] = 'V';
 	put_unaligned_le16(sizeof(buffer->driver_version),
 		&buffer->driver_version_length);
-	strncpy(buffer->driver_version, "Linux " DRIVER_VERSION,
-		sizeof(buffer->driver_version) - 1);
-	buffer->driver_version[sizeof(buffer->driver_version) - 1] = '\0';
+	strscpy(buffer->driver_version, "Linux " DRIVER_VERSION,
+		sizeof(buffer->driver_version));
 	buffer->dont_write_tag[0] = 'D';
 	buffer->dont_write_tag[1] = 'W';
 	buffer->end_tag[0] = 'Z';

-- 
2.44.0.rc0.258.g7320e95886-goog

[PATCH 7/7] scsi: wd33c93: replace deprecated strncpy with strscpy

[Justin Stitt]

@p1 is assigned to @setup_buffer and then we manually assign a NUL-byte
at the first index. This renders the following strlen() call useless.
Moreover, we don't need to reassign p1 to setup_buffer for any reason --
neither do we need to manually set a NUL-byte at the end. strscpy()
resolves all this code making it easier to read.

Even considering the path where @str is falsey, the manual NUL-byte
assignment is useless as setup_buffer is declared with static storage
duration in the top-level scope which should NUL-initialize the whole
buffer.

Signed-off-by: Justin Stitt <justinstitt@google.com>
---
 drivers/scsi/wd33c93.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/scsi/wd33c93.c b/drivers/scsi/wd33c93.c
index e4fafc77bd20..a44b60c9004a 100644
--- a/drivers/scsi/wd33c93.c
+++ b/drivers/scsi/wd33c93.c
@@ -1721,9 +1721,7 @@ wd33c93_setup(char *str)
 	p1 = setup_buffer;
 	*p1 = '\0';
 	if (str)
-		strncpy(p1, str, SETUP_BUFFER_SIZE - strlen(setup_buffer));
-	setup_buffer[SETUP_BUFFER_SIZE - 1] = '\0';
-	p1 = setup_buffer;
+		strscpy(p1, str, SETUP_BUFFER_SIZE);
 	i = 0;
 	while (*p1 && (i < MAX_SETUP_ARGS)) {
 		p2 = strchr(p1, ',');

-- 
2.44.0.rc0.258.g7320e95886-goog

Re: [PATCH 7/7] scsi: wd33c93: replace deprecated strncpy with strscpy

[Finn Thain]

On Fri, 23 Feb 2024, Justin Stitt wrote:

> @p1 is assigned to @setup_buffer and then we manually assign a NUL-byte
> at the first index. This renders the following strlen() call useless.
> Moreover, we don't need to reassign p1 to setup_buffer for any reason --
> neither do we need to manually set a NUL-byte at the end. strscpy()
> resolves all this code making it easier to read.
> 
> Even considering the path where @str is falsey, the manual NUL-byte
> assignment is useless 

And yet your patch would only remove one of those assignments...

> as setup_buffer is declared with static storage
> duration in the top-level scope which should NUL-initialize the whole
> buffer.
> 

So, in order to review this patch, to try to avoid regressions, I would 
have to check your assumption that setup_buffer cannot change after being 
statically initialized. (The author of this code apparently was not 
willing to make that assumption.) It seems that patch review would require 
exhaustively searching for functions using the buffer, and examining the 
call graphs involving those functions. Is it really worth the effort?

> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
>  drivers/scsi/wd33c93.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/wd33c93.c b/drivers/scsi/wd33c93.c
> index e4fafc77bd20..a44b60c9004a 100644
> --- a/drivers/scsi/wd33c93.c
> +++ b/drivers/scsi/wd33c93.c
> @@ -1721,9 +1721,7 @@ wd33c93_setup(char *str)
>  	p1 = setup_buffer;
>  	*p1 = '\0';
>  	if (str)
> -		strncpy(p1, str, SETUP_BUFFER_SIZE - strlen(setup_buffer));
> -	setup_buffer[SETUP_BUFFER_SIZE - 1] = '\0';
> -	p1 = setup_buffer;
> +		strscpy(p1, str, SETUP_BUFFER_SIZE);
>  	i = 0;
>  	while (*p1 && (i < MAX_SETUP_ARGS)) {
>  		p2 = strchr(p1, ',');
> 
> 

Re: [PATCH 1/7] scsi: mpi3mr: replace deprecated strncpy with strscpy

[Finn Thain]

On Fri, 23 Feb 2024, Justin Stitt wrote:

> Really, there's no bug with the current code. 

If (hypothetically) you needed to reduce stack size, just copy the char 
pointer instead of copying the chars onto the stack.

If (hypothetically) strncpy() was banned altogether (rather than merely 
deprecated) I would do the same -- but I'm not the maintainer.

Re: [PATCH 7/7] scsi: wd33c93: replace deprecated strncpy with strscpy

[Kees Cook]

On Sat, Feb 24, 2024 at 10:44:12AM +1100, Finn Thain wrote:
> 
> On Fri, 23 Feb 2024, Justin Stitt wrote:
> 
> > @p1 is assigned to @setup_buffer and then we manually assign a NUL-byte
> > at the first index. This renders the following strlen() call useless.
> > Moreover, we don't need to reassign p1 to setup_buffer for any reason --
> > neither do we need to manually set a NUL-byte at the end. strscpy()
> > resolves all this code making it easier to read.
> > 
> > Even considering the path where @str is falsey, the manual NUL-byte
> > assignment is useless 
> 
> And yet your patch would only remove one of those assignments...

The first is needed in case it is called again.

> 
> > as setup_buffer is declared with static storage
> > duration in the top-level scope which should NUL-initialize the whole
> > buffer.
> > 
> 
> So, in order to review this patch, to try to avoid regressions, I would 
> have to check your assumption that setup_buffer cannot change after being 
> statically initialized. (The author of this code apparently was not 
> willing to make that assumption.) It seems that patch review would require 
> exhaustively searching for functions using the buffer, and examining the 
> call graphs involving those functions. Is it really worth the effort?

It seems to be run for each device? Regardless, I think leaving the
initial "*p1 = '\0';" solves this. (Though I fear for parallel
initializations, but that was already buggy: this code is from pre-git
history...)

> 
> > Signed-off-by: Justin Stitt <justinstitt@google.com>
> > ---
> >  drivers/scsi/wd33c93.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/scsi/wd33c93.c b/drivers/scsi/wd33c93.c
> > index e4fafc77bd20..a44b60c9004a 100644
> > --- a/drivers/scsi/wd33c93.c
> > +++ b/drivers/scsi/wd33c93.c
> > @@ -1721,9 +1721,7 @@ wd33c93_setup(char *str)
> >  	p1 = setup_buffer;
> >  	*p1 = '\0';
> >  	if (str)
> > -		strncpy(p1, str, SETUP_BUFFER_SIZE - strlen(setup_buffer));
> > -	setup_buffer[SETUP_BUFFER_SIZE - 1] = '\0';
> > -	p1 = setup_buffer;
> > +		strscpy(p1, str, SETUP_BUFFER_SIZE);
> >  	i = 0;
> >  	while (*p1 && (i < MAX_SETUP_ARGS)) {
> >  		p2 = strchr(p1, ',');
> > 
> > 

I think this conversion looks right.

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

-- 
Kees Cook

Re: [PATCH 1/7] scsi: mpi3mr: replace deprecated strncpy with strscpy

[Kees Cook]

On Sat, Feb 24, 2024 at 10:58:07AM +1100, Finn Thain wrote:
> 
> On Fri, 23 Feb 2024, Justin Stitt wrote:
> 
> > Really, there's no bug with the current code. 
> 
> If (hypothetically) you needed to reduce stack size, just copy the char 
> pointer instead of copying the chars onto the stack.
> 
> If (hypothetically) strncpy() was banned altogether (rather than merely 
> deprecated) I would do the same -- but I'm not the maintainer.

I'd agree. This can just be using:

const char *personality;
...
	personality = "RAID";

etc

-- 
Kees Cook

Re: [PATCH 2/7] scsi: mpt3sas: replace deprecated strncpy with strscpy

[Kees Cook]

On Fri, Feb 23, 2024 at 10:23:07PM +0000, Justin Stitt wrote:
> The replacement in mpt3sas_base.c is a trivial one because desc is
> already zero-initialized meaning there is no functional change here.
> 
> For mpt3sas_transport.c, we know edev is zero-initialized as well while
> manufacture_reply comes from dma_alloc_coherent(). No functional change
> here either.
> 
> For all cases, use the more idiomatic strscpy() usage of:
> strscpy(dest, src, sizeof(dest))
> 
> Signed-off-by: Justin Stitt <justinstitt@google.com>

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

-- 
Kees Cook

Re: [PATCH 3/7] scsi: qedf: replace deprecated strncpy with strscpy

[Kees Cook]

On Fri, Feb 23, 2024 at 10:23:08PM +0000, Justin Stitt wrote:
> We expect slowpath_params.name to be NUL-terminated based on its future
> usage with other string APIs:
> 
> |	static int qed_slowpath_start(struct qed_dev *cdev,
> |				      struct qed_slowpath_params *params)
> ...
> |	strscpy(drv_version.name, params->name,
> |		MCP_DRV_VER_STR_SIZE - 4);
> 
> Moreover, NUL-padding is not necessary as the only use for this slowpath
> name parameter is to copy into the drv_version.name field.
> 
> Also, let's prefer using strscpy(src, dest, sizeof(src)) in two
> instances (one of which is outside of the scsi system but it is trivial
> and related to this patch).
> 
> We can see the drv_version.name size here:
> |	struct qed_mcp_drv_version {
> |		u32	version;
> |		u8	name[MCP_DRV_VER_STR_SIZE - 4];
> |	};

What a weird length. :P

> 
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
>  drivers/net/ethernet/qlogic/qed/qed_main.c | 2 +-
>  drivers/scsi/qedf/qedf_main.c              | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c b/drivers/net/ethernet/qlogic/qed/qed_main.c
> index c278f8893042..d39e198fe8db 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_main.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_main.c
> @@ -1351,7 +1351,7 @@ static int qed_slowpath_start(struct qed_dev *cdev,
>  				      (params->drv_rev << 8) |
>  				      (params->drv_eng);
>  		strscpy(drv_version.name, params->name,
> -			MCP_DRV_VER_STR_SIZE - 4);
> +			sizeof(drv_version.name));
>  		rc = qed_mcp_send_drv_version(hwfn, hwfn->p_main_ptt,
>  					      &drv_version);
>  		if (rc) {
> diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c
> index a58353b7b4e8..fd12439cbaab 100644
> --- a/drivers/scsi/qedf/qedf_main.c
> +++ b/drivers/scsi/qedf/qedf_main.c
> @@ -3468,7 +3468,7 @@ static int __qedf_probe(struct pci_dev *pdev, int mode)
>  	slowpath_params.drv_minor = QEDF_DRIVER_MINOR_VER;
>  	slowpath_params.drv_rev = QEDF_DRIVER_REV_VER;
>  	slowpath_params.drv_eng = QEDF_DRIVER_ENG_VER;
> -	strncpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
> +	strscpy(slowpath_params.name, "qedf", sizeof(slowpath_params.name));
>  	rc = qed_ops->common->slowpath_start(qedf->cdev, &slowpath_params);
>  	if (rc) {
>  		QEDF_ERR(&(qedf->dbg_ctx), "Cannot start slowpath.\n");

Yeah, looks good.

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

-- 
Kees Cook

Re: [PATCH 4/7] scsi: qla4xxx: replace deprecated strncpy with strscpy

[Kees Cook]

On Fri, Feb 23, 2024 at 10:23:09PM +0000, Justin Stitt wrote:
> Replace 3 instances of strncpy in ql4_mbx.c
> 
> No bugs exist in the current implementation as some care was taken to
> ensure the write length was decreased by one to leave some space for a
> NUL-byte. However, instead of using strncpy(dest, src, LEN-1) we can opt
> for strscpy(dest, src, sizeof(dest)) which will result in
> NUL-termination as well. It should be noted that the entire chap_table
> is zero-allocated so the NUL-padding provided by strncpy is not needed.
> 
> While here, I noticed that MIN_CHAP_SECRET_LEN was not used anywhere.
> Since strscpy gives us the number of bytes copied into the destination
> buffer (or an -E2BIG) we can check both for an error during copying and
> also for a non-length compliant secret. Add a new jump label so we can
> properly clean up our chap_table should we have to abort due to bad
> secret.
> 
> The third instance in this file involves some more peculiar handling of
> strings:
> |	uint32_t mbox_cmd[MBOX_REG_COUNT];
> |	...
> |	memset(&mbox_cmd, 0, sizeof(mbox_cmd));
> |	...
> |	mbox_cmd[0] = MBOX_CMD_SET_PARAM;
> |	if (param == SET_DRVR_VERSION) {
> |		mbox_cmd[1] = SET_DRVR_VERSION;
> |		strncpy((char *)&mbox_cmd[2], QLA4XXX_DRIVER_VERSION,
> |			MAX_DRVR_VER_LEN - 1);
> 
> mbox_cmd has a size of 8:
> |	#define MBOX_REG_COUNT 8
> ... and its type width is 4 bytes. Hence, we have 32 bytes to work with
> here. The first 4 bytes are used as a flag for the MBOX_CMD_SET_PARAM.
> The next 4 bytes are used for SET_DRVR_VERSION. We now have 32-8=24
> bytes remaining -- which thankfully is what MAX_DRVR_VER_LEN is equal to
> |	#define MAX_DRVR_VER_LEN                    24
> 
> ... and the thing we're copying into this pseudo-string buffer is
> |	#define QLA4XXX_DRIVER_VERSION        "5.04.00-k6"
> 
> ... which is great because its less than 24 bytes (therefore we aren't
> truncating the source).
> 
> All to say, there's no bug in the existing implementation (yay!) but we
> can clean the code up a bit by using strscpy().
> 
> In ql4_os.c, there aren't any strncpy() uses to replace but there are
> some existing strscpy() calls that could be made more idiomatic. Where
> possible, use strscpy(dest, src, sizeof(dest)). Note that
> chap_rec->password has a size of ISCSI_CHAP_AUTH_SECRET_MAX_LEN
> |	#define ISCSI_CHAP_AUTH_SECRET_MAX_LEN	256
> ... while the current strscpy usage uses QL4_CHAP_MAX_SECRET_LEN
> |	#define QL4_CHAP_MAX_SECRET_LEN 100
> ... however since chap_table->secret was set and bounded properly in its
> string assignment its probably safe here to switch over to sizeof().
> 
> |	struct iscsi_chap_rec {
> 	...
> |		char username[ISCSI_CHAP_AUTH_NAME_MAX_LEN];
> |		uint8_t password[ISCSI_CHAP_AUTH_SECRET_MAX_LEN];
> 	...
> |	};
> 
> |	strscpy(chap_rec->password, chap_table->secret,
> |		QL4_CHAP_MAX_SECRET_LEN);
> 
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
>  drivers/scsi/qla4xxx/ql4_mbx.c | 17 ++++++++++++-----
>  drivers/scsi/qla4xxx/ql4_os.c  | 14 +++++++-------
>  2 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/scsi/qla4xxx/ql4_mbx.c b/drivers/scsi/qla4xxx/ql4_mbx.c
> index 249f1d7021d4..75125d2021f5 100644
> --- a/drivers/scsi/qla4xxx/ql4_mbx.c
> +++ b/drivers/scsi/qla4xxx/ql4_mbx.c
> @@ -1641,6 +1641,7 @@ int qla4xxx_set_chap(struct scsi_qla_host *ha, char *username, char *password,
>  	struct ql4_chap_table *chap_table;
>  	uint32_t chap_size = 0;
>  	dma_addr_t chap_dma;
> +	ssize_t secret_len;
>  
>  	chap_table = dma_pool_zalloc(ha->chap_dma_pool, GFP_KERNEL, &chap_dma);
>  	if (chap_table == NULL) {
> @@ -1652,9 +1653,13 @@ int qla4xxx_set_chap(struct scsi_qla_host *ha, char *username, char *password,
>  		chap_table->flags |= BIT_6; /* peer */
>  	else
>  		chap_table->flags |= BIT_7; /* local */
> -	chap_table->secret_len = strlen(password);
> -	strncpy(chap_table->secret, password, MAX_CHAP_SECRET_LEN - 1);
> -	strncpy(chap_table->name, username, MAX_CHAP_NAME_LEN - 1);
> +
> +	secret_len = strscpy(chap_table->secret, password,
> +			     sizeof(chap_table->secret));
> +	if (secret_len < MIN_CHAP_SECRET_LEN)
> +		goto cleanup_chap_table;
> +	chap_table->secret_len = (uint8_t)secret_len;
> +	strscpy(chap_table->name, username, sizeof(chap_table->name));

I'm genuinely not sure what to do here, but I suspect your approach is
safest.

I can't see where chap_table->secret is getting set, but if it was
longer than 100 bytes, there was a bug here, as strncpy() would truncate
but chap_table->secret_len would continue to have the original length.
However, it looks like the buf_len passed to iscsi_set_param() is
ignored. O_o

>  	chap_table->cookie = cpu_to_le16(CHAP_VALID_COOKIE);
>  
>  	if (is_qla40XX(ha)) {
> @@ -1679,6 +1684,8 @@ int qla4xxx_set_chap(struct scsi_qla_host *ha, char *username, char *password,
>  		memcpy((struct ql4_chap_table *)ha->chap_list + idx,
>  		       chap_table, sizeof(struct ql4_chap_table));
>  	}
> +
> +cleanup_chap_table:
>  	dma_pool_free(ha->chap_dma_pool, chap_table, chap_dma);
>  	if (rval != QLA_SUCCESS)
>  		ret =  -EINVAL;
> @@ -2281,8 +2288,8 @@ int qla4_8xxx_set_param(struct scsi_qla_host *ha, int param)
>  	mbox_cmd[0] = MBOX_CMD_SET_PARAM;
>  	if (param == SET_DRVR_VERSION) {
>  		mbox_cmd[1] = SET_DRVR_VERSION;
> -		strncpy((char *)&mbox_cmd[2], QLA4XXX_DRIVER_VERSION,
> -			MAX_DRVR_VER_LEN - 1);
> +		strscpy((char *)&mbox_cmd[2], QLA4XXX_DRIVER_VERSION,
> +			MAX_DRVR_VER_LEN);

As you mentioned in the commit log, this is a weird destination, but
is a legit calculation.

>  	} else {
>  		ql4_printk(KERN_ERR, ha, "%s: invalid parameter 0x%x\n",
>  			   __func__, param);
> diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
> index 675332e49a7b..17cccd14765f 100644
> --- a/drivers/scsi/qla4xxx/ql4_os.c
> +++ b/drivers/scsi/qla4xxx/ql4_os.c
> @@ -799,10 +799,10 @@ static int qla4xxx_get_chap_list(struct Scsi_Host *shost, uint16_t chap_tbl_idx,
>  
>  		chap_rec->chap_tbl_idx = i;
>  		strscpy(chap_rec->username, chap_table->name,
> -			ISCSI_CHAP_AUTH_NAME_MAX_LEN);
> -		strscpy(chap_rec->password, chap_table->secret,
> -			QL4_CHAP_MAX_SECRET_LEN);
> -		chap_rec->password_length = chap_table->secret_len;
> +			sizeof(chap_rec->username));
> +		chap_rec->password_length = strscpy(chap_rec->password,
> +						    chap_table->secret,
> +						    sizeof(chap_rec->password));
>  
>  		if (chap_table->flags & BIT_7) /* local */
>  			chap_rec->chap_type = CHAP_TYPE_OUT;
> @@ -6291,8 +6291,8 @@ static void qla4xxx_get_param_ddb(struct ddb_entry *ddb_entry,
>  
>  	tddb->tpgt = sess->tpgt;
>  	tddb->port = conn->persistent_port;
> -	strscpy(tddb->iscsi_name, sess->targetname, ISCSI_NAME_SIZE);
> -	strscpy(tddb->ip_addr, conn->persistent_address, DDB_IPADDR_LEN);
> +	strscpy(tddb->iscsi_name, sess->targetname, sizeof(tddb->iscsi_name));
> +	strscpy(tddb->ip_addr, conn->persistent_address, sizeof(tddb->ip_addr));
>  }
>  
>  static void qla4xxx_convert_param_ddb(struct dev_db_entry *fw_ddb_entry,
> @@ -7792,7 +7792,7 @@ static int qla4xxx_sysfs_ddb_logout(struct iscsi_bus_flash_session *fnode_sess,
>  	}
>  
>  	strscpy(flash_tddb->iscsi_name, fnode_sess->targetname,
> -		ISCSI_NAME_SIZE);
> +		sizeof(flash_tddb->iscsi_name));
>  
>  	if (!strncmp(fnode_sess->portal_type, PORTAL_TYPE_IPV6, 4))
>  		sprintf(flash_tddb->ip_addr, "%pI6", fnode_conn->ipaddress);
> 
> -- 
> 2.44.0.rc0.258.g7320e95886-goog
> 

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

-- 
Kees Cook

Re: [PATCH 5/7] scsi: devinfo: replace strncpy and manual pad

[Kees Cook]

On Fri, Feb 23, 2024 at 10:23:10PM +0000, Justin Stitt wrote:
> Depending on the state of @compatible, we are going to do different
> things with our @to buffer.
> 
> When @compatible is true we want a NUL-term'd and NUL-padded destination
> buffer. Conversely, if @compatible is false we just want a space-padded
> destination buffer (no NUL-term required).
> 
> As per:
> /**
>  * scsi_dev_info_list_add_keyed - add one dev_info list entry.
>  * @compatible: if true, null terminate short strings.  Otherwise space pad.
> ...
> 
> Note that we can't easily use `strtomem_pad` here as the size of the @to
> buffer is unknown to the compiler due to indirection layers.
> 
> Now, the intent of the code is more clear (I probably didn't even need
> to add a comment -- that's how clear it is).
> 
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
>  drivers/scsi/scsi_devinfo.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
> index 3fcaf10a9dfe..2d3dbce25629 100644
> --- a/drivers/scsi/scsi_devinfo.c
> +++ b/drivers/scsi/scsi_devinfo.c
> @@ -293,14 +293,16 @@ static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,
>  	size_t from_length;
>  
>  	from_length = strlen(from);
> -	/* This zero-pads the destination */
> -	strncpy(to, from, to_length);

A rare case of the padding intent being expressed! :)

> -	if (from_length < to_length && !compatible) {
> -		/*
> -		 * space pad the string if it is short.
> -		 */
> -		memset(&to[from_length], ' ', to_length - from_length);
> -	}
> +
> +	/*
> +	 * null pad and null terminate if compatible
> +	 * otherwise space pad
> +	 */
> +	if (compatible)
> +		strscpy_pad(to, from, to_length);
> +	else
> +		memcpy_and_pad(to, to_length, from, from_length, ' ');

Yeah, this is much nicer to read.

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

(Some day I want to rename "memcpy_and_pad" ... the "and" seems
verbose...)

-- 
Kees Cook

Re: [PATCH 6/7] scsi: smartpqi: replace deprecated strncpy with strscpy

[Kees Cook]

On Fri, Feb 23, 2024 at 10:23:11PM +0000, Justin Stitt wrote:
> buffer->driver_version is sized 32:
> |	struct bmic_host_wellness_driver_version {
> |	...
> |		char	driver_version[32];
> ... the source string "Linux " + DRIVER_VERISON is sized at 16. There's
> really no bug in the existing code since the buffers are sized
> appropriately with great care taken to manually NUL-terminate the
> destination buffer. Nonetheless, let's make the swap over to strscpy()
> for robustness' (and readability's) sake.
> 
> Signed-off-by: Justin Stitt <justinstitt@google.com>

Yup, good cleanup.

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

-- 
Kees Cook