On Wed, 10 Jun 2026 20:50:33 -0700 Viacheslav Dubeyko <[email protected]> wrote:
> 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? 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. > > > + 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'. > > > 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. David > > 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);
