On Thu, 11 Jun 2026 21:02:05 -0700 Viacheslav Dubeyko <[email protected]> wrote:
> On Thu, 2026-06-11 at 09:18 +0100, David Laight wrote: > > 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. > > 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. The code relies on prefixlen being the number of bytes. It is the size used for the kmalloc(). The current strcpy() (for the prefix) relies on the caller passing in the correct size that matches the length. Although the strings are Unicode it doesn't really matter here at all. They are UTF-8 encoded which means they can be processed as normal '\0' terminated C strings. The only difficulty is that the length of the C string can be much longer than the number of Unicode characters. Provided you make the use the lengths of the '\0' terminated strings everything is fine - which is what the change does. The existing code just hopes that no one manages to pass in a string that is too long for the buffer. Maybe it shouldn't happen - but any test is way earlier in the function call stack. If there is a bug somewhere you've got a heap overrun. You really do need to ensure that the copies use exactly the same lengths that were used for the kmalloc(). There is also the possibility that the strings can get changed by another thread (maybe you can prove it doesn't happen here, but it really is better to be safe) so you really need to only calculate the length once, strlen() shouldn't be followed by strcpy(). For absolute safety you need to use memcpy() and then write in the terminating '\0'. David > > > > > > > > > > + 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.
