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

[Viacheslav Dubeyko <slava@dubeyko.com>]

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,