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@xxxxxxxxx wrote:
> From: David Laight <david.laight.linux@xxxxxxxxx>
> 
> 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@xxxxxxxxx>
> ---
> 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);