| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <202402281606.199AC93A4B@keescook>
Date: Wed, 28 Feb 2024 16:15:35 -0800
From: Kees Cook <keescook@...omium.org>
To: Justin Stitt <justinstitt@...gle.com>
Cc: Sathya Prakash Veerichetty <sathya.prakash@...adcom.com>,
Kashyap Desai <kashyap.desai@...adcom.com>,
Sumit Saxena <sumit.saxena@...adcom.com>,
Sreekanth Reddy <sreekanth.reddy@...adcom.com>,
"James E.J. Bottomley" <jejb@...ux.ibm.com>,
"Martin K. Petersen" <martin.petersen@...cle.com>,
Suganath Prabu Subramani <suganath-prabu.subramani@...adcom.com>,
Ariel Elior <aelior@...vell.com>,
Manish Chopra <manishc@...vell.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Saurav Kashyap <skashyap@...vell.com>,
Javed Hasan <jhasan@...vell.com>,
GR-QLogic-Storage-Upstream@...vell.com,
Nilesh Javali <njavali@...vell.com>,
Manish Rangankar <mrangankar@...vell.com>,
Don Brace <don.brace@...rochip.com>,
mpi3mr-linuxdrv.pdl@...adcom.com, linux-scsi@...r.kernel.org,
linux-hardening@...r.kernel.org, linux-kernel@...r.kernel.org,
MPT-FusionLinux.pdl@...adcom.com, netdev@...r.kernel.org,
storagedev@...rochip.com
Subject: Re: [PATCH v2 4/7] scsi: qla4xxx: replace deprecated strncpy with
strscpy
On Wed, Feb 28, 2024 at 10:59:04PM +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@...gle.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));
This hunk took me a bit to convince myself it's safe, but yes, I agree.
It all boils down to sizeof(chap_table->secret) being less than
sizeof(chap_rec->password), so there's no behavioral change here.
Reviewed-by: Kees Cook <keescook@...omium.org>
--
Kees Cook
Powered by blists - more mailing lists