* [PATCH] replace strcpy with strscpy for safe copy
@ 2025-10-21 14:57 Biancaa Ramesh
2025-10-24 0:55 ` Barry Song
2025-10-24 8:37 ` David Laight
0 siblings, 2 replies; 6+ messages in thread
From: Biancaa Ramesh @ 2025-10-21 14:57 UTC (permalink / raw)
To: linux-kernel; +Cc: 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/>
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] replace strcpy with strscpy for safe copy
2025-10-21 14:57 [PATCH] replace strcpy with strscpy for safe copy Biancaa Ramesh
@ 2025-10-24 0:55 ` Barry Song
2025-10-24 8:37 ` David Laight
1 sibling, 0 replies; 6+ messages in thread
From: Barry Song @ 2025-10-24 0:55 UTC (permalink / raw)
To: Biancaa Ramesh; +Cc: linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] replace strcpy with strscpy for safe copy
2025-10-21 14:57 [PATCH] replace strcpy with strscpy for safe copy Biancaa Ramesh
2025-10-24 0:55 ` Barry Song
@ 2025-10-24 8:37 ` David Laight
1 sibling, 0 replies; 6+ messages in thread
From: David Laight @ 2025-10-24 8:37 UTC (permalink / raw)
To: Biancaa Ramesh; +Cc: linux-kernel
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);
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] replace strcpy with strscpy for safe copy
@ 2025-10-21 14:39 Biancaa Ramesh
2025-10-22 19:23 ` Al Viro
0 siblings, 1 reply; 6+ messages in thread
From: Biancaa Ramesh @ 2025-10-21 14:39 UTC (permalink / raw)
To: linux-kernel; +Cc: 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/>
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] replace strcpy with strscpy for safe copy
2025-10-21 14:39 Biancaa Ramesh
@ 2025-10-22 19:23 ` Al Viro
2025-10-23 8:20 ` David Laight
0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2025-10-22 19:23 UTC (permalink / raw)
To: Biancaa Ramesh; +Cc: linux-kernel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] replace strcpy with strscpy for safe copy
2025-10-22 19:23 ` Al Viro
@ 2025-10-23 8:20 ` David Laight
0 siblings, 0 replies; 6+ messages in thread
From: David Laight @ 2025-10-23 8:20 UTC (permalink / raw)
To: Al Viro; +Cc: Biancaa Ramesh, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-10-24 8:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-21 14:57 [PATCH] replace strcpy with strscpy for safe copy Biancaa Ramesh
2025-10-24 0:55 ` Barry Song
2025-10-24 8:37 ` David Laight
-- strict thread matches above, loose matches on Subject: below --
2025-10-21 14:39 Biancaa Ramesh
2025-10-22 19:23 ` Al Viro
2025-10-23 8:20 ` David Laight
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox