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

From: Justin Stitt <justinstitt@google.com>
To: Arnd Bergmann <arnd@kernel.org>
Cc: linux-kernel@vger.kernel.org, Mike Marshall <hubcap@omnibond.com>,
	 Arnd Bergmann <arnd@arndb.de>,
	Martin Brandenburg <martin@omnibond.com>,
	 Jeff Layton <jlayton@kernel.org>, Jan Kara <jack@suse.cz>,
	Christian Brauner <brauner@kernel.org>,
	 Vlastimil Babka <vbabka@suse.cz>,
	devel@lists.orangefs.org
Subject: Re: [PATCH 04/11] orangefs: convert strncpy() to strscpy()
Date: Thu, 28 Mar 2024 23:17:33 +0000	[thread overview]
Message-ID: <l27q6xcktcki7hfn3b6bvoyhu7hppyb2ouolwqfvracvdc4tte@qql4swc5yfbh> (raw)
In-Reply-To: <20240328140512.4148825-5-arnd@kernel.org>

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