On Mon, 2026-06-08 at 10:55 +0100, [email protected] wrote:
> From: David Laight <[email protected]>
>
> 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 <[email protected]>
> ---
> 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);
