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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251022192318.GI2441659@ZenIV>
Date: Wed, 22 Oct 2025 20:23:18 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Biancaa Ramesh <biancaa2210329@....edu.in>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH] replace strcpy with strscpy for safe copy

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.