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

[David Laight <david.laight.linux@gmail.com>]

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.