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

[Maurycy Pawlowski-Wieronski]

I'm sorry for the late response. Fine with me! Everything looks good
to me. Thank you for keeping me as the author.


On Thu, Oct 22, 2026 at 1:17 PM Hengyu Liang <hengyul@xxxxxxxxxx> wrote:
>
> From: Maurycy Pawlowski-Wieronski <maurycy@xxxxxxxxxxx>
>
> 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().
>
> 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().
>
> Fixes: 7dcbf17e3f91 ("hfsplus: refactor copy_name to not use strncpy")
> Cc: stable@xxxxxxxxxxxxxxx # v6.10+
> Signed-off-by: Maurycy Pawlowski-Wieronski <maurycy@xxxxxxxxxxx>
> Signed-off-by: Hengyu Liang <hengyul@xxxxxxxxxx>
> ---
> Changes in v2:
> - Rewrite the commit message. No code change.
> - v1: https://lore.kernel.org/r/20260828-hfsplus-copy_name-fix-v1-1-75ed9f0f378c@xxxxxxxxxxx
>
> 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@xxxxxxxxxxx
>
> 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.
>
>  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
> --
> 2.53.0
>
>