From 7936a19e944b934d21d79f1b90d478d1f7081b63 Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Mon, 23 Oct 2023 19:50:57 +0000 Subject: [PATCH 1/5] scsi: 3w-sas: Replace deprecated strncpy() with strscpy() strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. This pattern of strncpy(dest, src, strlen(src)) is extremely bug-prone. This pattern basically never results in NUL-terminated destination strings unless `dest` was zero-initialized. The current implementation may be accidentally correct as tw_dev is zero-allocated via: host = scsi_host_alloc(&driver_template, sizeof(TW_Device_Extension)); ... tw_dev = shost_priv(host); ... wherein scsi_host_alloc() zero-allocates host: shost = kzalloc(sizeof(struct Scsi_Host) + privsize, GFP_KERNEL); Also, further suggesting this change is worthwhile is another strscpy() usage in 3w-9xxx.c: strscpy(tw_dev->tw_compat_info.driver_version, TW_DRIVER_VERSION, sizeof(tw_dev->tw_compat_info.driver_version)); Considering the above, a suitable replacement is strscpy() [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding. Let's not be accidentally correct, let's be definitely correct. 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 Link: https://lore.kernel.org/r/20231023-strncpy-drivers-scsi-3w-sas-c-v1-1-4c40a1e99dfc@google.com Reviewed-by: Kees Cook Signed-off-by: Martin K. Petersen --- drivers/scsi/3w-sas.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/scsi/3w-sas.c b/drivers/scsi/3w-sas.c index 55989eaa2d9f..9bdb75dfdcd7 100644 --- a/drivers/scsi/3w-sas.c +++ b/drivers/scsi/3w-sas.c @@ -1326,7 +1326,8 @@ static int twl_reset_sequence(TW_Device_Extension *tw_dev, int soft_reset) } /* Load rest of compatibility struct */ - strncpy(tw_dev->tw_compat_info.driver_version, TW_DRIVER_VERSION, strlen(TW_DRIVER_VERSION)); + strscpy(tw_dev->tw_compat_info.driver_version, TW_DRIVER_VERSION, + sizeof(tw_dev->tw_compat_info.driver_version)); tw_dev->tw_compat_info.driver_srl_high = TW_CURRENT_DRIVER_SRL; tw_dev->tw_compat_info.driver_branch_high = TW_CURRENT_DRIVER_BRANCH; tw_dev->tw_compat_info.driver_build_high = TW_CURRENT_DRIVER_BUILD; From b04a2eff9e9c6f8890d25dbb073fbf5c00892a9a Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Mon, 23 Oct 2023 20:12:22 +0000 Subject: [PATCH 2/5] scsi: bnx2fc: Replace deprecated strncpy() with strscpy() strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. We expect hba->chip_num to be NUL-terminated based on its usage with format strings: snprintf(fc_host_symbolic_name(lport->host), 256, "%s (QLogic %s) v%s over %s", BNX2FC_NAME, hba->chip_num, BNX2FC_VERSION, interface->netdev->name); Moreover, NUL-padding is not required as hba is zero-allocated from its callsite: hba = kzalloc(sizeof(*hba), GFP_KERNEL); Considering the above, a suitable replacement is strscpy() [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding. Regarding stats_addr->version, I've opted to also use strscpy() instead of strscpy_pad() as I typically see these XYZ_get_strings() pass zero-allocated data. I couldn't track all of where bnx2fc_ulp_get_stats() is used and if required, we could opt for strscpy_pad(). 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 Link: https://lore.kernel.org/r/20231023-strncpy-drivers-scsi-bnx2fc-bnx2fc_fcoe-c-v1-1-a3736943cde2@google.com Reviewed-by: Kees Cook Signed-off-by: Martin K. Petersen --- drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c index 05ddbb9bb7d8..3ebfb09329ad 100644 --- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c +++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c @@ -1737,32 +1737,32 @@ static int bnx2fc_bind_pcidev(struct bnx2fc_hba *hba) switch (pdev->device) { case PCI_DEVICE_ID_NX2_57710: - strncpy(hba->chip_num, "BCM57710", BCM_CHIP_LEN); + strscpy(hba->chip_num, "BCM57710", sizeof(hba->chip_num)); break; case PCI_DEVICE_ID_NX2_57711: - strncpy(hba->chip_num, "BCM57711", BCM_CHIP_LEN); + strscpy(hba->chip_num, "BCM57711", sizeof(hba->chip_num)); break; case PCI_DEVICE_ID_NX2_57712: case PCI_DEVICE_ID_NX2_57712_MF: case PCI_DEVICE_ID_NX2_57712_VF: - strncpy(hba->chip_num, "BCM57712", BCM_CHIP_LEN); + strscpy(hba->chip_num, "BCM57712", sizeof(hba->chip_num)); break; case PCI_DEVICE_ID_NX2_57800: case PCI_DEVICE_ID_NX2_57800_MF: case PCI_DEVICE_ID_NX2_57800_VF: - strncpy(hba->chip_num, "BCM57800", BCM_CHIP_LEN); + strscpy(hba->chip_num, "BCM57800", sizeof(hba->chip_num)); break; case PCI_DEVICE_ID_NX2_57810: case PCI_DEVICE_ID_NX2_57810_MF: case PCI_DEVICE_ID_NX2_57810_VF: - strncpy(hba->chip_num, "BCM57810", BCM_CHIP_LEN); + strscpy(hba->chip_num, "BCM57810", sizeof(hba->chip_num)); break; case PCI_DEVICE_ID_NX2_57840: case PCI_DEVICE_ID_NX2_57840_MF: case PCI_DEVICE_ID_NX2_57840_VF: case PCI_DEVICE_ID_NX2_57840_2_20: case PCI_DEVICE_ID_NX2_57840_4_10: - strncpy(hba->chip_num, "BCM57840", BCM_CHIP_LEN); + strscpy(hba->chip_num, "BCM57840", sizeof(hba->chip_num)); break; default: pr_err(PFX "Unknown device id 0x%x\n", pdev->device); @@ -1800,7 +1800,7 @@ static int bnx2fc_ulp_get_stats(void *handle) if (!stats_addr) return -EINVAL; - strncpy(stats_addr->version, BNX2FC_VERSION, + strscpy(stats_addr->version, BNX2FC_VERSION, sizeof(stats_addr->version)); stats_addr->txq_size = BNX2FC_SQ_WQES_MAX; stats_addr->rxq_size = BNX2FC_CQ_WQES_MAX; From dc7a7f10e673499856dd275691768df6d07a2815 Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Mon, 23 Oct 2023 20:20:14 +0000 Subject: [PATCH 3/5] scsi: ch: Replace deprecated strncpy() with strscpy() strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. These labels get copied out to the user so lets make sure they are NUL-terminated and NUL-padded. vparams is already memset to 0 so we don't need to do any NUL-padding (like what strncpy() is doing). Considering the above, a suitable replacement is strscpy() [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding. Let's also opt to use the more idiomatic strscpy() usage of: (dest, src, sizeof(dest)) as this more closely ties the destination buffer to the length. 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 Link: https://lore.kernel.org/r/20231023-strncpy-drivers-scsi-ch-c-v1-1-dc67ba8075a3@google.com Reviewed-by: Kees Cook Signed-off-by: Martin K. Petersen --- drivers/scsi/ch.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c index cb0a399be1cc..2b864061e073 100644 --- a/drivers/scsi/ch.c +++ b/drivers/scsi/ch.c @@ -659,19 +659,23 @@ static long ch_ioctl(struct file *file, memset(&vparams,0,sizeof(vparams)); if (ch->counts[CHET_V1]) { vparams.cvp_n1 = ch->counts[CHET_V1]; - strncpy(vparams.cvp_label1,vendor_labels[0],16); + strscpy(vparams.cvp_label1, vendor_labels[0], + sizeof(vparams.cvp_label1)); } if (ch->counts[CHET_V2]) { vparams.cvp_n2 = ch->counts[CHET_V2]; - strncpy(vparams.cvp_label2,vendor_labels[1],16); + strscpy(vparams.cvp_label2, vendor_labels[1], + sizeof(vparams.cvp_label2)); } if (ch->counts[CHET_V3]) { vparams.cvp_n3 = ch->counts[CHET_V3]; - strncpy(vparams.cvp_label3,vendor_labels[2],16); + strscpy(vparams.cvp_label3, vendor_labels[2], + sizeof(vparams.cvp_label3)); } if (ch->counts[CHET_V4]) { vparams.cvp_n4 = ch->counts[CHET_V4]; - strncpy(vparams.cvp_label4,vendor_labels[3],16); + strscpy(vparams.cvp_label4, vendor_labels[3], + sizeof(vparams.cvp_label4)); } if (copy_to_user(argp, &vparams, sizeof(vparams))) return -EFAULT; From 4592411784ccf83f873f98ba94aeae2482783c9a Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Mon, 23 Oct 2023 20:26:13 +0000 Subject: [PATCH 4/5] scsi: csiostor: Replace deprecated strncpy() with strscpy() strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. 'hw' is kzalloc'd just before this string assignment: | hw = kzalloc(sizeof(struct csio_hw), GFP_KERNEL); ... which means any NUL-padding is redundant. Since CSIO_DRV_VERSION is a small string literal (smaller than sizeof(dest)): ... there is functionally no change in this swap from strncpy() to strscpy(). Nonetheless, let's make the change for robustness' sake -- as it will ensure that drv_version is _always_ NUL-terminated. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt Link: https://lore.kernel.org/r/20231023-strncpy-drivers-scsi-csiostor-csio_init-c-v1-1-5ea445b56864@google.com Reviewed-by: Kees Cook Signed-off-by: Martin K. Petersen --- drivers/scsi/csiostor/csio_init.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/scsi/csiostor/csio_init.c b/drivers/scsi/csiostor/csio_init.c index 0c32faefad7c..d649b7a2a879 100644 --- a/drivers/scsi/csiostor/csio_init.c +++ b/drivers/scsi/csiostor/csio_init.c @@ -521,7 +521,8 @@ static struct csio_hw *csio_hw_alloc(struct pci_dev *pdev) goto err; hw->pdev = pdev; - strncpy(hw->drv_version, CSIO_DRV_VERSION, 32); + strscpy(hw->drv_version, CSIO_DRV_VERSION, + sizeof(hw->drv_version)); /* memory pool/DMA pool allocation */ if (csio_resource_alloc(hw)) From 1057f44137c5484e402cc69d0ad9954e6cd7e029 Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Thu, 26 Oct 2023 01:53:13 +0000 Subject: [PATCH 5/5] scsi: elx: libefc: Replace deprecated strncpy() with strscpy_pad()/memcpy() strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. To keep node->current_state_name and node->prev_state_name NUL-padded and NUL-terminated let's use strscpy_pad() as this implicitly provides both. For the swap between the two, a simple memcpy() will suffice. 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 Link: https://lore.kernel.org/r/20231026-strncpy-drivers-scsi-elx-libefc-efc_node-h-v2-1-5c083d0c13f4@google.com Reviewed-by: Kees Cook Signed-off-by: Martin K. Petersen --- drivers/scsi/elx/libefc/efc_node.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/scsi/elx/libefc/efc_node.h b/drivers/scsi/elx/libefc/efc_node.h index e9c600ac45d5..e57579988ba4 100644 --- a/drivers/scsi/elx/libefc/efc_node.h +++ b/drivers/scsi/elx/libefc/efc_node.h @@ -26,13 +26,13 @@ efc_node_evt_set(struct efc_sm_ctx *ctx, enum efc_sm_event evt, struct efc_node *node = ctx->app; if (evt == EFC_EVT_ENTER) { - strncpy(node->current_state_name, handler, - sizeof(node->current_state_name)); + strscpy_pad(node->current_state_name, handler, + sizeof(node->current_state_name)); } else if (evt == EFC_EVT_EXIT) { - strncpy(node->prev_state_name, node->current_state_name, - sizeof(node->prev_state_name)); - strncpy(node->current_state_name, "invalid", - sizeof(node->current_state_name)); + memcpy(node->prev_state_name, node->current_state_name, + sizeof(node->prev_state_name)); + strscpy_pad(node->current_state_name, "invalid", + sizeof(node->current_state_name)); } node->prev_evt = node->current_evt; node->current_evt = evt;