Re: [PATCH 04/11] orangefs: convert strncpy() to strscpy()

Justin Stitt
2024-03-28T23:17:37Z

Hi,

On Thu, Mar 28, 2024 at 03:04:48PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> gcc warns about a truncated string copy with a 255 byte string getting
> copied to a buffer of the same length, losing the 0-termination:
>
> In function 'orangefs_unmount',
> inlined from 'orangefs_kill_sb' at arm-soc/fs/orangefs/super.c:619:6:
> fs/orangefs/super.c:406:9: error: 'strncpy' output may be truncated copying 255 bytes from a string of length 255 [-Werror=stringop-truncation]
> 406 | strncpy(op->upcall.req.fs_umount.orangefs_config_server,
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 407 | devname, ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> I see that most string copies in orangefs are for the upcalls and use
> a buffer that is one short to get the implied termination from the
> zero-filled buffer, but some other instances lack the '-1' part.
>
> Convert from strncpy() to strscpy() to avoids both the warning about
> the buffer size and the need for the explicit padding, since strscpy
> guarantees a zero-terminated buffer.
>

I think I got most of these with my patch sent earlier last week:

https://lore.kernel.org/all/20240322-strncpy-fs-orangefs-dcache-c-v1-1-15d12debbf38@google.com/

>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> fs/orangefs/dcache.c | 4 ++--
> fs/orangefs/namei.c | 33 +++++++++++++++------------------
> fs/orangefs/super.c | 16 +++++++---------
> 3 files changed, 24 insertions(+), 29 deletions(-)
>
> diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c
> index 8bbe9486e3a6..96ed9900f7a9 100644
> --- a/fs/orangefs/dcache.c
> +++ b/fs/orangefs/dcache.c
> @@ -33,9 +33,9 @@ static int orangefs_revalidate_lookup(struct dentry *dentry)
>
> new_op->upcall.req.lookup.sym_follow = ORANGEFS_LOOKUP_LINK_NO_FOLLOW;
> new_op->upcall.req.lookup.parent_refn = parent->refn;
> - strncpy(new_op->upcall.req.lookup.d_name,
> + strscpy(new_op->upcall.req.lookup.d_name,
> dentry->d_name.name,
> - ORANGEFS_NAME_MAX - 1);
> + ORANGEFS_NAME_MAX);
>
> gossip_debug(GOSSIP_DCACHE_DEBUG,
> "%s:%s:%d interrupt flag [%d]\n",
> diff --git a/fs/orangefs/namei.c b/fs/orangefs/namei.c
> index c9dfd5c6a097..5e46d3bdcb05 100644
> --- a/fs/orangefs/namei.c
> +++ b/fs/orangefs/namei.c
> @@ -41,8 +41,8 @@ static int orangefs_create(struct mnt_idmap *idmap,
> fill_default_sys_attrs(new_op->upcall.req.create.attributes,
> ORANGEFS_TYPE_METAFILE, mode);
>
> - strncpy(new_op->upcall.req.create.d_name,
> - dentry->d_name.name, ORANGEFS_NAME_MAX - 1);
> + strscpy(new_op->upcall.req.create.d_name,
> + dentry->d_name.name, ORANGEFS_NAME_MAX);
>
> ret = service_operation(new_op, __func__, get_interruptible_flag(dir));
>
> @@ -137,8 +137,8 @@ static struct dentry *orangefs_lookup(struct inode *dir, struct dentry *dentry,
> &parent->refn.khandle);
> new_op->upcall.req.lookup.parent_refn = parent->refn;
>
> - strncpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name,
> - ORANGEFS_NAME_MAX - 1);
> + strscpy(new_op->upcall.req.lookup.d_name, dentry->d_name.name,
> + ORANGEFS_NAME_MAX);
>
> gossip_debug(GOSSIP_NAME_DEBUG,
> "%s: doing lookup on %s under %pU,%d\n",
> @@ -192,8 +192,8 @@ static int orangefs_unlink(struct inode *dir, struct dentry *dentry)
> return -ENOMEM;
>
> new_op->upcall.req.remove.parent_refn = parent->refn;
> - strncpy(new_op->upcall.req.remove.d_name, dentry->d_name.name,
> - ORANGEFS_NAME_MAX - 1);
> + strscpy(new_op->upcall.req.remove.d_name, dentry->d_name.name,
> + ORANGEFS_NAME_MAX);
>
> ret = service_operation(new_op, "orangefs_unlink",
> get_interruptible_flag(inode));
> @@ -247,10 +247,9 @@ static int orangefs_symlink(struct mnt_idmap *idmap,
> ORANGEFS_TYPE_SYMLINK,
> mode);
>
> - strncpy(new_op->upcall.req.sym.entry_name,
> - dentry->d_name.name,
> - ORANGEFS_NAME_MAX - 1);
> - strncpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX - 1);
> + strscpy(new_op->upcall.req.sym.entry_name,
> + dentry->d_name.name, ORANGEFS_NAME_MAX);
> + strscpy(new_op->upcall.req.sym.target, symname, ORANGEFS_NAME_MAX);
>
> ret = service_operation(new_op, __func__, get_interruptible_flag(dir));
>
> @@ -324,8 +323,8 @@ static int orangefs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
> fill_default_sys_attrs(new_op->upcall.req.mkdir.attributes,
> ORANGEFS_TYPE_DIRECTORY, mode);
>
> - strncpy(new_op->upcall.req.mkdir.d_name,
> - dentry->d_name.name, ORANGEFS_NAME_MAX - 1);
> + strscpy(new_op->upcall.req.mkdir.d_name,
> + dentry->d_name.name, ORANGEFS_NAME_MAX);
>
> ret = service_operation(new_op, __func__, get_interruptible_flag(dir));
>
> @@ -405,12 +404,10 @@ static int orangefs_rename(struct mnt_idmap *idmap,
> new_op->upcall.req.rename.old_parent_refn = ORANGEFS_I(old_dir)->refn;
> new_op->upcall.req.rename.new_parent_refn = ORANGEFS_I(new_dir)->refn;
>
> - strncpy(new_op->upcall.req.rename.d_old_name,
> - old_dentry->d_name.name,
> - ORANGEFS_NAME_MAX - 1);
> - strncpy(new_op->upcall.req.rename.d_new_name,
> - new_dentry->d_name.name,
> - ORANGEFS_NAME_MAX - 1);
> + strscpy(new_op->upcall.req.rename.d_old_name,
> + old_dentry->d_name.name, ORANGEFS_NAME_MAX);
> + strscpy(new_op->upcall.req.rename.d_new_name,
> + new_dentry->d_name.name, ORANGEFS_NAME_MAX);
>
> ret = service_operation(new_op,
> "orangefs_rename",
> diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c
> index d990f4356b30..c714380ab38b 100644
> --- a/fs/orangefs/super.c
> +++ b/fs/orangefs/super.c
> @@ -256,7 +256,7 @@ int orangefs_remount(struct orangefs_sb_info_s *orangefs_sb)
> new_op = op_alloc(ORANGEFS_VFS_OP_FS_MOUNT);
> if (!new_op)
> return -ENOMEM;
> - strncpy(new_op->upcall.req.fs_mount.orangefs_config_server,
> + strscpy(new_op->upcall.req.fs_mount.orangefs_config_server,
> orangefs_sb->devname,
> ORANGEFS_MAX_SERVER_ADDR_LEN);
>
> @@ -403,8 +403,8 @@ static int orangefs_unmount(int id, __s32 fs_id, const char *devname)
> return -ENOMEM;
> op->upcall.req.fs_umount.id = id;
> op->upcall.req.fs_umount.fs_id = fs_id;
> - strncpy(op->upcall.req.fs_umount.orangefs_config_server,
> - devname, ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
> + strscpy(op->upcall.req.fs_umount.orangefs_config_server,
> + devname, ORANGEFS_MAX_SERVER_ADDR_LEN);
> r = service_operation(op, "orangefs_fs_umount", 0);
> /* Not much to do about an error here. */
> if (r)
> @@ -497,9 +497,8 @@ struct dentry *orangefs_mount(struct file_system_type *fst,
> if (!new_op)
> return ERR_PTR(-ENOMEM);
>
> - strncpy(new_op->upcall.req.fs_mount.orangefs_config_server,
> - devname,
> - ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
> + strscpy(new_op->upcall.req.fs_mount.orangefs_config_server,
> + devname, ORANGEFS_MAX_SERVER_ADDR_LEN);
>
> gossip_debug(GOSSIP_SUPER_DEBUG,
> "Attempting ORANGEFS Mount via host %s\n",
> @@ -546,9 +545,8 @@ struct dentry *orangefs_mount(struct file_system_type *fst,
> * on successful mount, store the devname and data
> * used
> */
> - strncpy(ORANGEFS_SB(sb)->devname,
> - devname,
> - ORANGEFS_MAX_SERVER_ADDR_LEN - 1);
> + strscpy(ORANGEFS_SB(sb)->devname, devname,
> + ORANGEFS_MAX_SERVER_ADDR_LEN);
>
> /* mount_pending must be cleared */
> ORANGEFS_SB(sb)->mount_pending = 0;
> --
> 2.39.2
>
Thanks
Justin