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

["Darrick J. Wong" <djwong@kernel.org>]

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);
>