[PATCH] hfsplus: add missing osx prefix to FinderInfo attribute check

[PATCH] hfsplus: add missing osx prefix to FinderInfo attribute check

[Thomas Hebb]

By the time we reach strcmp_xattr_finder_info(), hfsplus_osx_getxattr()
has already prefixed the user-provided xattr name with "osx.", so the
existing check for the special FinderInfo attribute will always fail,
and ENODATA will be returned to userspace.

Since hfsplus_listxattr() reports the FinderInfo attribute, this
behavior causes certain userspace utilities (e.g. patch) to fail, since
the kernel lists an attribute that doesn't exist.

Cc: stable@vger.kernel.org
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Hin-Tak Leung <htl10@users.sourceforge.net>
Cc: Sergei Antonov <saproj@gmail.com>
Cc: Anton Altaparmakov <anton@tuxera.com>
Cc: Fabian Frederick <fabf@skynet.be>
Cc: Christian Kujau <lists@nerdbynature.de>
Signed-off-by: Thomas Hebb <tommyhebb@gmail.com>
---
 fs/hfsplus/xattr.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
index d98094a..da0664e 100644
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -28,8 +28,16 @@ const struct xattr_handler *hfsplus_xattr_handlers[] = {
  static int strcmp_xattr_finder_info(const char *name)
 {
+	int err = 0;
+
 	if (name) {
-		return strncmp(name, HFSPLUS_XATTR_FINDER_INFO_NAME,
+		err = strncmp(name, XATTR_MAC_OSX_PREFIX,
+				XATTR_MAC_OSX_PREFIX_LEN);
+		if (err)
+			return err;
+
+		return strncmp(name + XATTR_MAC_OSX_PREFIX_LEN,
+				HFSPLUS_XATTR_FINDER_INFO_NAME,
 				sizeof(HFSPLUS_XATTR_FINDER_INFO_NAME));
 	}
 	return -1;
-- 
2.3.4

Re: [PATCH] hfsplus: add missing osx prefix to FinderInfo attribute check

[Viacheslav Dubeyko]

On Tue, 2015-03-31 at 23:41 -0400, Thomas Hebb wrote:
> By the time we reach strcmp_xattr_finder_info(), hfsplus_osx_getxattr()
> has already prefixed the user-provided xattr name with "osx.", so the
> existing check for the special FinderInfo attribute will always fail,
> and ENODATA will be returned to userspace.
> 
> Since hfsplus_listxattr() reports the FinderInfo attribute, this
> behavior causes certain userspace utilities (e.g. patch) to fail, since
> the kernel lists an attribute that doesn't exist.
> 

Could you describe in more details how do you use userspace utilities
for extracting xattr? I suspect that you simply use it in incorrect way.

Thanks,
Vyacheslav Dubeyko.

> Cc: stable@vger.kernel.org
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Hin-Tak Leung <htl10@users.sourceforge.net>
> Cc: Sergei Antonov <saproj@gmail.com>
> Cc: Anton Altaparmakov <anton@tuxera.com>
> Cc: Fabian Frederick <fabf@skynet.be>
> Cc: Christian Kujau <lists@nerdbynature.de>
> Signed-off-by: Thomas Hebb <tommyhebb@gmail.com>
> ---
>  fs/hfsplus/xattr.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index d98094a..da0664e 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -28,8 +28,16 @@ const struct xattr_handler *hfsplus_xattr_handlers[] = {
>   static int strcmp_xattr_finder_info(const char *name)
>  {
> +	int err = 0;
> +
>  	if (name) {
> -		return strncmp(name, HFSPLUS_XATTR_FINDER_INFO_NAME,
> +		err = strncmp(name, XATTR_MAC_OSX_PREFIX,
> +				XATTR_MAC_OSX_PREFIX_LEN);
> +		if (err)
> +			return err;
> +
> +		return strncmp(name + XATTR_MAC_OSX_PREFIX_LEN,
> +				HFSPLUS_XATTR_FINDER_INFO_NAME,
>  				sizeof(HFSPLUS_XATTR_FINDER_INFO_NAME));
>  	}
>  	return -1;

Re: [PATCH] hfsplus: add missing osx prefix to FinderInfo attribute check

[Thomas Hebb]

> Could you describe in more details how do you use userspace utilities
> for extracting xattr?

Of course. I first create a new file on an HFS+ filesystem (case-
sensitive, journaling disabled). I then call listxattr() on that
file and receive a list of one attribute:
  osx.com.apple.FinderInfo
I call getxattr() with this name as the second argument.

Without the patch, getxattr() returns -1 and sets errno to either
ENOATTR (ENODATA) if the volume has an attribute tree or EOPNOTSUPP if
it doesn't. This behavior is clearly wrong, since listxattr() told me
that the attribute existed.

In the case where ENOATTR is returned (when an attribute b-tree exists,
as it does when any file on the filesystem has a real attribute), the
error causes GNU patch 2.7.5 to fail with the error message
  getting attribute osx.com.apple.FinderInfo of osx.com.apple.FinderInfo: No data available
(As shown in the attached typescript.) That's how I originally discovered
the problem.

I have written a test program to demonstrate the issue. It builds on
both Linux and OS X, so that the behaviors can be compared. You can
find it at https://github.com/tchebb/xattr-test.

After more investigation, however, I must withdraw the initial patch I
sent. It is not a correct solution to the entire problem; it's simply a
band-aid that fixes this one particular symptom. As you can see from
the included typescript, *any* osx-namespaced attribute is given an
additional copy of the prefix. I believe I've found a commit from 2014
that introduced this incorrect behavior, and I'll send another patch
soon that should resolve the issue entirely.

Thanks,
-Thomas Hebb

typescript
---
$ cat >a				# Make some test files
line 1
line 2
$ cat >b
line 1
line 2
line 3
$ diff -u a b | tee patch		# Create a diff between them
--- a	2015-04-02 17:03:18.000000000 -0400
+++ b	2015-04-02 17:03:30.000000000 -0400
@@ -1,2 +1,3 @@
 line 1
 line 2
+line 3
$ patch -i patch a			# Patch the file. Works fine, for now...
patching file a
$ ./xattr-test a			# But getxattr() still returns EOPNOTSUPP
Listing attributes of "a"
listxattr() returned 25
  List contents:
    osx.com.apple.FinderInfo

Getting attribute "osx.com.apple.FinderInfo"
getxattr() returned -1
getxattr() failed: Operation not supported

$ cat >a				# Let's try again
line 1
line 2
$ setfattr -n 'user.test' -v 'abc' a	# This time, give the volume an attr tree
$ patch -i patch a			# And patch fails!
patching file a
patch: getting attribute osx.com.apple.FinderInfo of osx.com.apple.FinderInfo: No data available
$ ./xattr-test a			# Because getxattr() returns ENODATA
Listing attributes of "a"
listxattr() returned 35
  List contents:
    osx.com.apple.FinderInfo
    user.test

Getting attribute "osx.com.apple.FinderInfo"
getxattr() returned -1
getxattr() failed: No data available

Getting attribute "user.test"
getxattr() returned 3
  Attribute value:
    0x61 0x62 0x63

$ setfattr -n 'osx.test' -v 'def' a	# Now, what if we create an attr in the osx namespace?
$ ./xattr-test a			# Aha, here's the real problem: "osx." is prefixed an extra time!
Listing attributes of "a"
listxattr() returned 48
  List contents:
    osx.com.apple.FinderInfo
    osx.osx.test
    user.test

Getting attribute "osx.com.apple.FinderInfo"
getxattr() returned -1
getxattr() failed: No data available

Getting attribute "osx.osx.test"
getxattr() returned -1
getxattr() failed: No data available

Getting attribute "user.test"
getxattr() returned 3
  Attribute value:
    0x61 0x62 0x63

$ exit

Re: [PATCH] hfsplus: add missing osx prefix to FinderInfo attribute check

[Viacheslav Dubeyko]

On Thu, 2015-04-02 at 19:57 -0400, Thomas Hebb wrote:
> > Could you describe in more details how do you use userspace utilities
> > for extracting xattr?
> 
> Of course. I first create a new file on an HFS+ filesystem (case-
> sensitive, journaling disabled). I then call listxattr() on that
> file and receive a list of one attribute:
>   osx.com.apple.FinderInfo
> I call getxattr() with this name as the second argument.
> 
> Without the patch, getxattr() returns -1 and sets errno to either
> ENOATTR (ENODATA) if the volume has an attribute tree or EOPNOTSUPP if
> it doesn't. This behavior is clearly wrong, since listxattr() told me
> that the attribute existed.
> 
> In the case where ENOATTR is returned (when an attribute b-tree exists,
> as it does when any file on the filesystem has a real attribute), the
> error causes GNU patch 2.7.5 to fail with the error message
>   getting attribute osx.com.apple.FinderInfo of osx.com.apple.FinderInfo: No data available
> (As shown in the attached typescript.) That's how I originally discovered
> the problem.
> 
> I have written a test program to demonstrate the issue. It builds on
> both Linux and OS X, so that the behaviors can be compared. You can
> find it at https://github.com/tchebb/xattr-test.

OK. Thank you. I need to check that xattr support doesn't work for HFS+
now. It worked pretty well earlier.

Thanks,
Vyacheslav Dubeyko.

Re: [PATCH] hfsplus: add missing osx prefix to FinderInfo attribute check

[Sergei Antonov]

On 1 April 2015 at 05:41, Thomas Hebb <tommyhebb@gmail.com> wrote:
> By the time we reach strcmp_xattr_finder_info(), hfsplus_osx_getxattr()
> has already prefixed the user-provided xattr name with "osx.", so the
> existing check for the special FinderInfo attribute will always fail,
> and ENODATA will be returned to userspace.
>
> Since hfsplus_listxattr() reports the FinderInfo attribute, this
> behavior causes certain userspace utilities (e.g. patch) to fail, since
> the kernel lists an attribute that doesn't exist.
>
> Cc: stable@vger.kernel.org
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Hin-Tak Leung <htl10@users.sourceforge.net>
> Cc: Sergei Antonov <saproj@gmail.com>
> Cc: Anton Altaparmakov <anton@tuxera.com>
> Cc: Fabian Frederick <fabf@skynet.be>
> Cc: Christian Kujau <lists@nerdbynature.de>
> Signed-off-by: Thomas Hebb <tommyhebb@gmail.com>
> ---
>  fs/hfsplus/xattr.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index d98094a..da0664e 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -28,8 +28,16 @@ const struct xattr_handler *hfsplus_xattr_handlers[] = {
>   static int strcmp_xattr_finder_info(const char *name)
>  {
> +       int err = 0;
> +
>         if (name) {
> -               return strncmp(name, HFSPLUS_XATTR_FINDER_INFO_NAME,
> +               err = strncmp(name, XATTR_MAC_OSX_PREFIX,
> +                               XATTR_MAC_OSX_PREFIX_LEN);
> +               if (err)
> +                       return err;
> +
> +               return strncmp(name + XATTR_MAC_OSX_PREFIX_LEN,
> +                               HFSPLUS_XATTR_FINDER_INFO_NAME,
>                                 sizeof(HFSPLUS_XATTR_FINDER_INFO_NAME));
>         }
>         return -1;
> --
> 2.3.4
>

Your patch does fix a bug (reproduced through
listxattr/getxattr/setxattr apis). But I'd recommend a more elegant
code:

--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -29,7 +29,9 @@ const struct xattr_handler *hfsplus_xattr_handlers[] = {
 static int strcmp_xattr_finder_info(const char *name)
 {
     if (name) {
-        return strncmp(name, HFSPLUS_XATTR_FINDER_INFO_NAME,
+        return strncmp(name, XATTR_MAC_OSX_PREFIX
+                HFSPLUS_XATTR_FINDER_INFO_NAME,
+                XATTR_MAC_OSX_PREFIX_LEN +
                 sizeof(HFSPLUS_XATTR_FINDER_INFO_NAME));
     }
     return -1;


If you resubmit it, make it Reviewed-by: Sergei Antonov <saproj@gmail.com>

Re: [PATCH] hfsplus: add missing osx prefix to FinderInfo attribute check

[Viacheslav Dubeyko]

On Fri, 2015-04-03 at 01:41 +0200, Sergei Antonov wrote:

> Your patch does fix a bug (reproduced through
> listxattr/getxattr/setxattr apis). But I'd recommend a more elegant
> code:
> 
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -29,7 +29,9 @@ const struct xattr_handler *hfsplus_xattr_handlers[] = {
>  static int strcmp_xattr_finder_info(const char *name)
>  {
>      if (name) {
> -        return strncmp(name, HFSPLUS_XATTR_FINDER_INFO_NAME,
> +        return strncmp(name, XATTR_MAC_OSX_PREFIX
> +                HFSPLUS_XATTR_FINDER_INFO_NAME,
> +                XATTR_MAC_OSX_PREFIX_LEN +
>                  sizeof(HFSPLUS_XATTR_FINDER_INFO_NAME));
>      }
>      return -1;
> 
> 
> If you resubmit it, make it Reviewed-by: Sergei Antonov <saproj@gmail.com>

The issue is located in hfsplus_osx_getxattr() method:

{
        char *xattr_name;
        int res;

        if (!strcmp(name, ""))
                return -EINVAL;

        /*
         * Don't allow retrieving properly prefixed attributes
         * by prepending them with "osx."
         */
        if (is_known_namespace(name))
                return -EOPNOTSUPP;
        xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE *
HFSPLUS_ATTR_MAX_STRLEN
                + XATTR_MAC_OSX_PREFIX_LEN + 1, GFP_KERNEL);
        if (!xattr_name)
                return -ENOMEM;

************************************************************
        strcpy(xattr_name, XATTR_MAC_OSX_PREFIX);
        strcpy(xattr_name + XATTR_MAC_OSX_PREFIX_LEN, name);
*************************************************************

This part adds "osx." prefix second time. So, it needs to fix the issue
here.

Thanks,
Vyacheslav Dubeyko.