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

[Viacheslav Dubeyko <slava@dubeyko.com>]

On Thu, 2026-06-11 at 09:18 +0100, David Laight wrote:
> 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.

You need to take into account that it is Unicode based symbols. I
dislike to use memcpy() because prefixlen is only number of symbols but
not size in bytes.

Frankly speaking, I dislike your approach in this patch. It is not safe
enough.

> 
> > 
> > > +	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'.

This is why I am talking about declaration in hfs_common.h.

> 
> > 
> > >  	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.

What do you mean by huge? It's only 127 symbol in maximum. It's pretty
nothing.

Thanks,
Slava.