| Message ID | 20240223-strncpy-drivers-scsi-mpi3mr-mpi3mr_fw-c-v1-0-9cd3882f0700@google.com |
|---|---|
| Headers | show |
| Series | scsi: replace deprecated strncpy | expand |
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: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>
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>