Re: [PATCH] replace strcpy with strscpy for safe copy

[David Laight <david.laight.linux@gmail.com>]

On Wed, 22 Oct 2025 20:23:18 +0100
Al Viro <viro@zeniv.linux.org.uk> wrote:

> On Tue, Oct 21, 2025 at 08:09:52PM +0530, Biancaa Ramesh wrote:
> 
> > diff --git a/fs/ufs/dir.c b/fs/ufs/dir.c
> > index 0388a1bae326..cffb7863adc5 100644
> > --- a/fs/ufs/dir.c
> > +++ b/fs/ufs/dir.c
> > @@ -557,14 +557,14 @@ int ufs_make_empty(struct inode * inode, struct inode *dir)
> >  	ufs_set_de_type(sb, de, inode->i_mode);
> >  	ufs_set_de_namlen(sb, de, 1);
> >  	de->d_reclen = cpu_to_fs16(sb, UFS_DIR_REC_LEN(1));
> > -	strcpy (de->d_name, ".");
> > +	strscpy(de->d_name, ".", sizeof(de->d_name));
> >  	de = (struct ufs_dir_entry *)
> >  		((char *)de + fs16_to_cpu(sb, de->d_reclen));
> >  	de->d_ino = cpu_to_fs32(sb, dir->i_ino);
> >  	ufs_set_de_type(sb, de, dir->i_mode);
> >  	de->d_reclen = cpu_to_fs16(sb, chunk_size - UFS_DIR_REC_LEN(1));
> >  	ufs_set_de_namlen(sb, de, 2);
> > -	strcpy (de->d_name, "..");
> > +	strscpy(de->d_name, "..", sizeof(de->d_name));
> >  	kunmap_local(kaddr);  
> 
> Hard NAK.  This kind of cargo-culting is completely pointless.
> 
> Think for a second.  Really.  We are creating "." and ".." entries in freshly
> created directory.  What your change does is "if directory entry name couldn't
> hold a 2-character string, we might have trouble".  No shit - we would.  Not of
> the "overflow something" variety, actually, but there's not much use for a
> filesystem that could only handle single-character filenames, is there?
> 
> What's worse, you are papering over a real subtlety here: directory entries on
> UFS are variable-length.  There is a fixed-sized header (8 bytes), followed by
> NUL-terminated name.  The size of entry is encoded in 16bit field in the header
> (offset 4), and name (including NUL) must not be longer than entry length - 8.
> 
> struct ufs_dir_entry describes the entry layout, all right, with ->d_name[]
> being the last member.  It is declared as
>         __u8    d_name[UFS_MAXNAMLEN + 1];      /* file name */
> which is to say, the longest we might need (255+1).  So your changes are basically
> 'check that "." or ".." aren't longer than 255 characters to make sure we are
> memory-safe'.  However, that does *NOT* guarantee memory safety - the first
> entry is actually only 12 bytes long, while the second one spans the rest of the
> block.  What is relevant is "entry size is at least UFS_DIR_REC_LEN(strlen(name))",
> which is true for both entries - the first one is explicitly UFS_DIR_REC_LEN(1)
> bytes long, the second - block size - UFS_DIR_REC_LEN(1), which is going to be
> greater than UFS_DIR_REC_LEN(2).  Block size is going to be over twenty four
> bytes, after all...
> 
> What we ought to do is turning ->d_name into a flex array:
>         __u8    d_name[];      /* file name, no more than UFS_MAXNAMLEN + 1 */
> at which point your obfuscation^Wimprovement falls apart.
> 
> Note that
> 	* use of strscpy() here was *not* any safer than strcpy()
> 	* it _pretended_ to improve safety ("move along, nothing to look
> at in this place"), but at the closer look result was a lot more fishy 
> than the original; it reads as "we have 256 bytes there", which is simply
> false.
> 
> This is not an improvement.
> 

It is also likely to make the code much worse, strscpy() is already slower
than strcpy() - because the compiler knows about strcpy().
The copy of "." can reduce to the write of a 16bit constant.
The copy of ".." is more problematic, a memcpy() of "..\0" might be better.

	David