From 30ad0627f169f56180e668e7223eaa43aa190a75 Mon Sep 17 00:00:00 2001 From: Azeem Shaikh Date: Wed, 10 May 2023 22:12:37 +0000 Subject: dlm: Replace all non-returning strlcpy with strscpy strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. In an effort to remove strlcpy() completely [2], replace strlcpy() here with strscpy(). No return values were used, so direct replacement is safe. [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [2] https://github.com/KSPP/linux/issues/89 Signed-off-by: Azeem Shaikh Reviewed-by: Kees Cook Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20230510221237.3509484-1-azeemshaikh38@gmail.com --- fs/dlm/config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'fs') diff --git a/fs/dlm/config.c b/fs/dlm/config.c index d31319d08581..2beceff024e3 100644 --- a/fs/dlm/config.c +++ b/fs/dlm/config.c @@ -116,9 +116,9 @@ static ssize_t cluster_cluster_name_store(struct config_item *item, { struct dlm_cluster *cl = config_item_to_cluster(item); - strlcpy(dlm_config.ci_cluster_name, buf, + strscpy(dlm_config.ci_cluster_name, buf, sizeof(dlm_config.ci_cluster_name)); - strlcpy(cl->cl_cluster_name, buf, sizeof(cl->cl_cluster_name)); + strscpy(cl->cl_cluster_name, buf, sizeof(cl->cl_cluster_name)); return len; } -- cgit v1.2.3 From 8ca25e00cf817b635f4e80d59b6d07686d74eff0 Mon Sep 17 00:00:00 2001 From: Azeem Shaikh Date: Fri, 12 May 2023 15:57:49 +0000 Subject: NFS: Prefer strscpy over strlcpy calls strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. Check for strscpy()'s return value of -E2BIG on truncate for safe replacement with strlcpy(). This is part of a tree-wide cleanup to remove the strlcpy() function entirely from the kernel [2]. [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [2] https://github.com/KSPP/linux/issues/89 Signed-off-by: Azeem Shaikh Reviewed-by: Kees Cook Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20230512155749.1356958-1-azeemshaikh38@gmail.com --- fs/nfs/nfsroot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs') diff --git a/fs/nfs/nfsroot.c b/fs/nfs/nfsroot.c index 620329b7e6ae..7600100ba26f 100644 --- a/fs/nfs/nfsroot.c +++ b/fs/nfs/nfsroot.c @@ -164,7 +164,7 @@ __setup("nfsroot=", nfs_root_setup); static int __init root_nfs_copy(char *dest, const char *src, const size_t destlen) { - if (strlcpy(dest, src, destlen) > destlen) + if (strscpy(dest, src, destlen) == -E2BIG) return -1; return 0; } -- cgit v1.2.3 From 883f8fe87686d1deef2614b1d3a23ca7e5193dff Mon Sep 17 00:00:00 2001 From: Azeem Shaikh Date: Wed, 10 May 2023 21:11:46 +0000 Subject: vboxsf: Replace all non-returning strlcpy with strscpy strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. In an effort to remove strlcpy() completely [2], replace strlcpy() here with strscpy(). No return values were used, so direct replacement is safe. [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [2] https://github.com/KSPP/linux/issues/89 Signed-off-by: Azeem Shaikh Reviewed-by: Hans de Goede Reviewed-by: Kees Cook Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20230510211146.3486600-1-azeemshaikh38@gmail.com --- fs/vboxsf/super.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs') diff --git a/fs/vboxsf/super.c b/fs/vboxsf/super.c index d2f6df69f611..1fb8f4df60cb 100644 --- a/fs/vboxsf/super.c +++ b/fs/vboxsf/super.c @@ -176,7 +176,7 @@ static int vboxsf_fill_super(struct super_block *sb, struct fs_context *fc) } folder_name->size = size; folder_name->length = size - 1; - strlcpy(folder_name->string.utf8, fc->source, size); + strscpy(folder_name->string.utf8, fc->source, size); err = vboxsf_map_folder(folder_name, &sbi->root); kfree(folder_name); if (err) { -- cgit v1.2.3 From d67790ddf0219aa0ad3e13b53ae0a7619b3425a2 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Mon, 22 May 2023 14:18:13 -0700 Subject: overflow: Add struct_size_t() helper While struct_size() is normally used in situations where the structure type already has a pointer instance, there are places where no variable is available. In the past, this has been worked around by using a typed NULL first argument, but this is a bit ugly. Add a helper to do this, and replace the handful of instances of the code pattern with it. Instances were found with this Coccinelle script: @struct_size_t@ identifier STRUCT, MEMBER; expression COUNT; @@ - struct_size((struct STRUCT *)\(0\|NULL\), + struct_size_t(struct STRUCT, MEMBER, COUNT) Suggested-by: Christoph Hellwig Cc: Jesse Brandeburg Cc: Tony Nguyen Cc: "David S. Miller" Cc: Eric Dumazet Cc: Paolo Abeni Cc: James Smart Cc: Keith Busch Cc: Jens Axboe Cc: Sagi Grimberg Cc: HighPoint Linux Team Cc: "James E.J. Bottomley" Cc: "Martin K. Petersen" Cc: Kashyap Desai Cc: Sumit Saxena Cc: Shivasharan S Cc: Don Brace Cc: "Darrick J. Wong" Cc: Dave Chinner Cc: Guo Xuenan Cc: Gwan-gyeong Mun Cc: Nick Desaulniers Cc: Daniel Latypov Cc: kernel test robot Cc: intel-wired-lan@lists.osuosl.org Cc: netdev@vger.kernel.org Cc: linux-nvme@lists.infradead.org Cc: linux-scsi@vger.kernel.org Cc: megaraidlinux.pdl@broadcom.com Cc: storagedev@microchip.com Cc: linux-xfs@vger.kernel.org Cc: linux-hardening@vger.kernel.org Signed-off-by: Kees Cook Acked-by: Martin K. Petersen Reviewed-by: Darrick J. Wong Reviewed-by: Gustavo A. R. Silva Reviewed-by: Christoph Hellwig Acked-by: Jakub Kicinski Reviewed-by: Alexander Lobakin Link: https://lore.kernel.org/r/20230522211810.never.421-kees@kernel.org --- drivers/net/ethernet/intel/ice/ice_ddp.h | 9 ++++----- drivers/nvme/host/fc.c | 8 ++++---- drivers/scsi/hptiop.c | 4 ++-- drivers/scsi/megaraid/megaraid_sas_base.c | 12 ++++++------ drivers/scsi/megaraid/megaraid_sas_fp.c | 6 +++--- drivers/scsi/smartpqi/smartpqi_init.c | 2 +- fs/xfs/libxfs/xfs_btree.h | 2 +- fs/xfs/scrub/btree.h | 2 +- include/linux/overflow.h | 18 +++++++++++++++++- lib/overflow_kunit.c | 2 +- 10 files changed, 40 insertions(+), 25 deletions(-) (limited to 'fs') diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.h b/drivers/net/ethernet/intel/ice/ice_ddp.h index 37eadb3d27a8..41acfe26df1c 100644 --- a/drivers/net/ethernet/intel/ice/ice_ddp.h +++ b/drivers/net/ethernet/intel/ice/ice_ddp.h @@ -185,7 +185,7 @@ struct ice_buf_hdr { #define ICE_MAX_ENTRIES_IN_BUF(hd_sz, ent_sz) \ ((ICE_PKG_BUF_SIZE - \ - struct_size((struct ice_buf_hdr *)0, section_entry, 1) - (hd_sz)) / \ + struct_size_t(struct ice_buf_hdr, section_entry, 1) - (hd_sz)) / \ (ent_sz)) /* ice package section IDs */ @@ -297,7 +297,7 @@ struct ice_label_section { }; #define ICE_MAX_LABELS_IN_BUF \ - ICE_MAX_ENTRIES_IN_BUF(struct_size((struct ice_label_section *)0, \ + ICE_MAX_ENTRIES_IN_BUF(struct_size_t(struct ice_label_section, \ label, 1) - \ sizeof(struct ice_label), \ sizeof(struct ice_label)) @@ -352,7 +352,7 @@ struct ice_boost_tcam_section { }; #define ICE_MAX_BST_TCAMS_IN_BUF \ - ICE_MAX_ENTRIES_IN_BUF(struct_size((struct ice_boost_tcam_section *)0, \ + ICE_MAX_ENTRIES_IN_BUF(struct_size_t(struct ice_boost_tcam_section, \ tcam, 1) - \ sizeof(struct ice_boost_tcam_entry), \ sizeof(struct ice_boost_tcam_entry)) @@ -372,8 +372,7 @@ struct ice_marker_ptype_tcam_section { }; #define ICE_MAX_MARKER_PTYPE_TCAMS_IN_BUF \ - ICE_MAX_ENTRIES_IN_BUF( \ - struct_size((struct ice_marker_ptype_tcam_section *)0, tcam, \ + ICE_MAX_ENTRIES_IN_BUF(struct_size_t(struct ice_marker_ptype_tcam_section, tcam, \ 1) - \ sizeof(struct ice_marker_ptype_tcam_entry), \ sizeof(struct ice_marker_ptype_tcam_entry)) diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c index 2ed75923507d..691f2df574ce 100644 --- a/drivers/nvme/host/fc.c +++ b/drivers/nvme/host/fc.c @@ -2917,8 +2917,8 @@ nvme_fc_create_io_queues(struct nvme_fc_ctrl *ctrl) ret = nvme_alloc_io_tag_set(&ctrl->ctrl, &ctrl->tag_set, &nvme_fc_mq_ops, 1, - struct_size((struct nvme_fcp_op_w_sgl *)NULL, priv, - ctrl->lport->ops->fcprqst_priv_sz)); + struct_size_t(struct nvme_fcp_op_w_sgl, priv, + ctrl->lport->ops->fcprqst_priv_sz)); if (ret) return ret; @@ -3536,8 +3536,8 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts, ret = nvme_alloc_admin_tag_set(&ctrl->ctrl, &ctrl->admin_tag_set, &nvme_fc_admin_mq_ops, - struct_size((struct nvme_fcp_op_w_sgl *)NULL, priv, - ctrl->lport->ops->fcprqst_priv_sz)); + struct_size_t(struct nvme_fcp_op_w_sgl, priv, + ctrl->lport->ops->fcprqst_priv_sz)); if (ret) goto fail_ctrl; diff --git a/drivers/scsi/hptiop.c b/drivers/scsi/hptiop.c index 06ccb51bf6a9..f5334ccbf2ca 100644 --- a/drivers/scsi/hptiop.c +++ b/drivers/scsi/hptiop.c @@ -1394,8 +1394,8 @@ static int hptiop_probe(struct pci_dev *pcidev, const struct pci_device_id *id) host->cmd_per_lun = le32_to_cpu(iop_config.max_requests); host->max_cmd_len = 16; - req_size = struct_size((struct hpt_iop_request_scsi_command *)0, - sg_list, hba->max_sg_descriptors); + req_size = struct_size_t(struct hpt_iop_request_scsi_command, + sg_list, hba->max_sg_descriptors); if ((req_size & 0x1f) != 0) req_size = (req_size + 0x1f) & ~0x1f; diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c index 317c944c68e3..050eed8e2684 100644 --- a/drivers/scsi/megaraid/megaraid_sas_base.c +++ b/drivers/scsi/megaraid/megaraid_sas_base.c @@ -5153,8 +5153,8 @@ static void megasas_update_ext_vd_details(struct megasas_instance *instance) fusion->max_map_sz = ventura_map_sz; } else { fusion->old_map_sz = - struct_size((struct MR_FW_RAID_MAP *)0, ldSpanMap, - instance->fw_supported_vd_count); + struct_size_t(struct MR_FW_RAID_MAP, ldSpanMap, + instance->fw_supported_vd_count); fusion->new_map_sz = sizeof(struct MR_FW_RAID_MAP_EXT); fusion->max_map_sz = @@ -5789,8 +5789,8 @@ megasas_setup_jbod_map(struct megasas_instance *instance) struct fusion_context *fusion = instance->ctrl_context; size_t pd_seq_map_sz; - pd_seq_map_sz = struct_size((struct MR_PD_CFG_SEQ_NUM_SYNC *)0, seq, - MAX_PHYSICAL_DEVICES); + pd_seq_map_sz = struct_size_t(struct MR_PD_CFG_SEQ_NUM_SYNC, seq, + MAX_PHYSICAL_DEVICES); instance->use_seqnum_jbod_fp = instance->support_seqnum_jbod_fp; @@ -8033,8 +8033,8 @@ skip_firing_dcmds: if (instance->adapter_type != MFI_SERIES) { megasas_release_fusion(instance); pd_seq_map_sz = - struct_size((struct MR_PD_CFG_SEQ_NUM_SYNC *)0, - seq, MAX_PHYSICAL_DEVICES); + struct_size_t(struct MR_PD_CFG_SEQ_NUM_SYNC, + seq, MAX_PHYSICAL_DEVICES); for (i = 0; i < 2 ; i++) { if (fusion->ld_map[i]) dma_free_coherent(&instance->pdev->dev, diff --git a/drivers/scsi/megaraid/megaraid_sas_fp.c b/drivers/scsi/megaraid/megaraid_sas_fp.c index 4463a538102a..b8b388a4e28f 100644 --- a/drivers/scsi/megaraid/megaraid_sas_fp.c +++ b/drivers/scsi/megaraid/megaraid_sas_fp.c @@ -326,9 +326,9 @@ u8 MR_ValidateMapInfo(struct megasas_instance *instance, u64 map_id) else if (instance->supportmax256vd) expected_size = sizeof(struct MR_FW_RAID_MAP_EXT); else - expected_size = struct_size((struct MR_FW_RAID_MAP *)0, - ldSpanMap, - le16_to_cpu(pDrvRaidMap->ldCount)); + expected_size = struct_size_t(struct MR_FW_RAID_MAP, + ldSpanMap, + le16_to_cpu(pDrvRaidMap->ldCount)); if (le32_to_cpu(pDrvRaidMap->totalSize) != expected_size) { dev_dbg(&instance->pdev->dev, "megasas: map info structure size 0x%x", diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c index 03de97cd72c2..f4e0aa262164 100644 --- a/drivers/scsi/smartpqi/smartpqi_init.c +++ b/drivers/scsi/smartpqi/smartpqi_init.c @@ -5015,7 +5015,7 @@ static int pqi_create_queues(struct pqi_ctrl_info *ctrl_info) } #define PQI_REPORT_EVENT_CONFIG_BUFFER_LENGTH \ - struct_size((struct pqi_event_config *)0, descriptors, PQI_MAX_EVENT_DESCRIPTORS) + struct_size_t(struct pqi_event_config, descriptors, PQI_MAX_EVENT_DESCRIPTORS) static int pqi_configure_events(struct pqi_ctrl_info *ctrl_info, bool enable_events) diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h index a2aa36b23e25..4d68a58be160 100644 --- a/fs/xfs/libxfs/xfs_btree.h +++ b/fs/xfs/libxfs/xfs_btree.h @@ -301,7 +301,7 @@ struct xfs_btree_cur static inline size_t xfs_btree_cur_sizeof(unsigned int nlevels) { - return struct_size((struct xfs_btree_cur *)NULL, bc_levels, nlevels); + return struct_size_t(struct xfs_btree_cur, bc_levels, nlevels); } /* cursor flags */ diff --git a/fs/xfs/scrub/btree.h b/fs/xfs/scrub/btree.h index 9d7b9ee8bef4..c32b5fad6174 100644 --- a/fs/xfs/scrub/btree.h +++ b/fs/xfs/scrub/btree.h @@ -60,7 +60,7 @@ struct xchk_btree { static inline size_t xchk_btree_sizeof(unsigned int nlevels) { - return struct_size((struct xchk_btree *)NULL, lastkey, nlevels - 1); + return struct_size_t(struct xchk_btree, lastkey, nlevels - 1); } int xchk_btree(struct xfs_scrub *sc, struct xfs_btree_cur *cur, diff --git a/include/linux/overflow.h b/include/linux/overflow.h index 0e33b5cbdb9f..f9b60313eaea 100644 --- a/include/linux/overflow.h +++ b/include/linux/overflow.h @@ -283,7 +283,7 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend) * @member: Name of the array member. * @count: Number of elements in the array. * - * Calculates size of memory needed for structure @p followed by an + * Calculates size of memory needed for structure of @p followed by an * array of @count number of @member elements. * * Return: number of bytes needed or SIZE_MAX on overflow. @@ -293,4 +293,20 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend) sizeof(*(p)) + flex_array_size(p, member, count), \ size_add(sizeof(*(p)), flex_array_size(p, member, count))) +/** + * struct_size_t() - Calculate size of structure with trailing flexible array + * @type: structure type name. + * @member: Name of the array member. + * @count: Number of elements in the array. + * + * Calculates size of memory needed for structure @type followed by an + * array of @count number of @member elements. Prefer using struct_size() + * when possible instead, to keep calculations associated with a specific + * instance variable of type @type. + * + * Return: number of bytes needed or SIZE_MAX on overflow. + */ +#define struct_size_t(type, member, count) \ + struct_size((type *)NULL, member, count) + #endif /* __LINUX_OVERFLOW_H */ diff --git a/lib/overflow_kunit.c b/lib/overflow_kunit.c index dcd3ba102db6..34db0b3aa502 100644 --- a/lib/overflow_kunit.c +++ b/lib/overflow_kunit.c @@ -649,7 +649,7 @@ struct __test_flex_array { static void overflow_size_helpers_test(struct kunit *test) { /* Make sure struct_size() can be used in a constant expression. */ - u8 ce_array[struct_size((struct __test_flex_array *)0, data, 55)]; + u8 ce_array[struct_size_t(struct __test_flex_array, data, 55)]; struct __test_flex_array *obj; int count = 0; int var; -- cgit v1.2.3 From 7391928025f28cd4cab43e59d2b4e36509b9337e Mon Sep 17 00:00:00 2001 From: Azeem Shaikh Date: Tue, 9 May 2023 01:41:36 +0000 Subject: befs: Replace all non-returning strlcpy with strscpy strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated. In an effort to remove strlcpy() completely, replace strlcpy() here with strscpy(). No return values were used, so direct replacement is safe. [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [2] https://github.com/KSPP/linux/issues/89 Signed-off-by: Azeem Shaikh Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20230509014136.2095900-1-azeemshaikh38@gmail.com --- fs/befs/btree.c | 2 +- fs/befs/linuxvfs.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'fs') diff --git a/fs/befs/btree.c b/fs/befs/btree.c index 1b7e0f7128d6..53b36aa29978 100644 --- a/fs/befs/btree.c +++ b/fs/befs/btree.c @@ -500,7 +500,7 @@ befs_btree_read(struct super_block *sb, const befs_data_stream *ds, goto error_alloc; } - strlcpy(keybuf, keystart, keylen + 1); + strscpy(keybuf, keystart, keylen + 1); *value = fs64_to_cpu(sb, valarray[cur_key]); *keysize = keylen; diff --git a/fs/befs/linuxvfs.c b/fs/befs/linuxvfs.c index 32749fcee090..eee9237386e2 100644 --- a/fs/befs/linuxvfs.c +++ b/fs/befs/linuxvfs.c @@ -374,7 +374,7 @@ static struct inode *befs_iget(struct super_block *sb, unsigned long ino) if (S_ISLNK(inode->i_mode) && !(befs_ino->i_flags & BEFS_LONG_SYMLINK)){ inode->i_size = 0; inode->i_blocks = befs_sb->block_size / VFS_BLOCK_SIZE; - strlcpy(befs_ino->i_data.symlink, raw_inode->data.symlink, + strscpy(befs_ino->i_data.symlink, raw_inode->data.symlink, BEFS_SYMLINK_LEN); } else { int num_blks; -- cgit v1.2.3 From 7afb6d8fa81fd8d332f70ead5e35d8c90abb8165 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Mon, 5 Jun 2023 20:05:51 +0300 Subject: jbd2: Avoid printing outside the boundary of the buffer Theoretically possible that "%pg" will take all room for the j_devname and hence the "-%lu" will go outside the boundary due to unconditional sprintf() in use. To make this code more robust, replace two sequential s*printf():s by a single call and then replace forbidden character. It's possible to do this way, because '/' won't ever be in the result of "-%lu". Reviewed-by: Jan Kara Signed-off-by: Andy Shevchenko Signed-off-by: Kees Cook Link: https://lore.kernel.org/r/20230605170553.7835-2-andriy.shevchenko@linux.intel.com --- fs/jbd2/journal.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'fs') diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index 8ae419152ff6..6e17f8f94dfd 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -1491,7 +1491,6 @@ journal_t *jbd2_journal_init_inode(struct inode *inode) { journal_t *journal; sector_t blocknr; - char *p; int err = 0; blocknr = 0; @@ -1515,9 +1514,8 @@ journal_t *jbd2_journal_init_inode(struct inode *inode) journal->j_inode = inode; snprintf(journal->j_devname, sizeof(journal->j_devname), - "%pg", journal->j_dev); - p = strreplace(journal->j_devname, '/', '!'); - sprintf(p, "-%lu", journal->j_inode->i_ino); + "%pg-%lu", journal->j_dev, journal->j_inode->i_ino); + strreplace(journal->j_devname, '/', '!'); jbd2_stats_proc_init(journal); return journal; -- cgit v1.2.3