[PATCH next] fs/hfsplus/xattr: Use memcpy() and strscpy() to build xattr_name

[PATCH next] fs/hfsplus/xattr: Use memcpy() and strscpy() to build xattr_name

[david.laight.linux]

From: David Laight <david.laight.linux@gmail.com>

xattr_name is kmalloc()ed at the (assumed) maximal size and then the prefix
and name concatenated together.
Use memcpy() for the prefix - its length is passed and strscpy() for the
name to ensure it really doesnt overflow.

Prior to bf29e886b242c the buffers were smaller and on-stack.
(But I cant see the copy in the old code.)
I am also not sure why the buffer isnt created "just long enough".

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---
This is one of a group of patches that remove potentially unbounded
strcpy() calls.

They are mostly replaced by strscpy() or, when strlen() has just been
called, with memcpy() (usually including the '\0').

Calls with copy string literals into arrays are left unchanged.
They are safe and easily detected as such.

The changes were made by getting the compiler to detect the calls and
then fixing the code by hand.

Note that all the changes are only compile tested.

Some Makefiles were changed to allow files to contain strcpy().
As well as 'difficult to fix' files, this included 'show' functions
as they really need to use sysfs_emit() or seq_printf().

All the patches are being sent individually to avoid very long cc lists.
Apologies for the terse commit messages and likely unexpected tags.
(There are about 100 patches in total.)

 fs/hfsplus/xattr.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
index 452a1f9becb2..0b3dd48c28c9 100644
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -550,8 +550,8 @@ int hfsplus_setxattr(struct inode *inode, const char *name,
 	xattr_name = kmalloc(xattr_name_len, GFP_KERNEL);
 	if (!xattr_name)
 		return -ENOMEM;
-	strcpy(xattr_name, prefix);
-	strcpy(xattr_name + prefixlen, name);
+	memcpy(xattr_name, prefix, prefixlen);
+	strscpy(xattr_name + prefixlen, name, xattr_name_len - prefixlen);
 	res = __hfsplus_setxattr(inode, xattr_name, value, size, flags);
 	kfree(xattr_name);
 
@@ -698,6 +698,7 @@ ssize_t hfsplus_getxattr(struct inode *inode, const char *name,
 			 void *value, size_t size,
 			 const char *prefix, size_t prefixlen)
 {
+	size_t xattr_name_len = NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1;
 	int res;
 	char *xattr_name;
 
@@ -705,13 +706,12 @@ ssize_t hfsplus_getxattr(struct inode *inode, const char *name,
 		inode->i_ino, name ? name : NULL,
 		prefix ? prefix : NULL);
 
-	xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
-			     GFP_KERNEL);
+	xattr_name = kmalloc(xattr_name_len, GFP_KERNEL);
 	if (!xattr_name)
 		return -ENOMEM;
 
-	strcpy(xattr_name, prefix);
-	strcpy(xattr_name + prefixlen, name);
+	memcpy(xattr_name, prefix, prefixlen);
+	strscpy(xattr_name + prefixlen, name, xattr_name_len - prefixlen);
 
 	res = __hfsplus_getxattr(inode, xattr_name, value, size);
 	kfree(xattr_name);
-- 
2.39.5

Re: [PATCH next] fs/hfsplus/xattr: Use memcpy() and strscpy() to build xattr_name

[Viacheslav Dubeyko]

On Mon, 2026-06-08 at 10:55 +0100, david.laight.linux@gmail.com wrote:
> From: David Laight <david.laight.linux@gmail.com>
> 
> xattr_name is kmalloc()ed at the (assumed) maximal size and then the
> prefix
> and name concatenated together.
> Use memcpy() for the prefix - its length is passed and strscpy() for
> the
> name to ensure it really doesnt overflow.
> 
> Prior to bf29e886b242c the buffers were smaller and on-stack.
> (But I cant see the copy in the old code.)
> I am also not sure why the buffer isnt created "just long enough".

What do you mean by "the buffer isn't created just long enough"? And
what is your vision of correct implementation of the logic?

Thanks,
Slava.

> 
> Signed-off-by: David Laight <david.laight.linux@gmail.com>
> ---
> This is one of a group of patches that remove potentially unbounded
> strcpy() calls.
> 
> They are mostly replaced by strscpy() or, when strlen() has just been
> called, with memcpy() (usually including the '\0').
> 
> Calls with copy string literals into arrays are left unchanged.
> They are safe and easily detected as such.
> 
> The changes were made by getting the compiler to detect the calls and
> then fixing the code by hand.
> 
> Note that all the changes are only compile tested.
> 
> Some Makefiles were changed to allow files to contain strcpy().
> As well as 'difficult to fix' files, this included 'show' functions
> as they really need to use sysfs_emit() or seq_printf().
> 
> All the patches are being sent individually to avoid very long cc
> lists.
> Apologies for the terse commit messages and likely unexpected tags.
> (There are about 100 patches in total.)
> 
>  fs/hfsplus/xattr.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index 452a1f9becb2..0b3dd48c28c9 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -550,8 +550,8 @@ int hfsplus_setxattr(struct inode *inode, const
> char *name,
>  	xattr_name = kmalloc(xattr_name_len, GFP_KERNEL);
>  	if (!xattr_name)
>  		return -ENOMEM;
> -	strcpy(xattr_name, prefix);
> -	strcpy(xattr_name + prefixlen, name);
> +	memcpy(xattr_name, prefix, prefixlen);
> +	strscpy(xattr_name + prefixlen, name, xattr_name_len -
> prefixlen);
>  	res = __hfsplus_setxattr(inode, xattr_name, value, size,
> flags);
>  	kfree(xattr_name);
>  
> @@ -698,6 +698,7 @@ ssize_t hfsplus_getxattr(struct inode *inode,
> const char *name,
>  			 void *value, size_t size,
>  			 const char *prefix, size_t prefixlen)
>  {
> +	size_t xattr_name_len = NLS_MAX_CHARSET_SIZE *
> HFSPLUS_ATTR_MAX_STRLEN + 1;
>  	int res;
>  	char *xattr_name;
>  
> @@ -705,13 +706,12 @@ ssize_t hfsplus_getxattr(struct inode *inode,
> const char *name,
>  		inode->i_ino, name ? name : NULL,
>  		prefix ? prefix : NULL);
>  
> -	xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE *
> HFSPLUS_ATTR_MAX_STRLEN + 1,
> -			     GFP_KERNEL);
> +	xattr_name = kmalloc(xattr_name_len, GFP_KERNEL);
>  	if (!xattr_name)
>  		return -ENOMEM;
>  
> -	strcpy(xattr_name, prefix);
> -	strcpy(xattr_name + prefixlen, name);
> +	memcpy(xattr_name, prefix, prefixlen);
> +	strscpy(xattr_name + prefixlen, name, xattr_name_len -
> prefixlen);
>  
>  	res = __hfsplus_getxattr(inode, xattr_name, value, size);
>  	kfree(xattr_name);

Re: [PATCH next] fs/hfsplus/xattr: Use memcpy() and strscpy() to build xattr_name

[David Laight]

On Tue, 09 Jun 2026 18:04:46 -0700
Viacheslav Dubeyko <slava@dubeyko.com> wrote:

> On Mon, 2026-06-08 at 10:55 +0100, david.laight.linux@gmail.com wrote:
> > From: David Laight <david.laight.linux@gmail.com>
> > 
> > xattr_name is kmalloc()ed at the (assumed) maximal size and then the
> > prefix
> > and name concatenated together.
> > Use memcpy() for the prefix - its length is passed and strscpy() for
> > the
> > name to ensure it really doesnt overflow.
> > 
> > Prior to bf29e886b242c the buffers were smaller and on-stack.
> > (But I cant see the copy in the old code.)
> > I am also not sure why the buffer isnt created "just long enough".  
> 
> What do you mean by "the buffer isn't created just long enough"? And
> what is your vision of correct implementation of the logic?

The old code is:
>  	xattr_name = kmalloc(xattr_name_len, GFP_KERNEL);
>  	if (!xattr_name)
>  		return -ENOMEM;
> 	strcpy(xattr_name, prefix);
> 	strcpy(xattr_name + prefixlen, name);

It would be more usual to do:
	size_t name_len = strlen(name) + 1;
	xattr_name = kmalloc(prefixlen + name_len);
	memcpy(xattr_name, prefix, prefixlen)
	memcpy(xattr_name + prefixlen, name, name_len);
So that the buffer is allocated the correct size for the strings.

(Not that it really makes much difference whether you do kmalloc(32) or
kmalloc(1024) for a temporary buffer - both come of a per-cpu list.)

What I couldn't decide, but seems to be inferred from some of fixes,
was whether a double-terminator was needed.
(That might just be the attribute value.)
One of the related functions got fixed to zero fill a buffer in a loop
because (AFICT) something was reading beyond a '\0' terminator.

This code does seem strange though.
It seems to add a text prefix here and then the called function does
string compares to find out which prefix has added and than act
differently based on the prefix.
I didn't try to follow things any further.

I also don't know if 'name' itself can be 127 wide characters (before the
prefix is added)?
The fixed length buffer isn't long enough if all 127 characters require
6 bytes to encode.

-- David

> 
> Thanks,
> Slava.

Re: [PATCH next] fs/hfsplus/xattr: Use memcpy() and strscpy() to build xattr_name

[Viacheslav Dubeyko]

On Wed, 2026-06-10 at 10:09 +0100, David Laight wrote:
> On Tue, 09 Jun 2026 18:04:46 -0700
> Viacheslav Dubeyko <slava@dubeyko.com> wrote:
> 
> > On Mon, 2026-06-08 at 10:55 +0100,
> > david.laight.linux@gmail.com wrote:
> > > From: David Laight <david.laight.linux@gmail.com>
> > > 
> > > xattr_name is kmalloc()ed at the (assumed) maximal size and then
> > > the
> > > prefix
> > > and name concatenated together.
> > > Use memcpy() for the prefix - its length is passed and strscpy()
> > > for
> > > the
> > > name to ensure it really doesnt overflow.
> > > 
> > > Prior to bf29e886b242c the buffers were smaller and on-stack.
> > > (But I cant see the copy in the old code.)
> > > I am also not sure why the buffer isnt created "just long
> > > enough".  
> > 
> > What do you mean by "the buffer isn't created just long enough"?
> > And
> > what is your vision of correct implementation of the logic?
> 
> The old code is:
> >  	xattr_name = kmalloc(xattr_name_len, GFP_KERNEL);
> >  	if (!xattr_name)
> >  		return -ENOMEM;
> > 	strcpy(xattr_name, prefix);
> > 	strcpy(xattr_name + prefixlen, name);
> 
> It would be more usual to do:
> 	size_t name_len = strlen(name) + 1;
> 	xattr_name = kmalloc(prefixlen + name_len);
> 	memcpy(xattr_name, prefix, prefixlen)
> 	memcpy(xattr_name + prefixlen, name, name_len);
> So that the buffer is allocated the correct size for the strings.
> 
> (Not that it really makes much difference whether you do kmalloc(32)
> or
> kmalloc(1024) for a temporary buffer - both come of a per-cpu list.)

The xattr name cannot contain more than 127 symbols [1]. So, the buffer
of fixed size is allocated to keep any potential string inside of this
limit no matter of size of the prefix and name. This is because we must
have string that not longer that 127 symbols. If it is longer than
that, then something is going wrong. Following to your logic, there is
no big difference to allocate the buffer for 127 symbols or for precise
length of the name. So, your suggestion doesn't make sense to me.

> 
> What I couldn't decide, but seems to be inferred from some of fixes,
> was whether a double-terminator was needed.
> (That might just be the attribute value.)
> One of the related functions got fixed to zero fill a buffer in a
> loop
> because (AFICT) something was reading beyond a '\0' terminator.

The rest is dedicated to Unicode peculiarities, as far as I can see.

> 
> This code does seem strange though.
> It seems to add a text prefix here and then the called function does
> string compares to find out which prefix has added and than act
> differently based on the prefix.
> I didn't try to follow things any further.

There is no strange thing here. Different prefixes are processed by
different logic.

> 
> I also don't know if 'name' itself can be 127 wide characters (before
> the
> prefix is added)?
> The fixed length buffer isn't long enough if all 127 characters
> require
> 6 bytes to encode.

The whole string cannot be bigger than 127 symbols.

Thanks,
Slava.

[1] https://elixir.bootlin.com/linux/v7.1-rc6/source/include/linux/hfs_common.h#L78

Re: [PATCH next] fs/hfsplus/xattr: Use memcpy() and strscpy() to build xattr_name

[Viacheslav Dubeyko]

On Mon, 2026-06-08 at 10:55 +0100, david.laight.linux@gmail.com wrote:
> From: David Laight <david.laight.linux@gmail.com>
> 
> xattr_name is kmalloc()ed at the (assumed) maximal size and then the
> prefix
> and name concatenated together.
> Use memcpy() for the prefix - its length is passed and strscpy() for
> the
> name to ensure it really doesnt overflow.
> 
> Prior to bf29e886b242c the buffers were smaller and on-stack.
> (But I cant see the copy in the old code.)
> I am also not sure why the buffer isnt created "just long enough".
> 
> Signed-off-by: David Laight <david.laight.linux@gmail.com>
> ---
> This is one of a group of patches that remove potentially unbounded
> strcpy() calls.
> 
> They are mostly replaced by strscpy() or, when strlen() has just been
> called, with memcpy() (usually including the '\0').
> 
> Calls with copy string literals into arrays are left unchanged.
> They are safe and easily detected as such.
> 
> The changes were made by getting the compiler to detect the calls and
> then fixing the code by hand.
> 
> Note that all the changes are only compile tested.
> 
> Some Makefiles were changed to allow files to contain strcpy().
> As well as 'difficult to fix' files, this included 'show' functions
> as they really need to use sysfs_emit() or seq_printf().
> 
> All the patches are being sent individually to avoid very long cc
> lists.
> Apologies for the terse commit messages and likely unexpected tags.
> (There are about 100 patches in total.)
> 
>  fs/hfsplus/xattr.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index 452a1f9becb2..0b3dd48c28c9 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -550,8 +550,8 @@ int hfsplus_setxattr(struct inode *inode, const
> char *name,
>  	xattr_name = kmalloc(xattr_name_len, GFP_KERNEL);
>  	if (!xattr_name)
>  		return -ENOMEM;
> -	strcpy(xattr_name, prefix);
> -	strcpy(xattr_name + prefixlen, name);
> +	memcpy(xattr_name, prefix, prefixlen);

What's the point to mix memcpy and str*() family of methods? What's
wrong with str*() method here? Otherwise, if it is wrong to use str*()
family of methods, then why is it correct to use for second operation?

> +	strscpy(xattr_name + prefixlen, name, xattr_name_len -
> prefixlen);

Why strscpy() is better than strncpy()? What is the main argument here?

>  	res = __hfsplus_setxattr(inode, xattr_name, value, size,
> flags);
>  	kfree(xattr_name);
>  
> @@ -698,6 +698,7 @@ ssize_t hfsplus_getxattr(struct inode *inode,
> const char *name,
>  			 void *value, size_t size,
>  			 const char *prefix, size_t prefixlen)
>  {
> +	size_t xattr_name_len = NLS_MAX_CHARSET_SIZE *
> HFSPLUS_ATTR_MAX_STRLEN + 1;

Frankly speaking, it looks like a constant that should be declared in
hfs_common.h. Even if we would like to declare it here, then it should
be const size_t, from my point of view.

>  	int res;
>  	char *xattr_name;
>  
> @@ -705,13 +706,12 @@ ssize_t hfsplus_getxattr(struct inode *inode,
> const char *name,
>  		inode->i_ino, name ? name : NULL,
>  		prefix ? prefix : NULL);
>  
> -	xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE *
> HFSPLUS_ATTR_MAX_STRLEN + 1,
> -			     GFP_KERNEL);
> +	xattr_name = kmalloc(xattr_name_len, GFP_KERNEL);

Finally, I think kzalloc() should be much better for both cases.

Thanks,
Slava.

>  	if (!xattr_name)
>  		return -ENOMEM;
>  
> -	strcpy(xattr_name, prefix);
> -	strcpy(xattr_name + prefixlen, name);
> +	memcpy(xattr_name, prefix, prefixlen);
> +	strscpy(xattr_name + prefixlen, name, xattr_name_len -
> prefixlen);
>  
>  	res = __hfsplus_getxattr(inode, xattr_name, value, size);
>  	kfree(xattr_name);

Re: [PATCH next] fs/hfsplus/xattr: Use memcpy() and strscpy() to build xattr_name

[Darrick J. Wong]

On Wed, Jun 10, 2026 at 08:50:33PM -0700, Viacheslav Dubeyko wrote:
> On Mon, 2026-06-08 at 10:55 +0100, david.laight.linux@gmail.com wrote:
> > From: David Laight <david.laight.linux@gmail.com>
> > 
> > xattr_name is kmalloc()ed at the (assumed) maximal size and then the
> > prefix
> > and name concatenated together.
> > Use memcpy() for the prefix - its length is passed and strscpy() for
> > the
> > name to ensure it really doesnt overflow.
> > 
> > Prior to bf29e886b242c the buffers were smaller and on-stack.
> > (But I cant see the copy in the old code.)
> > I am also not sure why the buffer isnt created "just long enough".
> > 
> > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > ---
> > This is one of a group of patches that remove potentially unbounded
> > strcpy() calls.
> > 
> > They are mostly replaced by strscpy() or, when strlen() has just been
> > called, with memcpy() (usually including the '\0').
> > 
> > Calls with copy string literals into arrays are left unchanged.
> > They are safe and easily detected as such.
> > 
> > The changes were made by getting the compiler to detect the calls and
> > then fixing the code by hand.
> > 
> > Note that all the changes are only compile tested.
> > 
> > Some Makefiles were changed to allow files to contain strcpy().
> > As well as 'difficult to fix' files, this included 'show' functions
> > as they really need to use sysfs_emit() or seq_printf().
> > 
> > All the patches are being sent individually to avoid very long cc
> > lists.
> > Apologies for the terse commit messages and likely unexpected tags.
> > (There are about 100 patches in total.)
> > 
> >  fs/hfsplus/xattr.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> > index 452a1f9becb2..0b3dd48c28c9 100644
> > --- a/fs/hfsplus/xattr.c
> > +++ b/fs/hfsplus/xattr.c
> > @@ -550,8 +550,8 @@ int hfsplus_setxattr(struct inode *inode, const
> > char *name,
> >  	xattr_name = kmalloc(xattr_name_len, GFP_KERNEL);
> >  	if (!xattr_name)
> >  		return -ENOMEM;
> > -	strcpy(xattr_name, prefix);
> > -	strcpy(xattr_name + prefixlen, name);
> > +	memcpy(xattr_name, prefix, prefixlen);
> 
> What's the point to mix memcpy and str*() family of methods? What's
> wrong with str*() method here? Otherwise, if it is wrong to use str*()
> family of methods, then why is it correct to use for second operation?
> 
> > +	strscpy(xattr_name + prefixlen, name, xattr_name_len -
> > prefixlen);
> 
> Why strscpy() is better than strncpy()? What is the main argument here?
> 
> >  	res = __hfsplus_setxattr(inode, xattr_name, value, size,
> > flags);
> >  	kfree(xattr_name);
> >  
> > @@ -698,6 +698,7 @@ ssize_t hfsplus_getxattr(struct inode *inode,
> > const char *name,
> >  			 void *value, size_t size,
> >  			 const char *prefix, size_t prefixlen)
> >  {
> > +	size_t xattr_name_len = NLS_MAX_CHARSET_SIZE *
> > HFSPLUS_ATTR_MAX_STRLEN + 1;
> 
> Frankly speaking, it looks like a constant that should be declared in
> hfs_common.h. Even if we would like to declare it here, then it should
> be const size_t, from my point of view.
> 
> >  	int res;
> >  	char *xattr_name;
> >  
> > @@ -705,13 +706,12 @@ ssize_t hfsplus_getxattr(struct inode *inode,
> > const char *name,
> >  		inode->i_ino, name ? name : NULL,
> >  		prefix ? prefix : NULL);
> >  
> > -	xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE *
> > HFSPLUS_ATTR_MAX_STRLEN + 1,
> > -			     GFP_KERNEL);
> > +	xattr_name = kmalloc(xattr_name_len, GFP_KERNEL);
> 
> Finally, I think kzalloc() should be much better for both cases.

kasprintf()?

--D

> Thanks,
> Slava.
> 
> >  	if (!xattr_name)
> >  		return -ENOMEM;
> >  
> > -	strcpy(xattr_name, prefix);
> > -	strcpy(xattr_name + prefixlen, name);
> > +	memcpy(xattr_name, prefix, prefixlen);
> > +	strscpy(xattr_name + prefixlen, name, xattr_name_len -
> > prefixlen);
> >  
> >  	res = __hfsplus_getxattr(inode, xattr_name, value, size);
> >  	kfree(xattr_name);
> 

Re: [PATCH next] fs/hfsplus/xattr: Use memcpy() and strscpy() to build xattr_name

[David Laight]

On Wed, 10 Jun 2026 18:05:02 -0700
Viacheslav Dubeyko <slava@dubeyko.com> wrote:

> On Wed, 2026-06-10 at 10:09 +0100, David Laight wrote:
> > On Tue, 09 Jun 2026 18:04:46 -0700
> > Viacheslav Dubeyko <slava@dubeyko.com> wrote:
> >   
> > > On Mon, 2026-06-08 at 10:55 +0100,
> > > david.laight.linux@gmail.com wrote:  
> > > > From: David Laight <david.laight.linux@gmail.com>
> > > > 
> > > > xattr_name is kmalloc()ed at the (assumed) maximal size and then
> > > > the
> > > > prefix
> > > > and name concatenated together.
> > > > Use memcpy() for the prefix - its length is passed and strscpy()
> > > > for
> > > > the
> > > > name to ensure it really doesnt overflow.
> > > > 
> > > > Prior to bf29e886b242c the buffers were smaller and on-stack.
> > > > (But I cant see the copy in the old code.)
> > > > I am also not sure why the buffer isnt created "just long
> > > > enough".    
> > > 
> > > What do you mean by "the buffer isn't created just long enough"?
> > > And
> > > what is your vision of correct implementation of the logic?  
> > 
> > The old code is:  
> > >  	xattr_name = kmalloc(xattr_name_len, GFP_KERNEL);
> > >  	if (!xattr_name)
> > >  		return -ENOMEM;
> > > 	strcpy(xattr_name, prefix);
> > > 	strcpy(xattr_name + prefixlen, name);  
> > 
> > It would be more usual to do:
> > 	size_t name_len = strlen(name) + 1;
> > 	xattr_name = kmalloc(prefixlen + name_len);
> > 	memcpy(xattr_name, prefix, prefixlen)
> > 	memcpy(xattr_name + prefixlen, name, name_len);
> > So that the buffer is allocated the correct size for the strings.
> > 
> > (Not that it really makes much difference whether you do kmalloc(32)
> > or
> > kmalloc(1024) for a temporary buffer - both come of a per-cpu list.)  
> 
> The xattr name cannot contain more than 127 symbols [1]. So, the buffer
> of fixed size is allocated to keep any potential string inside of this
> limit no matter of size of the prefix and name. This is because we must
> have string that not longer that 127 symbols. If it is longer than
> that, then something is going wrong. Following to your logic, there is
> no big difference to allocate the buffer for 127 symbols or for precise
> length of the name. So, your suggestion doesn't make sense to me.

This code has no idea of the number of symbols.
Assuming that the callers haven't passed something that is overlong
is just asking for trouble - especially since it doesn't matter to
this code.
The caller is unlikely to know the length of the prefix - so may be
passing in a 'name' that is nearly 127 symbols long.
So adding the prefix can generate something longer than 127 symbols.
The called code has to handle/detect that.

> 
> > 
> > What I couldn't decide, but seems to be inferred from some of fixes,
> > was whether a double-terminator was needed.
> > (That might just be the attribute value.)
> > One of the related functions got fixed to zero fill a buffer in a
> > loop
> > because (AFICT) something was reading beyond a '\0' terminator.  
> 
> The rest is dedicated to Unicode peculiarities, as far as I can see.

Eh? a UTF-8 encoded buffer is terminated by a single '\0'.
The changes that stopped 'old values' being used on a later loop
iteration seems to be fixed by zeroing after the '\0', but neither
the code change nor commit message makes it clear what was fixed.

> 
> > 
> > This code does seem strange though.
> > It seems to add a text prefix here and then the called function does
> > string compares to find out which prefix has added and than act
> > differently based on the prefix.
> > I didn't try to follow things any further.  
> 
> There is no strange thing here. Different prefixes are processed by
> different logic.

So why do string compares for the prefixes?

> 
> > 
> > I also don't know if 'name' itself can be 127 wide characters (before
> > the
> > prefix is added)?
> > The fixed length buffer isn't long enough if all 127 characters
> > require
> > 6 bytes to encode.  
> 
> The whole string cannot be bigger than 127 symbols.

'cannot' or 'must not'?
As I said above where is the actual guarantee that the limit isn't exceeded.

	David

> 
> Thanks,
> Slava.
> 
> [1] https://elixir.bootlin.com/linux/v7.1-rc6/source/include/linux/hfs_common.h#L78

Re: [PATCH next] fs/hfsplus/xattr: Use memcpy() and strscpy() to build xattr_name

[David Laight]

On Wed, 10 Jun 2026 20:50:33 -0700
Viacheslav Dubeyko <slava@dubeyko.com> wrote:

> On Mon, 2026-06-08 at 10:55 +0100, david.laight.linux@gmail.com wrote:
> > From: David Laight <david.laight.linux@gmail.com>
> > 
> > xattr_name is kmalloc()ed at the (assumed) maximal size and then the
> > prefix
> > and name concatenated together.
> > Use memcpy() for the prefix - its length is passed and strscpy() for
> > the
> > name to ensure it really doesnt overflow.
> > 
> > Prior to bf29e886b242c the buffers were smaller and on-stack.
> > (But I cant see the copy in the old code.)
> > I am also not sure why the buffer isnt created "just long enough".
> > 
> > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > ---
> > This is one of a group of patches that remove potentially unbounded
> > strcpy() calls.
> > 
> > They are mostly replaced by strscpy() or, when strlen() has just been
> > called, with memcpy() (usually including the '\0').
> > 
> > Calls with copy string literals into arrays are left unchanged.
> > They are safe and easily detected as such.
> > 
> > The changes were made by getting the compiler to detect the calls and
> > then fixing the code by hand.
> > 
> > Note that all the changes are only compile tested.
> > 
> > Some Makefiles were changed to allow files to contain strcpy().
> > As well as 'difficult to fix' files, this included 'show' functions
> > as they really need to use sysfs_emit() or seq_printf().
> > 
> > All the patches are being sent individually to avoid very long cc
> > lists.
> > Apologies for the terse commit messages and likely unexpected tags.
> > (There are about 100 patches in total.)
> > 
> >  fs/hfsplus/xattr.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> > index 452a1f9becb2..0b3dd48c28c9 100644
> > --- a/fs/hfsplus/xattr.c
> > +++ b/fs/hfsplus/xattr.c
> > @@ -550,8 +550,8 @@ int hfsplus_setxattr(struct inode *inode, const
> > char *name,
> >  	xattr_name = kmalloc(xattr_name_len, GFP_KERNEL);
> >  	if (!xattr_name)
> >  		return -ENOMEM;
> > -	strcpy(xattr_name, prefix);
> > -	strcpy(xattr_name + prefixlen, name);
> > +	memcpy(xattr_name, prefix, prefixlen);  
> 
> What's the point to mix memcpy and str*() family of methods? What's
> wrong with str*() method here? Otherwise, if it is wrong to use str*()
> family of methods, then why is it correct to use for second operation?

They all just copy memory...
memcpy() copies a number of bytes,
strcpy() copies up to (and including) a zero byte.
strscpy() copies up to a zero byte, but no more than the specified length
	and always zero terminates the written data.

memcpy() is always going to be faster because it doesn't need to
look at the data being copied.

> 
> > +	strscpy(xattr_name + prefixlen, name, xattr_name_len -
> > prefixlen);  
> 
> Why strscpy() is better than strncpy()? What is the main argument here?

strncpy() is completely broken (but not as badly as strncat).

And, replying to the next email.
You really don't want to use kasprintf(), especially just to concatenate
two strings.

> 
> >  	res = __hfsplus_setxattr(inode, xattr_name, value, size,
> > flags);
> >  	kfree(xattr_name);
> >  
> > @@ -698,6 +698,7 @@ ssize_t hfsplus_getxattr(struct inode *inode,
> > const char *name,
> >  			 void *value, size_t size,
> >  			 const char *prefix, size_t prefixlen)
> >  {
> > +	size_t xattr_name_len = NLS_MAX_CHARSET_SIZE *
> > HFSPLUS_ATTR_MAX_STRLEN + 1;  
> 
> Frankly speaking, it looks like a constant that should be declared in
> hfs_common.h. Even if we would like to declare it here, then it should
> be const size_t, from my point of view.

There is little point marking variables as 'const'.

> 
> >  	int res;
> >  	char *xattr_name;
> >  
> > @@ -705,13 +706,12 @@ ssize_t hfsplus_getxattr(struct inode *inode,
> > const char *name,
> >  		inode->i_ino, name ? name : NULL,
> >  		prefix ? prefix : NULL);
> >  
> > -	xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE *
> > HFSPLUS_ATTR_MAX_STRLEN + 1,
> > -			     GFP_KERNEL);
> > +	xattr_name = kmalloc(xattr_name_len, GFP_KERNEL);  
> 
> Finally, I think kzalloc() should be much better for both cases.

No point taking the cost of zeroing large amounts of memory you
aren't going to access.

	David

> 
> Thanks,
> Slava.
> 
> >  	if (!xattr_name)
> >  		return -ENOMEM;
> >  
> > -	strcpy(xattr_name, prefix);
> > -	strcpy(xattr_name + prefixlen, name);
> > +	memcpy(xattr_name, prefix, prefixlen);
> > +	strscpy(xattr_name + prefixlen, name, xattr_name_len -
> > prefixlen);
> >  
> >  	res = __hfsplus_getxattr(inode, xattr_name, value, size);
> >  	kfree(xattr_name);