Re: [PATCH v2] hfsplus: fix truncated xattr names in listxattr()

[Viacheslav Dubeyko <slava@dubeyko.com>]

On Thu, 2026-10-22 at 07:17 -0400, Hengyu Liang wrote:
> From: Maurycy Pawlowski-Wieronski <maurycy@maurycy.com>
> 
> Commit 7dcbf17e3f91 ("hfsplus: refactor copy_name to not use
> strncpy")
> made copy_name() build names in the osx. namespace with scnprintf().
> 
> However, the size passed to scnprintf() leaves no room for the
> terminating NUL, so the last character of the name is dropped. As of
> now, listxattr() returns every osx. name without its last character,
> and these names cannot be passed back to getxattr().

Yeah, I see the point now. Thanks for clear explanation. The main 
point that scnprintf() expects buffer size that includes terminating
NUL too.

> 
> The issue can be reproduced with:
> 
>     truncate -s 64M hfs.img
>     mkfs.hfsplus hfs.img
>     mount -o loop hfs.img /mnt
>     touch /mnt/file
>     setfattr -n osx.test -v 1 /mnt/file
>     getfattr -m - /mnt/file
> 
> Without this patch, getfattr lists "osx.com.apple.FinderInf" and
> "osx.tes". With this patch, it lists "osx.com.apple.FinderInfo" and
> "osx.test".
> 
> This patch adds room for the NUL to the size passed to scnprintf().

Usually, "This patch" is not good idea to use. :) It will be better
another way: "Add room for the NUL to the size passed to scnprintf()".

> 
> Fixes: 7dcbf17e3f91 ("hfsplus: refactor copy_name to not use
> strncpy")
> Cc: stable@vger.kernel.org # v6.10+
> Signed-off-by: Maurycy Pawlowski-Wieronski <maurycy@maurycy.com>
> Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>
> ---
> Changes in v2:
> - Rewrite the commit message. No code change.
> - v1:
> https://lore.kernel.org/r/20260828-hfsplus-copy_name-fix-v1-1-75ed9f0f378c@maurycy.com
> 
> Maurycy, v1 got no follow-up, so I am resending it as Viacheslav
> asked
> in the thread below, with you kept as the author. Let me know if you
> would rather send it yourself.
> https://lore.kernel.org/r/dc187921249a3f6716b829624853e64c10321cd1.camel@dubeyko.com
> 
> Answers to the questions on v1:
> - generic/377 passes with and without this patch. It only uses user.
>   names, which go through the strscpy() branch. That branch already
>   passes name_len + 1. No xfstests test uses osx. names.
> - Why + 1 helps: the prefix and the name are name_len +
>   XATTR_MAC_OSX_PREFIX_LEN characters long, and scnprintf() writes at
>   most size - 1 characters before the NUL. Zeroing the buffer first
>   does not help, as the last character is never written.
> - Why the write stays in bounds: both callers check that the buffer
>   has room for name_len() bytes, which covers the prefix, the name
> and
>   the NUL, before calling copy_name(). This is also the number of
> bytes
>   copy_name() wrote before commit 7dcbf17e3f91.
> - All xfstests: see the full run below.
> 
> Tested in KVM with KASAN on a804c5742bff, without and with this
> patch:
> - The commands above, and a C test that compares listxattr() with the
>   names set on files and a directory: short, long (up to 127
>   characters) and non-ASCII osx. names, FinderInfo, and user. and
>   trusted. names. Without the patch, every osx. name loses its last
>   character, fewer bytes are copied than the size query returns, a
>   buffer one byte short does not get -ERANGE, and getxattr() fails on
>   the listed osx. names. With it, the names match, the copied length
>   equals the size query, and a buffer one byte short gets -ERANGE.
>   KASAN reported nothing with exact-size buffers.
> - generic/377 and generic/062: 5/5 passed on both kernels.
> - Full fstests (805 generic tests, 608 not run on both kernels): 188
>   passed and 9 failed without the patch, 187 and 10 with it. The only
>   difference is generic/617, which fails on both kernels most of the
>   time (19 of 20 reruns without the patch, 18 of 20 with it).
>   fsck.hfsplus -fn reported no errors.

The xfstests cannot help with testing osx. names. This is how I missed
the issue. It will be great to have a KUnit test(s) dedicated to
testing HFS+ xattr functionality.

> 
>  fs/hfsplus/xattr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index 71364e093fa7..470f090b2699 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -515,8 +515,8 @@ static ssize_t copy_name(char *buffer, const char
> *xattr_name, size_t name_len)
>  	memset(buffer, 0, name_len);
>  
>  	if (!is_known_namespace(xattr_name)) {
> -		len = scnprintf(buffer, name_len +
> XATTR_MAC_OSX_PREFIX_LEN,
> -				 "%s%s", XATTR_MAC_OSX_PREFIX,
> xattr_name);
> +		len = scnprintf(buffer, name_len +
> XATTR_MAC_OSX_PREFIX_LEN + 1,
> +				"%s%s", XATTR_MAC_OSX_PREFIX,
> xattr_name);
>  	} else {
>  		len = strscpy(buffer, xattr_name, name_len + 1);
>  		if (len < 0) {
> 
> base-commit: a804c5742bff224d4cb4d25dd383554d598be32b

Looks good.

Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Slava.