[PATCH] btrfs: replace deprecated strcpy with strscpy

[PATCH] btrfs: replace deprecated strcpy with strscpy

[Brahmajit Das]

strcpy is deprecated due to lack of bounds checking. This patch replaces
strcpy with strscpy, the recommended alternative for null terminated
strings, to follow best practices.

There are instances where strscpy cannot be used such as where both the
source and destination are character pointers. In that instance we can
use scnprintf or a memcpy.

No functional changes intended.

Link: https://github.com/KSPP/linux/issues/88

Signed-off-by: Brahmajit Das <listout@listout.xyz>
---
 fs/btrfs/ioctl.c   | 2 +-
 fs/btrfs/send.c    | 2 +-
 fs/btrfs/volumes.c | 2 +-
 fs/btrfs/xattr.c   | 5 +++--
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 913acef3f0a9..203f309f00b1 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4200,7 +4200,7 @@ static int btrfs_ioctl_set_fslabel(struct file *file, void __user *arg)
 	}
 
 	spin_lock(&fs_info->super_lock);
-	strcpy(super_block->label, label);
+	strscpy(super_block->label, label);
 	spin_unlock(&fs_info->super_lock);
 	ret = btrfs_commit_transaction(trans);
 
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 2891ec4056c6..66ee9e1b1e96 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -758,7 +758,7 @@ static int send_header(struct send_ctx *sctx)
 {
 	struct btrfs_stream_header hdr;
 
-	strcpy(hdr.magic, BTRFS_SEND_STREAM_MAGIC);
+	strscpy(hdr.magic, BTRFS_SEND_STREAM_MAGIC);
 	hdr.version = cpu_to_le32(sctx->proto);
 	return write_buf(sctx->send_filp, &hdr, sizeof(hdr),
 					&sctx->send_off);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 89835071cfea..ec5304f19ac2 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -215,7 +215,7 @@ void btrfs_describe_block_groups(u64 bg_flags, char *buf, u32 size_buf)
 	u32 size_bp = size_buf;
 
 	if (!flags) {
-		strcpy(bp, "NONE");
+		memcpy(bp, "NONE", 4);
 		return;
 	}
 
diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index 3e0edbcf73e1..6b3485112840 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -12,6 +12,7 @@
 #include <linux/posix_acl_xattr.h>
 #include <linux/iversion.h>
 #include <linux/sched/mm.h>
+#include <linux/string.h>
 #include "ctree.h"
 #include "fs.h"
 #include "messages.h"
@@ -516,8 +517,8 @@ static int btrfs_initxattrs(struct inode *inode,
 			ret = -ENOMEM;
 			break;
 		}
-		strcpy(name, XATTR_SECURITY_PREFIX);
-		strcpy(name + XATTR_SECURITY_PREFIX_LEN, xattr->name);
+		scnprintf(name, sizeof(name), "%s%s", XATTR_SECURITY_PREFIX,
+			  xattr->name);
 
 		if (strcmp(name, XATTR_NAME_CAPS) == 0)
 			clear_bit(BTRFS_INODE_NO_CAP_XATTR, &BTRFS_I(inode)->runtime_flags);
-- 
2.50.0

[PATCH v2] btrfs: replace deprecated strcpy with strscpy

[Brahmajit Das]

strcpy is deprecated due to lack of bounds checking. This patch replaces
strcpy with strscpy, the recommended alternative for null terminated
strings, to follow best practices.

There are instances where strscpy cannot be used such as where both the
source and destination are character pointers. In that instance we can
use sysfs_emit or a memcpy.

Update in v2: using sysfs_emit instead of scnprintf

No functional changes intended.

Link: https://github.com/KSPP/linux/issues/88

Suggested-by: Anthony Iliopoulos <ailiop@suse.com>
Signed-off-by: Brahmajit Das <listout@listout.xyz>
---
 fs/btrfs/ioctl.c   | 2 +-
 fs/btrfs/send.c    | 2 +-
 fs/btrfs/volumes.c | 2 +-
 fs/btrfs/xattr.c   | 4 ++--
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 913acef3f0a9..203f309f00b1 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4200,7 +4200,7 @@ static int btrfs_ioctl_set_fslabel(struct file *file, void __user *arg)
 	}
 
 	spin_lock(&fs_info->super_lock);
-	strcpy(super_block->label, label);
+	strscpy(super_block->label, label);
 	spin_unlock(&fs_info->super_lock);
 	ret = btrfs_commit_transaction(trans);
 
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 2891ec4056c6..66ee9e1b1e96 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -758,7 +758,7 @@ static int send_header(struct send_ctx *sctx)
 {
 	struct btrfs_stream_header hdr;
 
-	strcpy(hdr.magic, BTRFS_SEND_STREAM_MAGIC);
+	strscpy(hdr.magic, BTRFS_SEND_STREAM_MAGIC);
 	hdr.version = cpu_to_le32(sctx->proto);
 	return write_buf(sctx->send_filp, &hdr, sizeof(hdr),
 					&sctx->send_off);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 89835071cfea..ec5304f19ac2 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -215,7 +215,7 @@ void btrfs_describe_block_groups(u64 bg_flags, char *buf, u32 size_buf)
 	u32 size_bp = size_buf;
 
 	if (!flags) {
-		strcpy(bp, "NONE");
+		memcpy(bp, "NONE", 4);
 		return;
 	}
 
diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index 3e0edbcf73e1..9f652932895c 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -12,6 +12,7 @@
 #include <linux/posix_acl_xattr.h>
 #include <linux/iversion.h>
 #include <linux/sched/mm.h>
+#include <linux/string.h>
 #include "ctree.h"
 #include "fs.h"
 #include "messages.h"
@@ -516,8 +517,7 @@ static int btrfs_initxattrs(struct inode *inode,
 			ret = -ENOMEM;
 			break;
 		}
-		strcpy(name, XATTR_SECURITY_PREFIX);
-		strcpy(name + XATTR_SECURITY_PREFIX_LEN, xattr->name);
+		sysfs_emit(name, "%s%s", XATTR_SECURITY_PREFIX, xattr->name);
 
 		if (strcmp(name, XATTR_NAME_CAPS) == 0)
 			clear_bit(BTRFS_INODE_NO_CAP_XATTR, &BTRFS_I(inode)->runtime_flags);
-- 
2.50.0

Re: [PATCH] btrfs: replace deprecated strcpy with strscpy

[Mark Harmstone]

On 19/06/2025 3.06 pm, Brahmajit Das wrote:
> strcpy is deprecated due to lack of bounds checking. This patch replaces
> strcpy with strscpy, the recommended alternative for null terminated
> strings, to follow best practices.

I think calling strcpy "deprecated" is a bit tendentious. IMHO the way to proceed
is to use KASAN, which catches the misuse of strcpy as well as other bugs.

> ...snip...

> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -215,7 +215,7 @@ void btrfs_describe_block_groups(u64 bg_flags, char *buf, u32 size_buf)
>   	u32 size_bp = size_buf;
>   
>   	if (!flags) {
> -		strcpy(bp, "NONE");
> +		memcpy(bp, "NONE", 4);
>   		return;
>   	}

These aren't equivalent. strcpy copies the source plus its trailing null - the
equivalent would be memcpy(bp, "NONE", 4). So 4 here should really be 5 - but
you shouldn't be hardcoding magic numbers anyway.

On top of that memcpy is just as "unsafe" as strcpy, so there's no benefit to
this particular change. gcc -O2 compiles it the same way anyway:
https://godbolt.org/z/8fEaKTTzo

Mark

Re: [PATCH v2] btrfs: replace deprecated strcpy with strscpy

[Mark Harmstone]

On 19/06/2025 4.39 pm, Brahmajit Das wrote:
> strcpy is deprecated due to lack of bounds checking. This patch replaces
> strcpy with strscpy, the recommended alternative for null terminated
> strings, to follow best practices.
> 
> There are instances where strscpy cannot be used such as where both the
> source and destination are character pointers. In that instance we can
> use sysfs_emit or a memcpy.
> 
> Update in v2: using sysfs_emit instead of scnprintf
> 
> No functional changes intended.
> 
> Link: https://github.com/KSPP/linux/issues/88
> 
> Suggested-by: Anthony Iliopoulos <ailiop@suse.com>
> Signed-off-by: Brahmajit Das <listout@listout.xyz>
> ---
>   fs/btrfs/ioctl.c   | 2 +-
>   fs/btrfs/send.c    | 2 +-
>   fs/btrfs/volumes.c | 2 +-
>   fs/btrfs/xattr.c   | 4 ++--
>   4 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 913acef3f0a9..203f309f00b1 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4200,7 +4200,7 @@ static int btrfs_ioctl_set_fslabel(struct file *file, void __user *arg)
>   	}
>   
>   	spin_lock(&fs_info->super_lock);
> -	strcpy(super_block->label, label);
> +	strscpy(super_block->label, label);
>   	spin_unlock(&fs_info->super_lock);
>   	ret = btrfs_commit_transaction(trans);

Surely this doesn't compile... strscpy takes three parameters.

> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 2891ec4056c6..66ee9e1b1e96 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -758,7 +758,7 @@ static int send_header(struct send_ctx *sctx)
>   {
>   	struct btrfs_stream_header hdr;
>   
> -	strcpy(hdr.magic, BTRFS_SEND_STREAM_MAGIC);
> +	strscpy(hdr.magic, BTRFS_SEND_STREAM_MAGIC);
>   	hdr.version = cpu_to_le32(sctx->proto);
>   	return write_buf(sctx->send_filp, &hdr, sizeof(hdr),
>   					&sctx->send_off);
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 89835071cfea..ec5304f19ac2 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -215,7 +215,7 @@ void btrfs_describe_block_groups(u64 bg_flags, char *buf, u32 size_buf)
>   	u32 size_bp = size_buf;
>   
>   	if (!flags) {
> -		strcpy(bp, "NONE");
> +		memcpy(bp, "NONE", 4);
>   		return;
>   	}

Same issue here as with the other patch.

>   
> diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
> index 3e0edbcf73e1..9f652932895c 100644
> --- a/fs/btrfs/xattr.c
> +++ b/fs/btrfs/xattr.c
> @@ -12,6 +12,7 @@
>   #include <linux/posix_acl_xattr.h>
>   #include <linux/iversion.h>
>   #include <linux/sched/mm.h>
> +#include <linux/string.h>
>   #include "ctree.h"
>   #include "fs.h"
>   #include "messages.h"
> @@ -516,8 +517,7 @@ static int btrfs_initxattrs(struct inode *inode,
>   			ret = -ENOMEM;
>   			break;
>   		}
> -		strcpy(name, XATTR_SECURITY_PREFIX);
> -		strcpy(name + XATTR_SECURITY_PREFIX_LEN, xattr->name);
> +		sysfs_emit(name, "%s%s", XATTR_SECURITY_PREFIX, xattr->name);
>   
>   		if (strcmp(name, XATTR_NAME_CAPS) == 0)
>   			clear_bit(BTRFS_INODE_NO_CAP_XATTR, &BTRFS_I(inode)->runtime_flags);

Re: [PATCH v2] btrfs: replace deprecated strcpy with strscpy

[Brahmajit Das]

On 19.06.2025 18:06, Mark Harmstone wrote:
> On 19/06/2025 4.39 pm, Brahmajit Das wrote:
...
> 
> Surely this doesn't compile... strscpy takes three parameters.
> 
It does, the third parameter is optional. From include/linux/string.h

#define strscpy(dst, src, ...) \
	CONCATENATE(__strscpy, COUNT_ARGS(__VA_ARGS__))(dst, src, __VA_ARGS__)

But I'm more than happy to add the third parameter.

-- 
Regards,
listout

Re: [PATCH] btrfs: replace deprecated strcpy with strscpy

[Brahmajit Das]

On 19.06.2025 18:03, Mark Harmstone wrote:
> On 19/06/2025 3.06 pm, Brahmajit Das wrote:
> > strcpy is deprecated due to lack of bounds checking. This patch replaces
> > strcpy with strscpy, the recommended alternative for null terminated
> > strings, to follow best practices.
> 
> I think calling strcpy "deprecated" is a bit tendentious. IMHO the way to proceed
> is to use KASAN, which catches the misuse of strcpy as well as other bugs.
> 
Understood, thanks for point it out.
> > ...snip...
> 
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -215,7 +215,7 @@ void btrfs_describe_block_groups(u64 bg_flags, char *buf, u32 size_buf)
> >   	u32 size_bp = size_buf;
> >   	if (!flags) {
> > -		strcpy(bp, "NONE");
> > +		memcpy(bp, "NONE", 4);
> >   		return;
> >   	}
> 
> These aren't equivalent. strcpy copies the source plus its trailing null - the
> equivalent would be memcpy(bp, "NONE", 4). So 4 here should really be 5 - but
> you shouldn't be hardcoding magic numbers anyway.
> 
> On top of that memcpy is just as "unsafe" as strcpy, so there's no benefit to
> this particular change. gcc -O2 compiles it the same way anyway:
> https://godbolt.org/z/8fEaKTTzo
> 
> Mark
> 

I was planning to use strscpy, but it doesn't work with char pointers,
hence went with memcpy. If you or anyone has a better approach for this,
I'm more than happy to send that as a v3.
-- 
Regards,
listout

Re: [PATCH v2] btrfs: replace deprecated strcpy with strscpy

[Mark Harmstone]

On 19/06/2025 6.59 pm, Brahmajit Das wrote:
> On 19.06.2025 18:06, Mark Harmstone wrote:
>> On 19/06/2025 4.39 pm, Brahmajit Das wrote:
> ...
>>
>> Surely this doesn't compile... strscpy takes three parameters.
>>
> It does, the third parameter is optional. From include/linux/string.h
> 
> #define strscpy(dst, src, ...) \
> 	CONCATENATE(__strscpy, COUNT_ARGS(__VA_ARGS__))(dst, src, __VA_ARGS__)
> 
> But I'm more than happy to add the third parameter.

Okay. It looks like this was added by e6584c3964f2ff76a9fb5a701e4a59997b35e547 in
2023 - the docs don't seem to have caught up yet.

[PATCH v3] btrfs: replace deprecated strcpy with strscpy

[Brahmajit Das]

strcpy is deprecated due to lack of bounds checking. This patch replaces
strcpy with strscpy, the recommended alternative for null terminated
strings, to follow best practices.

There are instances where strscpy cannot be used such as where both the
source and destination are character pointers. In that instance we can
use sysfs_emit or a memcpy.

Update in v2: using sysfs_emit instead of scnprintf
Update in v3: Removed string.h in xattr, since we are not using any
fucntions from string.h and fixed length in memcpy in volumes.c

No functional changes intended.

Link: https://github.com/KSPP/linux/issues/88

Suggested-by: Anthony Iliopoulos <ailiop@suse.com>
Suggested-by: Mark Harmstone <mark@harmstone.com>
Signed-off-by: Brahmajit Das <listout@listout.xyz>
---
 fs/btrfs/ioctl.c   | 2 +-
 fs/btrfs/send.c    | 2 +-
 fs/btrfs/volumes.c | 2 +-
 fs/btrfs/xattr.c   | 3 +--
 4 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 913acef3f0a9..203f309f00b1 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4200,7 +4200,7 @@ static int btrfs_ioctl_set_fslabel(struct file *file, void __user *arg)
 	}
 
 	spin_lock(&fs_info->super_lock);
-	strcpy(super_block->label, label);
+	strscpy(super_block->label, label);
 	spin_unlock(&fs_info->super_lock);
 	ret = btrfs_commit_transaction(trans);
 
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 2891ec4056c6..66ee9e1b1e96 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -758,7 +758,7 @@ static int send_header(struct send_ctx *sctx)
 {
 	struct btrfs_stream_header hdr;
 
-	strcpy(hdr.magic, BTRFS_SEND_STREAM_MAGIC);
+	strscpy(hdr.magic, BTRFS_SEND_STREAM_MAGIC);
 	hdr.version = cpu_to_le32(sctx->proto);
 	return write_buf(sctx->send_filp, &hdr, sizeof(hdr),
 					&sctx->send_off);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 89835071cfea..86a898bb2fbb 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -215,7 +215,7 @@ void btrfs_describe_block_groups(u64 bg_flags, char *buf, u32 size_buf)
 	u32 size_bp = size_buf;
 
 	if (!flags) {
-		strcpy(bp, "NONE");
+		memcpy(bp, "NONE", 5);
 		return;
 	}
 
diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index 3e0edbcf73e1..49fd8a49584a 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -516,8 +516,7 @@ static int btrfs_initxattrs(struct inode *inode,
 			ret = -ENOMEM;
 			break;
 		}
-		strcpy(name, XATTR_SECURITY_PREFIX);
-		strcpy(name + XATTR_SECURITY_PREFIX_LEN, xattr->name);
+		sysfs_emit(name, "%s%s", XATTR_SECURITY_PREFIX, xattr->name);
 
 		if (strcmp(name, XATTR_NAME_CAPS) == 0)
 			clear_bit(BTRFS_INODE_NO_CAP_XATTR, &BTRFS_I(inode)->runtime_flags);
-- 
2.50.0

Re: [PATCH v3] btrfs: replace deprecated strcpy with strscpy

[Mark Harmstone]

On 20/06/2025 2.43 am, Brahmajit Das wrote:
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -215,7 +215,7 @@ void btrfs_describe_block_groups(u64 bg_flags, char *buf, u32 size_buf)
>   	u32 size_bp = size_buf;
>   
>   	if (!flags) {
> -		strcpy(bp, "NONE");
> +		memcpy(bp, "NONE", 5);
>   		return;
>   	}

There's still the problem here that you're hardcoding a magic number,
and that memcpy is no safer than strcpy.

If your aim is to get rid of strcpy entirely in the codebase, this is
more than likely impossible.

Re: [PATCH] btrfs: replace deprecated strcpy with strscpy

[David Sterba]

On Thu, Jun 19, 2025 at 11:32:58PM +0530, Brahmajit Das wrote:
> On 19.06.2025 18:03, Mark Harmstone wrote:
> > On 19/06/2025 3.06 pm, Brahmajit Das wrote:
> > > strcpy is deprecated due to lack of bounds checking. This patch replaces
> > > strcpy with strscpy, the recommended alternative for null terminated
> > > strings, to follow best practices.
> > 
> > I think calling strcpy "deprecated" is a bit tendentious. IMHO the way to proceed
> > is to use KASAN, which catches the misuse of strcpy as well as other bugs.
> > 
> Understood, thanks for point it out.
> > > ...snip...
> > 
> > > --- a/fs/btrfs/volumes.c
> > > +++ b/fs/btrfs/volumes.c
> > > @@ -215,7 +215,7 @@ void btrfs_describe_block_groups(u64 bg_flags, char *buf, u32 size_buf)
> > >   	u32 size_bp = size_buf;
> > >   	if (!flags) {
> > > -		strcpy(bp, "NONE");
> > > +		memcpy(bp, "NONE", 4);
> > >   		return;
> > >   	}
> > 
> > These aren't equivalent. strcpy copies the source plus its trailing null - the
> > equivalent would be memcpy(bp, "NONE", 4). So 4 here should really be 5 - but
> > you shouldn't be hardcoding magic numbers anyway.
> > 
> > On top of that memcpy is just as "unsafe" as strcpy, so there's no benefit to
> > this particular change. gcc -O2 compiles it the same way anyway:
> > https://godbolt.org/z/8fEaKTTzo
> > 
> > Mark
> > 
> 
> I was planning to use strscpy, but it doesn't work with char pointers,
> hence went with memcpy. If you or anyone has a better approach for this,
> I'm more than happy to send that as a v3.

As the code is structured, you can move "NONE" as initial value of buf
in describe_relocation() and just return from "if (!flags)".

Re: [PATCH v3] btrfs: replace deprecated strcpy with strscpy

[David Sterba]

On Fri, Jun 20, 2025 at 07:13:44AM +0530, Brahmajit Das wrote:
> strcpy is deprecated due to lack of bounds checking. This patch replaces
> strcpy with strscpy, the recommended alternative for null terminated
> strings, to follow best practices.
> 
> There are instances where strscpy cannot be used such as where both the
> source and destination are character pointers. In that instance we can
> use sysfs_emit or a memcpy.
> 
> Update in v2: using sysfs_emit instead of scnprintf
> Update in v3: Removed string.h in xattr, since we are not using any
> fucntions from string.h and fixed length in memcpy in volumes.c

This should be placed under the "---" marker. If it's a new information
relevant for the patch then it should be a normal part of the changelog.

> No functional changes intended.

No need to write this.

> 
> Link: https://github.com/KSPP/linux/issues/88

No newline here.

> Suggested-by: Anthony Iliopoulos <ailiop@suse.com>
> Suggested-by: Mark Harmstone <mark@harmstone.com>
> Signed-off-by: Brahmajit Das <listout@listout.xyz>

Otherwise it looks good, thanks.