[PATCH] replace strcpy with strscpy for safe copy

[PATCH] replace strcpy with strscpy for safe copy

[Biancaa Ramesh]

Signed-off-by: Biancaa Ramesh <biancaa2210329@ssn.edu.in>
---
 fs/ufs/dir.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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);
 
 	ufs_commit_chunk(folio, 0, chunk_size);
-- 
2.43.0


-- 
::DISCLAIMER::

---------------------------------------------------------------------
The 
contents of this e-mail and any attachment(s) are confidential and
intended 
for the named recipient(s) only. Views or opinions, if any,
presented in 
this email are solely those of the author and may not
necessarily reflect 
the views or opinions of SSN Institutions (SSN) or its
affiliates. Any form 
of reproduction, dissemination, copying, disclosure,
modification, 
distribution and / or publication of this message without the
prior written 
consent of authorized representative of SSN is strictly
prohibited. If you 
have received this email in error please delete it and
notify the sender 
immediately.
---------------------------------------------------------------------
Header of this mail should have a valid DKIM signature for the domain 
ssn.edu.in <http://www.ssn.edu.in/>

[PATCH] replace strcpy with strscpy for safe copy

[Biancaa Ramesh]

Signed-off-by: Biancaa Ramesh <biancaa2210329@ssn.edu.in>
---
 mm/shmem.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index b9081b817d28..6e5a5d6fc7e9 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -657,17 +657,18 @@ static int shmem_parse_huge(const char *str)
 	if (!str)
 		return -EINVAL;
 
-	if (!strcmp(str, "never"))
+	if (!strncmp(str,"never",strlen("never")+1)){
 		huge = SHMEM_HUGE_NEVER;
-	else if (!strcmp(str, "always"))
+	}
+	else if (!strncmp(str, "always", strlen("always") + 1))
 		huge = SHMEM_HUGE_ALWAYS;
-	else if (!strcmp(str, "within_size"))
+	else if (!strncmp(str, "within_size",strlen("Within_size")+1))
 		huge = SHMEM_HUGE_WITHIN_SIZE;
-	else if (!strcmp(str, "advise"))
+	else if (!strncmp(str,"advise",strlen("advise")+1))
 		huge = SHMEM_HUGE_ADVISE;
-	else if (!strcmp(str, "deny"))
+	else if (!strncmp(str,"deny",strlen("deny")+1))
 		huge = SHMEM_HUGE_DENY;
-	else if (!strcmp(str, "force"))
+	else if (!strncmp(str,"force",strlen("force")+1))
 		huge = SHMEM_HUGE_FORCE;
 	else
 		return -EINVAL;
@@ -5679,27 +5680,27 @@ static int __init setup_thp_shmem(char *str)
 				goto err;
 
 			nr = end - start + 1;
-			if (!strcmp(policy, "always")) {
+			if (!strncmp(policy,"always",strlen("always")+1)){
 				bitmap_set(&always, start, nr);
 				bitmap_clear(&inherit, start, nr);
 				bitmap_clear(&madvise, start, nr);
 				bitmap_clear(&within_size, start, nr);
-			} else if (!strcmp(policy, "advise")) {
+			} else if (!strncmp(policy,"advise",strlen("advise")+1)){
 				bitmap_set(&madvise, start, nr);
 				bitmap_clear(&inherit, start, nr);
 				bitmap_clear(&always, start, nr);
 				bitmap_clear(&within_size, start, nr);
-			} else if (!strcmp(policy, "inherit")) {
+			} else if (!strncmp(policy,"inherit",strlen("inherit")+1)){
 				bitmap_set(&inherit, start, nr);
 				bitmap_clear(&madvise, start, nr);
 				bitmap_clear(&always, start, nr);
 				bitmap_clear(&within_size, start, nr);
-			} else if (!strcmp(policy, "within_size")) {
+			} else if (!strncmp(policy,"within_size",strlen("within_size")+1)){
 				bitmap_set(&within_size, start, nr);
 				bitmap_clear(&inherit, start, nr);
 				bitmap_clear(&madvise, start, nr);
 				bitmap_clear(&always, start, nr);
-			} else if (!strcmp(policy, "never")) {
+			} else if (!strncmp(policy,"never",strlen("never")+1)){
 				bitmap_clear(&inherit, start, nr);
 				bitmap_clear(&madvise, start, nr);
 				bitmap_clear(&always, start, nr);
-- 
2.43.0


-- 
::DISCLAIMER::

---------------------------------------------------------------------
The 
contents of this e-mail and any attachment(s) are confidential and
intended 
for the named recipient(s) only. Views or opinions, if any,
presented in 
this email are solely those of the author and may not
necessarily reflect 
the views or opinions of SSN Institutions (SSN) or its
affiliates. Any form 
of reproduction, dissemination, copying, disclosure,
modification, 
distribution and / or publication of this message without the
prior written 
consent of authorized representative of SSN is strictly
prohibited. If you 
have received this email in error please delete it and
notify the sender 
immediately.
---------------------------------------------------------------------
Header of this mail should have a valid DKIM signature for the domain 
ssn.edu.in <http://www.ssn.edu.in/>

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

[Al Viro]

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.

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

[David Laight]

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

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

[Barry Song]

On Wed, Oct 22, 2025 at 1:31 PM Biancaa Ramesh
<biancaa2210329@ssn.edu.in> wrote:
>
> Signed-off-by: Biancaa Ramesh <biancaa2210329@ssn.edu.in>
> ---
>  mm/shmem.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index b9081b817d28..6e5a5d6fc7e9 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -657,17 +657,18 @@ static int shmem_parse_huge(const char *str)
>         if (!str)
>                 return -EINVAL;
>
> -       if (!strcmp(str, "never"))
> +       if (!strncmp(str,"never",strlen("never")+1)){
>                 huge = SHMEM_HUGE_NEVER;
> -       else if (!strcmp(str, "always"))
> +       }
> +       else if (!strncmp(str, "always", strlen("always") + 1))
>                 huge = SHMEM_HUGE_ALWAYS;
> -       else if (!strcmp(str, "within_size"))
> +       else if (!strncmp(str, "within_size",strlen("Within_size")+1))
>                 huge = SHMEM_HUGE_WITHIN_SIZE;
> -       else if (!strcmp(str, "advise"))
> +       else if (!strncmp(str,"advise",strlen("advise")+1))
>                 huge = SHMEM_HUGE_ADVISE;
> -       else if (!strcmp(str, "deny"))
> +       else if (!strncmp(str,"deny",strlen("deny")+1))
>                 huge = SHMEM_HUGE_DENY;
> -       else if (!strcmp(str, "force"))
> +       else if (!strncmp(str,"force",strlen("force")+1))
>                 huge = SHMEM_HUGE_FORCE;
>         else
>                 return -EINVAL;


I think this patch is incorrect. If the goal isn’t to guard against
stack overflow,
it shouldn’t use strlen("never"); it should use the length of str.
On the other hand, we should ensure that str is large enough to hold values like
“never” or “always”.

Thanks
Barry

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

[David Laight]

On Tue, 21 Oct 2025 20:27:00 +0530
Biancaa Ramesh <biancaa2210329@ssn.edu.in> wrote:

A complete pile of bollocks....

> Signed-off-by: Biancaa Ramesh <biancaa2210329@ssn.edu.in>
> ---
>  mm/shmem.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index b9081b817d28..6e5a5d6fc7e9 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -657,17 +657,18 @@ static int shmem_parse_huge(const char *str)
>  	if (!str)
>  		return -EINVAL;
>  
> -	if (!strcmp(str, "never"))
> +	if (!strncmp(str,"never",strlen("never")+1)){
>  		huge = SHMEM_HUGE_NEVER;
> -	else if (!strcmp(str, "always"))
> +	}
> +	else if (!strncmp(str, "always", strlen("always") + 1))
>  		huge = SHMEM_HUGE_ALWAYS;
> -	else if (!strcmp(str, "within_size"))
> +	else if (!strncmp(str, "within_size",strlen("Within_size")+1))
>  		huge = SHMEM_HUGE_WITHIN_SIZE;
> -	else if (!strcmp(str, "advise"))
> +	else if (!strncmp(str,"advise",strlen("advise")+1))
>  		huge = SHMEM_HUGE_ADVISE;
> -	else if (!strcmp(str, "deny"))
> +	else if (!strncmp(str,"deny",strlen("deny")+1))
>  		huge = SHMEM_HUGE_DENY;
> -	else if (!strcmp(str, "force"))
> +	else if (!strncmp(str,"force",strlen("force")+1))
>  		huge = SHMEM_HUGE_FORCE;
>  	else
>  		return -EINVAL;
> @@ -5679,27 +5680,27 @@ static int __init setup_thp_shmem(char *str)
>  				goto err;
>  
>  			nr = end - start + 1;
> -			if (!strcmp(policy, "always")) {
> +			if (!strncmp(policy,"always",strlen("always")+1)){
>  				bitmap_set(&always, start, nr);
>  				bitmap_clear(&inherit, start, nr);
>  				bitmap_clear(&madvise, start, nr);
>  				bitmap_clear(&within_size, start, nr);
> -			} else if (!strcmp(policy, "advise")) {
> +			} else if (!strncmp(policy,"advise",strlen("advise")+1)){
>  				bitmap_set(&madvise, start, nr);
>  				bitmap_clear(&inherit, start, nr);
>  				bitmap_clear(&always, start, nr);
>  				bitmap_clear(&within_size, start, nr);
> -			} else if (!strcmp(policy, "inherit")) {
> +			} else if (!strncmp(policy,"inherit",strlen("inherit")+1)){
>  				bitmap_set(&inherit, start, nr);
>  				bitmap_clear(&madvise, start, nr);
>  				bitmap_clear(&always, start, nr);
>  				bitmap_clear(&within_size, start, nr);
> -			} else if (!strcmp(policy, "within_size")) {
> +			} else if (!strncmp(policy,"within_size",strlen("within_size")+1)){
>  				bitmap_set(&within_size, start, nr);
>  				bitmap_clear(&inherit, start, nr);
>  				bitmap_clear(&madvise, start, nr);
>  				bitmap_clear(&always, start, nr);
> -			} else if (!strcmp(policy, "never")) {
> +			} else if (!strncmp(policy,"never",strlen("never")+1)){
>  				bitmap_clear(&inherit, start, nr);
>  				bitmap_clear(&madvise, start, nr);
>  				bitmap_clear(&always, start, nr);