Re: [PATCH] Use strscpy() instead of strcpy()

[Abdul Rahim <abdul.rahim@xxxxxxxxxxx>]

On Wed, Nov 13, 2024 at 10:28:36PM +0100, Christophe JAILLET wrote:
> Le 13/11/2024 � 21:15, Abdul Rahim a �crit�:
> > On Tue, Nov 12, 2024 at 07:21:54AM +0100, Christophe JAILLET wrote:
> > > Le 11/11/2024 � 23:10, Abdul Rahim a �crit�:
> > > > strcpy() is generally considered unsafe and use of strscpy() is
> > > > recommended [1]
> > > > 
> > > > this fixes checkpatch warning:
> > > >       WARNING: Prefer strscpy over strcpy
> > > > 
> > > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy [1]
> > > > Signed-off-by: Abdul Rahim <abdul.rahim@xxxxxxxxxxx>
> > > > ---
> > > >    fs/ceph/export.c | 2 +-
> > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/ceph/export.c b/fs/ceph/export.c
> > > > index 44451749c544..0e5b3c7b3756 100644
> > > > --- a/fs/ceph/export.c
> > > > +++ b/fs/ceph/export.c
> > > > @@ -452,7 +452,7 @@ static int __get_snap_name(struct dentry *parent, char *name,
> > > >    		goto out;
> > > >    	if (ceph_snap(inode) == CEPH_SNAPDIR) {
> > > >    		if (ceph_snap(dir) == CEPH_NOSNAP) {
> > > > -			strcpy(name, fsc->mount_options->snapdir_name);
> > > > +			strscpy(name, fsc->mount_options->snapdir_name);
> > > 
> > > This does not compile because when the size of 'name' is not known at
> > > compilation time, you need to use the 3-argument version of strscpy().
> > > 
> > > Please always compile test your patches before sending them. Even, when the
> > > change looks trivial.
> > > 
> > 
> > Sure.
> > 
> > > CJ
> > > 
> > > >    			err = 0;
> > > >    		}
> > > >    		goto out;
> > > 
> > 

> > Should it be: NAME_MAX+1 ?
> > 
> > See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/exportfs.h?h=v6.12-rc7#n203
> > 
> > 
> 
> Looks like a good idea, maybe with a comment related to get_name() in the
> export_operations structure, so that we remind where this limit comes from?
> 
> CJ
> 

diff --git a/fs/ceph/export.c b/fs/ceph/export.c
index 0e5b3c7b3756..48265c879fcf 100644
--- a/fs/ceph/export.c
+++ b/fs/ceph/export.c
@@ -452,7 +452,12 @@ static int __get_snap_name(struct dentry *parent, char *name,
 		goto out;
 	if (ceph_snap(inode) == CEPH_SNAPDIR) {
 		if (ceph_snap(dir) == CEPH_NOSNAP) {
-			strcpy(name, fsc->mount_options->snapdir_name);
+			/*
+			 * get_name assumes that name is pointing to a
+			 * NAME_MAX+1 sized buffer
+			 */
+			strscpy(name, fsc->mount_options->snapdir_name, 
+					NAME_MAX+1);
 			err = 0;
 		}
 		goto out;


Looks good?