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

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

[Hengyu Liang]

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().

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@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.

 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

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@cs.unc.edu> 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().
>
> 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@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.
>
>  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
>
>

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

[Viacheslav Dubeyko]

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.