[0/7] scsi: replace deprecated strncpy

  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>
  

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>
  

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>
  

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>
  

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

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>