[PATCH] hfsplus: don't store special "osx" xattr prefix on-disk

[PATCH] hfsplus: don't store special "osx" xattr prefix on-disk

[Thomas Hebb]

On Mac OS X, HFS+ extended attributes are not namespaced. Since we want
to be compatible with OS X filesystems and yet still support the Linux
namespacing system, the hfsplus driver implements a special "osx"
namespace that is reported for any attribute that is not namespaced
on-disk. However, the current code for getting and setting these
unprefixed attributes is broken.

hfsplus_osx_setattr() and hfsplus_osx_getattr() are passed names that
have already had their "osx." prefixes stripped by the generic functions.
The functions first, quite correctly, check those names to make sure
that they aren't prefixed with a known namespace, which would allow
namespace access restrictions to be bypassed. However, the functions
then prepend "osx." to the name they're given before passing it on to
hfsplus_getattr() and hfsplus_setattr(). Not only does this cause the
"osx." prefix to be stored on-disk, defeating its purpose, it also breaks
the check for the special "com.apple.FinderInfo" attribute, which is
reported for all files, and as a consequence makes some userspace
applications (e.g. GNU patch) fail even when extended attributes are not
otherwise in use.

There are three commits which have touched this particular code:

  127e5f5ae51e ("hfsplus: rework functionality of getting, setting and deleting of extended attributes")
  b168fff72109 ("hfsplus: use xattr handlers for removexattr")
  bf29e886b242 ("hfsplus: correct usage of HFSPLUS_ATTR_MAX_STRLEN for non-English attributes")

The first commit creates the functions to begin with. The namespace is
prepended by the original code, which I believe was correct at the time,
since hfsplus_?etattr() stripped the prefix if found. The second commit
removed this behavior from hfsplus_?etattr() and appears to have been
intended to also remove the prefixing from hfsplus_osx_?etattr(). However,
what it actually did was remove a necessary strncpy() call completely,
breaking the osx namespace entirely. The third and final commit re-added
the strncpy() call as it was originally (but didn't mention it in its
commit message).

This commit does what b168fff attempted to do (prevent the prefix from
being added), but does it properly, instead of passing in an empty buffer
(which is what b168fff actually did).

This is the original version of the commit, which applies to 3.16 through
4.0. Mainline commit db579e76f06e78de011b2cb7e028740a82f5558c is rebased
on the newly-refactored *getxattr() and *setxattr() functions in 4.1.

Fixes: b168fff72109 ("hfsplus: use xattr handlers for removexattr")
Cc: stable@vger.kernel.org
Signed-off-by: Thomas Hebb <tommyhebb@gmail.com>
---
 fs/hfsplus/xattr.c | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
index d98094a..ff10f3d 100644
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -806,9 +806,6 @@ end_removexattr:
 static int hfsplus_osx_getxattr(struct dentry *dentry, const char *name,
 					void *buffer, size_t size, int type)
 {
-	char *xattr_name;
-	int res;
-
 	if (!strcmp(name, ""))
 		return -EINVAL;
 
@@ -818,24 +815,19 @@ static int hfsplus_osx_getxattr(struct dentry *dentry, const char *name,
 	 */
 	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);
 
-	res = hfsplus_getxattr(dentry, xattr_name, buffer, size);
-	kfree(xattr_name);
-	return res;
+	/*
+	 * osx is the namespace we use to indicate an unprefixed
+	 * attribute on the filesystem (like the ones that OS X
+	 * creates), so we pass the name through unmodified (after
+	 * ensuring it doesn't conflict with another namespace).
+	 */
+	return hfsplus_getxattr(dentry, name, buffer, size);
 }
 
 static int hfsplus_osx_setxattr(struct dentry *dentry, const char *name,
 		const void *buffer, size_t size, int flags, int type)
 {
-	char *xattr_name;
-	int res;
-
 	if (!strcmp(name, ""))
 		return -EINVAL;
 
@@ -845,16 +837,14 @@ static int hfsplus_osx_setxattr(struct dentry *dentry, const char *name,
 	 */
 	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);
 
-	res = hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
-	kfree(xattr_name);
-	return res;
+	/*
+	 * osx is the namespace we use to indicate an unprefixed
+	 * attribute on the filesystem (like the ones that OS X
+	 * creates), so we pass the name through unmodified (after
+	 * ensuring it doesn't conflict with another namespace).
+	 */
+	return hfsplus_setxattr(dentry, name, buffer, size, flags);
 }
 
 static size_t hfsplus_osx_listxattr(struct dentry *dentry, char *list,
-- 
2.3.4

[PATCH] hfsplus: don't store special "osx" xattr prefix on-disk

[Thomas Hebb]

On Mac OS X, HFS+ extended attributes are not namespaced. Since we want
to be compatible with OS X filesystems and yet still support the Linux
namespacing system, the hfsplus driver implements a special "osx"
namespace that is reported for any attribute that is not namespaced
on-disk. However, the current code for getting and setting these
unprefixed attributes is broken.

hfsplus_osx_setattr() and hfsplus_osx_getattr() are passed names that
have already had their "osx." prefixes stripped by the generic functions.
The functions first, quite correctly, check those names to make sure
that they aren't prefixed with a known namespace, which would allow
namespace access restrictions to be bypassed. However, the functions
then prepend "osx." to the name they're given before passing it on to
hfsplus_getattr() and hfsplus_setattr(). Not only does this cause the
"osx." prefix to be stored on-disk, defeating its purpose, it also breaks
the check for the special "com.apple.FinderInfo" attribute, which is
reported for all files, and--as a consequence--makes some userspace
applications (e.g. GNU patch) fail even when extended attributes are not
otherwise in use.

There are three commits which have touched this particular code:

  127e5f5ae51e ("hfsplus: rework functionality of getting, setting and deleting of extended attributes")
  b168fff72109 ("hfsplus: use xattr handlers for removexattr")
  bf29e886b242 ("hfsplus: correct usage of HFSPLUS_ATTR_MAX_STRLEN for non-English attributes")

The first commit creates the functions to begin with. The namespace is
prepended by the original code, which I believe was correct at the time,
since hfsplus_?etattr() stripped the prefix if found. The second commit
removed this behavior from hfsplus_?etattr() and appears to have been
intended to also remove the prefixing from hfsplus_osx_?etattr(). However,
what it actually did was remove a necessary strncpy() call, breaking the
osx namespace entirely. The third and final commit re-added the strncpy()
call as it was originally (but didn't mention it in its commit message).

This commit removes the strncpy() call, as b168fff did, but also fixes
the calls to hfsplus_?etattr() to directly pass in the name received
from userspace rather than an empty buffer, which is what b168fff did.

Fixes: b168fff72109 ("hfsplus: use xattr handlers for removexattr")
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 | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
index d98094a..ff10f3d 100644
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -806,9 +806,6 @@ end_removexattr:
 static int hfsplus_osx_getxattr(struct dentry *dentry, const char *name,
 					void *buffer, size_t size, int type)
 {
-	char *xattr_name;
-	int res;
-
 	if (!strcmp(name, ""))
 		return -EINVAL;
 
@@ -818,24 +815,19 @@ static int hfsplus_osx_getxattr(struct dentry *dentry, const char *name,
 	 */
 	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);
 
-	res = hfsplus_getxattr(dentry, xattr_name, buffer, size);
-	kfree(xattr_name);
-	return res;
+	/*
+	 * osx is the namespace we use to indicate an unprefixed
+	 * attribute on the filesystem (like the ones that OS X
+	 * creates), so we pass the name through unmodified (after
+	 * ensuring it doesn't conflict with another namespace).
+	 */
+	return hfsplus_getxattr(dentry, name, buffer, size);
 }
 
 static int hfsplus_osx_setxattr(struct dentry *dentry, const char *name,
 		const void *buffer, size_t size, int flags, int type)
 {
-	char *xattr_name;
-	int res;
-
 	if (!strcmp(name, ""))
 		return -EINVAL;
 
@@ -845,16 +837,14 @@ static int hfsplus_osx_setxattr(struct dentry *dentry, const char *name,
 	 */
 	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);
 
-	res = hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
-	kfree(xattr_name);
-	return res;
+	/*
+	 * osx is the namespace we use to indicate an unprefixed
+	 * attribute on the filesystem (like the ones that OS X
+	 * creates), so we pass the name through unmodified (after
+	 * ensuring it doesn't conflict with another namespace).
+	 */
+	return hfsplus_setxattr(dentry, name, buffer, size, flags);
 }
 
 static size_t hfsplus_osx_listxattr(struct dentry *dentry, char *list,
-- 
2.3.4

Re: [PATCH] hfsplus: don't store special "osx" xattr prefix on-disk

[Viacheslav Dubeyko]

On Thu, 2015-04-02 at 23:03 -0400, Thomas Hebb wrote:
> On Mac OS X, HFS+ extended attributes are not namespaced. Since we want
> to be compatible with OS X filesystems and yet still support the Linux
> namespacing system, the hfsplus driver implements a special "osx"
> namespace that is reported for any attribute that is not namespaced
> on-disk. However, the current code for getting and setting these
> unprefixed attributes is broken.
> 
> hfsplus_osx_setattr() and hfsplus_osx_getattr() are passed names that
> have already had their "osx." prefixes stripped by the generic functions.
> The functions first, quite correctly, check those names to make sure
> that they aren't prefixed with a known namespace, which would allow
> namespace access restrictions to be bypassed. However, the functions
> then prepend "osx." to the name they're given before passing it on to
> hfsplus_getattr() and hfsplus_setattr(). Not only does this cause the
> "osx." prefix to be stored on-disk, defeating its purpose, it also breaks
> the check for the special "com.apple.FinderInfo" attribute, which is
> reported for all files, and--as a consequence--makes some userspace
> applications (e.g. GNU patch) fail even when extended attributes are not
> otherwise in use.
> 
> There are three commits which have touched this particular code:
> 
>   127e5f5ae51e ("hfsplus: rework functionality of getting, setting and deleting of extended attributes")
>   b168fff72109 ("hfsplus: use xattr handlers for removexattr")
>   bf29e886b242 ("hfsplus: correct usage of HFSPLUS_ATTR_MAX_STRLEN for non-English attributes")
> 
> The first commit creates the functions to begin with. The namespace is
> prepended by the original code, which I believe was correct at the time,
> since hfsplus_?etattr() stripped the prefix if found. The second commit
> removed this behavior from hfsplus_?etattr() and appears to have been
> intended to also remove the prefixing from hfsplus_osx_?etattr(). However,
> what it actually did was remove a necessary strncpy() call, breaking the
> osx namespace entirely. The third and final commit re-added the strncpy()
> call as it was originally (but didn't mention it in its commit message).
> 
> This commit removes the strncpy() call, as b168fff did, but also fixes
> the calls to hfsplus_?etattr() to directly pass in the name received
> from userspace rather than an empty buffer, which is what b168fff did.
> 
> Fixes: b168fff72109 ("hfsplus: use xattr handlers for removexattr")
> 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>

Looks good for me.

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

Thanks,
Vyacheslav Dubeyko.


> ---
>  fs/hfsplus/xattr.c | 38 ++++++++++++++------------------------
>  1 file changed, 14 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index d98094a..ff10f3d 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -806,9 +806,6 @@ end_removexattr:
>  static int hfsplus_osx_getxattr(struct dentry *dentry, const char *name,
>  					void *buffer, size_t size, int type)
>  {
> -	char *xattr_name;
> -	int res;
> -
>  	if (!strcmp(name, ""))
>  		return -EINVAL;
>  
> @@ -818,24 +815,19 @@ static int hfsplus_osx_getxattr(struct dentry *dentry, const char *name,
>  	 */
>  	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);
>  
> -	res = hfsplus_getxattr(dentry, xattr_name, buffer, size);
> -	kfree(xattr_name);
> -	return res;
> +	/*
> +	 * osx is the namespace we use to indicate an unprefixed
> +	 * attribute on the filesystem (like the ones that OS X
> +	 * creates), so we pass the name through unmodified (after
> +	 * ensuring it doesn't conflict with another namespace).
> +	 */
> +	return hfsplus_getxattr(dentry, name, buffer, size);
>  }
>  
>  static int hfsplus_osx_setxattr(struct dentry *dentry, const char *name,
>  		const void *buffer, size_t size, int flags, int type)
>  {
> -	char *xattr_name;
> -	int res;
> -
>  	if (!strcmp(name, ""))
>  		return -EINVAL;
>  
> @@ -845,16 +837,14 @@ static int hfsplus_osx_setxattr(struct dentry *dentry, const char *name,
>  	 */
>  	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);
>  
> -	res = hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
> -	kfree(xattr_name);
> -	return res;
> +	/*
> +	 * osx is the namespace we use to indicate an unprefixed
> +	 * attribute on the filesystem (like the ones that OS X
> +	 * creates), so we pass the name through unmodified (after
> +	 * ensuring it doesn't conflict with another namespace).
> +	 */
> +	return hfsplus_setxattr(dentry, name, buffer, size, flags);
>  }
>  
>  static size_t hfsplus_osx_listxattr(struct dentry *dentry, char *list,

Re: [PATCH] hfsplus: don't store special "osx" xattr prefix on-disk

[Andrew Morton]

On Thu, 02 Apr 2015 23:03:50 -0400 Thomas Hebb <tommyhebb@gmail.com> wrote:

> On Mac OS X, HFS+ extended attributes are not namespaced. Since we want
> to be compatible with OS X filesystems and yet still support the Linux
> namespacing system, the hfsplus driver implements a special "osx"
> namespace that is reported for any attribute that is not namespaced
> on-disk. However, the current code for getting and setting these
> unprefixed attributes is broken.

This patch has quite bad collisions with pending hfsplus changes in
linux-next.  Specifically
fs-hfsplus-move-xattr_name-allocation-in-hfsplus_setxattr.patch and
fs-hfsplus-move-xattr_name-allocation-in-hfsplus_getxattr.patch.

Would it be possible for you to redo the patch against linux-next?

Sorry, this doesn't happen often.

THanks.