[PATCH 0/3] hfsplus: fix series for non-english names and attributes

[PATCH 0/3] hfsplus: fix series for non-english names and attributes

[Hin-Tak Leung]

From: Hin-Tak Leung <htl10@users.sourceforge.net>

This is a series of 3 patches which corrects issues in HFS+
concerning the use of non-english file names and attributes.
Names and attributes are stored internally as UTF-16 units up to a
fixed maximum size, and convert to and from user-representation by NLS.
The code incorrectly assume that NLS string lengths are equal
to unicode lengths, which is only true for English ascii usage.

This is largely a repost (with some editing/formatting to the messages) of the
earlier 4-patch series, as the original 4th patch
"hfsplus: fixes error propagation of hfsplus_asc2uni" is no longer needed.
The 4th patch is functionally identical to "hfsplus: fix longname handling"
from Sougata Santra posted a few weeks ago.

Patch 1 differs from the single patch sent to Andrew Morton by one
kzalloc to kmalloc change, as the kzalloc is not needed on examination.

The others only differ from the previous series of 4 posted to fs-devel
by editing/formatting and additions of CC's of the commit messages
- there is no code change.

Hin-Tak Leung (3):
  hfsplus: fixes worst-case unicode to char conversion of file names and
    attributes
  hfsplus: correct usage of HFSPLUS_ATTR_MAX_STRLEN for non-English
    attributes
  hfsplus: remove unused routine hfsplus_attr_build_key_uni

 fs/hfsplus/attributes.c     | 36 ++++----------------------------
 fs/hfsplus/dir.c            | 11 ++++++++--
 fs/hfsplus/hfsplus_fs.h     |  3 ---
 fs/hfsplus/xattr.c          | 50 ++++++++++++++++++++++++++++++---------------
 fs/hfsplus/xattr_security.c | 47 +++++++++++++++++++++++-------------------
 fs/hfsplus/xattr_trusted.c  | 30 ++++++++++++++++-----------
 fs/hfsplus/xattr_user.c     | 30 ++++++++++++++++-----------
 7 files changed, 108 insertions(+), 99 deletions(-)

-- 
1.9.0

[PATCH 1/3] hfsplus: fixes worst-case unicode to char conversion of file names and attributes

[Hin-Tak Leung]

From: Hin-Tak Leung <htl10@users.sourceforge.net>

The HFS Plus Volume Format specification (TN1150) states that
file names are stored internally as a maximum of 255 unicode
characters, as defined by The Unicode Standard, Version 2.0
[Unicode, Inc. ISBN 0-201-48345-9]. File names are converted by
the NLS system on Linux before presented to the user.

255 CJK characters converts to UTF-8 with 1 unicode character
to up to 3 bytes, and to GB18030 with 1 unicode character to
up to 4 bytes. Thus, trying in a UTF-8 locale to list files
with names of more than 85 CJK characters results in:

    $ ls /mnt
    ls: reading directory /mnt: File name too long

The receiving buffer to hfsplus_uni2asc() needs to be 255 x
NLS_MAX_CHARSET_SIZE bytes, not 255 bytes as the code has always been.

Similar consideration applies to attributes, which are stored
internally as a maximum of 127 UTF-16BE units. See XNU source for
an up-to-date reference on attributes.

Strictly speaking, the maximum value of NLS_MAX_CHARSET_SIZE = 6
is not attainable in the case of conversion to UTF-8, as going
beyond 3 bytes requires the use of surrogate pairs, i.e. consuming
two input units.

Thanks Anton Altaparmakov for reviewing an earlier version of this
change.

This patch fixes all callers of hfsplus_uni2asc(), and also enables
the use of long non-English file names in HFS+. The getting and
setting, and general usage of long non-English attributes
requires further forthcoming work, in the following patches of this
series.

Signed-off-by: Hin-Tak Leung <htl10@users.sourceforge.net>
Reviewed-by: Anton Altaparmakov <anton@tuxera.com>
CC: Vyacheslav Dubeyko <slava@dubeyko.com>
CC: Al Viro <viro@zeniv.linux.org.uk>
CC: Christoph Hellwig <hch@infradead.org>
CC: Sougata Santra <sougata@tuxera.com>
---
 fs/hfsplus/dir.c   | 11 +++++++++--
 fs/hfsplus/xattr.c | 14 +++++++++++---
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index bdec665..fb07d26 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -12,6 +12,7 @@
 #include <linux/fs.h>
 #include <linux/slab.h>
 #include <linux/random.h>
+#include <linux/nls.h>
 
 #include "hfsplus_fs.h"
 #include "hfsplus_raw.h"
@@ -127,7 +128,7 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
 	struct inode *inode = file_inode(file);
 	struct super_block *sb = inode->i_sb;
 	int len, err;
-	char strbuf[HFSPLUS_MAX_STRLEN + 1];
+	char *strbuf;
 	hfsplus_cat_entry entry;
 	struct hfs_find_data fd;
 	struct hfsplus_readdir_data *rd;
@@ -139,6 +140,11 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
 	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
 	if (err)
 		return err;
+	strbuf = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_MAX_STRLEN + 1, GFP_KERNEL);
+	if (!strbuf) {
+		err = -ENOMEM;
+		goto out;
+	}
 	hfsplus_cat_build_key(sb, fd.search_key, inode->i_ino, NULL);
 	err = hfs_brec_find(&fd, hfs_find_rec_by_key);
 	if (err)
@@ -193,7 +199,7 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
 		hfs_bnode_read(fd.bnode, &entry, fd.entryoffset,
 			fd.entrylength);
 		type = be16_to_cpu(entry.type);
-		len = HFSPLUS_MAX_STRLEN;
+		len = NLS_MAX_CHARSET_SIZE * HFSPLUS_MAX_STRLEN;
 		err = hfsplus_uni2asc(sb, &fd.key->cat.name, strbuf, &len);
 		if (err)
 			goto out;
@@ -246,6 +252,7 @@ next:
 	}
 	memcpy(&rd->key, fd.key, sizeof(struct hfsplus_cat_key));
 out:
+	kfree(strbuf);
 	hfs_find_exit(&fd);
 	return err;
 }
diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
index 4e27edc..40c0a63 100644
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -8,6 +8,7 @@
 
 #include "hfsplus_fs.h"
 #include <linux/posix_acl_xattr.h>
+#include <linux/nls.h>
 #include "xattr.h"
 #include "acl.h"
 
@@ -645,8 +646,7 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
 	struct hfs_find_data fd;
 	u16 key_len = 0;
 	struct hfsplus_attr_key attr_key;
-	char strbuf[HFSPLUS_ATTR_MAX_STRLEN +
-			XATTR_MAC_OSX_PREFIX_LEN + 1] = {0};
+	char *strbuf;
 	int xattr_name_len;
 
 	if ((!S_ISREG(inode->i_mode) &&
@@ -666,6 +666,13 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
 		return err;
 	}
 
+	strbuf = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN +
+			XATTR_MAC_OSX_PREFIX_LEN + 1, GFP_KERNEL);
+	if (!strbuf) {
+		res = -ENOMEM;
+		goto out;
+	}
+
 	err = hfsplus_find_attr(inode->i_sb, inode->i_ino, NULL, &fd);
 	if (err) {
 		if (err == -ENOENT) {
@@ -692,7 +699,7 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
 		if (be32_to_cpu(attr_key.cnid) != inode->i_ino)
 			goto end_listxattr;
 
-		xattr_name_len = HFSPLUS_ATTR_MAX_STRLEN;
+		xattr_name_len = NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN;
 		if (hfsplus_uni2asc(inode->i_sb,
 			(const struct hfsplus_unistr *)&fd.key->attr.key_name,
 					strbuf, &xattr_name_len)) {
@@ -718,6 +725,7 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
 	}
 
 end_listxattr:
+	kfree(strbuf);
 	hfs_find_exit(&fd);
 	return res;
 }
-- 
1.9.0

[PATCH 2/3] hfsplus: correct usage of HFSPLUS_ATTR_MAX_STRLEN for non-English attributes

[Hin-Tak Leung]

From: Hin-Tak Leung <htl10@users.sourceforge.net>

HFSPLUS_ATTR_MAX_STRLEN (=127) is the limit of attribute names for
the number of unicode character (UTF-16BE) storable in the HFS+ file
system. Almost all the current usage of it is wrong, in relation to
NLS to on-disk conversion.

Except for one use calling hfsplus_asc2uni (which should stay the
same) and its uses in calling hfsplus_uni2asc (which was corrected in
the earlier patch in this series concerning usage of hfsplus_uni2asc),
all the other uses are of the forms:

- char buffer[size]

- bound check: "if (namespace_adjusted_input_length > size) return failure;"

Conversion between on-disk unicode representation and NLS char strings
(in whichever direction) always needs to accommodate the worst-case NLS
conversion, so all char buffers of that size need to have a
NLS_MAX_CHARSET_SIZE x .

The bound checks are all wrong, since they compare nls_length derived
from strlen() to a unicode length limit.

It turns out that all the bound-checks do is to protect
hfsplus_asc2uni(), which can fail if the input is too large. There is
only one usage of it as far as attributes are concerned, in
hfsplus_attr_build_key(). It is in turn used by hfsplus_find_attr(),
hfsplus_create_attr(), hfsplus_delete_attr(). Thus making sure that
errors from hfsplus_asc2uni() is caught in hfsplus_attr_build_key()
and propagated is sufficient to replace all the bound checks.

Unpropagated errors from hfsplus_asc2uni() in the file catalog code was
addressed recently in an independent patch "hfsplus: fix longname handling"
by Sougata Santra.

Before this patch, trying to set a 55 CJK character (in a UTF-8
locale, > 127/3=42) attribute plus user prefix fails with:

    $ setfattr -n user.`cat testing-string` -v `cat testing-string` \
        testing-string
    setfattr: testing-string: Operation not supported

and retrieving a stored long attributes is particular ugly(!):

    find /mnt/* -type f -exec getfattr -d {} \;
    getfattr: /mnt/testing-string: Input/output error

with console log:
    [268008.389781] hfsplus: unicode conversion failed

After the patch, both of the above works.

FYI, the test attribute string is prepared with:

echo -e -n \
"\xe9\x80\x99\xe6\x98\xaf\xe4\xb8\x80\xe5\x80\x8b\xe9\x9d\x9e\xe5" \
"\xb8\xb8\xe6\xbc\xab\xe9\x95\xb7\xe8\x80\x8c\xe6\xa5\xb5\xe5\x85" \
"\xb6\xe4\xb9\x8f\xe5\x91\xb3\xe5\x92\x8c\xe7\x9b\xb8\xe7\x95\xb6" \
"\xe7\x84\xa1\xe8\xb6\xa3\xe3\x80\x81\xe4\xbb\xa5\xe5\x8f\x8a\xe7" \
"\x84\xa1\xe7\x94\xa8\xe7\x9a\x84\xe3\x80\x81\xe5\x86\x8d\xe5\x8a" \
"\xa0\xe4\xb8\x8a\xe6\xaf\xab\xe7\x84\xa1\xe6\x84\x8f\xe7\xbe\xa9" \
"\xe7\x9a\x84\xe6\x93\xb4\xe5\xb1\x95\xe5\xb1\xac\xe6\x80\xa7\xef" \
"\xbc\x8c\xe8\x80\x8c\xe5\x85\xb6\xe5\x94\xaf\xe4\xb8\x80\xe5\x89" \
"\xb5\xe5\xbb\xba\xe7\x9b\xae\xe7\x9a\x84\xe5\x83\x85\xe6\x98\xaf" \
"\xe7\x82\xba\xe4\xba\x86\xe6\xb8\xac\xe8\xa9\xa6\xe4\xbd\x9c\xe7" \
"\x94\xa8\xe3\x80\x82" | tr -d ' '

(= "pointlessly long attribute for testing", elaborate Chinese in
UTF-8 enoding).

However, it is not possible to set double the size (110 + 5 is still
under 127) in a UTF-8 locale:

    $setfattr -n user.`cat testing-string testing-string` -v \
        `cat testing-string testing-string` testing-string
    setfattr: testing-string: Numerical result out of range

110 CJK char in UTF-8 is 330 bytes - the generic get/set attribute
system call code in linux/fs/xattr.c imposes a 255 byte limit. One can
use a combination of iconv to encode content, changing terminal locale
for viewing, and an nls=cp932/cp936/cp949/cp950 mount option to fully
use 127-unicode attribute in a double-byte locale.

Also, as an additional information, it is possible to (mis-)use unicode
half-width/full-width forms (U+FFxx) to write attributes which looks like
english but not actually ascii.

Thanks Anton Altaparmakov for reviewing the earlier ideas behind this
change.

Signed-off-by: Hin-Tak Leung <htl10@users.sourceforge.net>
CC: Anton Altaparmakov <anton@tuxera.com>
CC: Vyacheslav Dubeyko <slava@dubeyko.com>
CC: Al Viro <viro@zeniv.linux.org.uk>
CC: Christoph Hellwig <hch@infradead.org>
CC: Sougata Santra <sougata@tuxera.com>
---
 fs/hfsplus/attributes.c     | 11 ++++-------
 fs/hfsplus/xattr.c          | 36 ++++++++++++++++++++--------------
 fs/hfsplus/xattr_security.c | 47 +++++++++++++++++++++++++--------------------
 fs/hfsplus/xattr_trusted.c  | 30 +++++++++++++++++------------
 fs/hfsplus/xattr_user.c     | 30 +++++++++++++++++------------
 5 files changed, 88 insertions(+), 66 deletions(-)

diff --git a/fs/hfsplus/attributes.c b/fs/hfsplus/attributes.c
index caf89a7..f3345c0 100644
--- a/fs/hfsplus/attributes.c
+++ b/fs/hfsplus/attributes.c
@@ -54,14 +54,11 @@ int hfsplus_attr_build_key(struct super_block *sb, hfsplus_btree_key *key,
 	memset(key, 0, sizeof(struct hfsplus_attr_key));
 	key->attr.cnid = cpu_to_be32(cnid);
 	if (name) {
-		len = strlen(name);
-		if (len > HFSPLUS_ATTR_MAX_STRLEN) {
-			pr_err("invalid xattr name's length\n");
-			return -EINVAL;
-		}
-		hfsplus_asc2uni(sb,
+		int res = hfsplus_asc2uni(sb,
 				(struct hfsplus_unistr *)&key->attr.key_name,
-				HFSPLUS_ATTR_MAX_STRLEN, name, len);
+				HFSPLUS_ATTR_MAX_STRLEN, name, strlen(name));
+		if (res)
+			return res;
 		len = be16_to_cpu(key->attr.key_name.length);
 	} else {
 		key->attr.key_name.length = 0;
diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
index 40c0a63..3ddf50c 100644
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -805,47 +805,55 @@ end_removexattr:
 static int hfsplus_osx_getxattr(struct dentry *dentry, const char *name,
 					void *buffer, size_t size, int type)
 {
-	char xattr_name[HFSPLUS_ATTR_MAX_STRLEN +
-				XATTR_MAC_OSX_PREFIX_LEN + 1] = {0};
-	size_t len = strlen(name);
+	char *xattr_name;
+	int res;
 
 	if (!strcmp(name, ""))
 		return -EINVAL;
 
-	if (len > HFSPLUS_ATTR_MAX_STRLEN)
-		return -EOPNOTSUPP;
-
 	/*
 	 * 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);
 
-	return hfsplus_getxattr(dentry, xattr_name, buffer, size);
+	res = hfsplus_getxattr(dentry, xattr_name, buffer, size);
+	kfree(xattr_name);
+	return res;
 }
 
 static int hfsplus_osx_setxattr(struct dentry *dentry, const char *name,
 		const void *buffer, size_t size, int flags, int type)
 {
-	char xattr_name[HFSPLUS_ATTR_MAX_STRLEN +
-				XATTR_MAC_OSX_PREFIX_LEN + 1] = {0};
-	size_t len = strlen(name);
+	char *xattr_name;
+	int res;
 
 	if (!strcmp(name, ""))
 		return -EINVAL;
 
-	if (len > HFSPLUS_ATTR_MAX_STRLEN)
-		return -EOPNOTSUPP;
-
 	/*
 	 * Don't allow setting 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);
 
-	return hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
+	res = hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
+	kfree(xattr_name);
+	return res;
 }
 
 static size_t hfsplus_osx_listxattr(struct dentry *dentry, char *list,
diff --git a/fs/hfsplus/xattr_security.c b/fs/hfsplus/xattr_security.c
index 0072276..716eedb 100644
--- a/fs/hfsplus/xattr_security.c
+++ b/fs/hfsplus/xattr_security.c
@@ -14,37 +14,43 @@
 static int hfsplus_security_getxattr(struct dentry *dentry, const char *name,
 					void *buffer, size_t size, int type)
 {
-	char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
-	size_t len = strlen(name);
+	char *xattr_name;
+	int res;
 
 	if (!strcmp(name, ""))
 		return -EINVAL;
 
-	if (len + XATTR_SECURITY_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
-		return -EOPNOTSUPP;
-
+	xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
+		GFP_KERNEL);
+	if (!xattr_name)
+		return -ENOMEM;
 	strcpy(xattr_name, XATTR_SECURITY_PREFIX);
 	strcpy(xattr_name + XATTR_SECURITY_PREFIX_LEN, name);
 
-	return hfsplus_getxattr(dentry, xattr_name, buffer, size);
+	res = hfsplus_getxattr(dentry, xattr_name, buffer, size);
+	kfree(xattr_name);
+	return res;
 }
 
 static int hfsplus_security_setxattr(struct dentry *dentry, const char *name,
 		const void *buffer, size_t size, int flags, int type)
 {
-	char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
-	size_t len = strlen(name);
+	char *xattr_name;
+	int res;
 
 	if (!strcmp(name, ""))
 		return -EINVAL;
 
-	if (len + XATTR_SECURITY_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
-		return -EOPNOTSUPP;
-
+	xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
+		GFP_KERNEL);
+	if (!xattr_name)
+		return -ENOMEM;
 	strcpy(xattr_name, XATTR_SECURITY_PREFIX);
 	strcpy(xattr_name + XATTR_SECURITY_PREFIX_LEN, name);
 
-	return hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
+	res = hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
+	kfree(xattr_name);
+	return res;
 }
 
 static size_t hfsplus_security_listxattr(struct dentry *dentry, char *list,
@@ -62,31 +68,30 @@ static int hfsplus_initxattrs(struct inode *inode,
 				void *fs_info)
 {
 	const struct xattr *xattr;
-	char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
-	size_t xattr_name_len;
+	char *xattr_name;
 	int err = 0;
 
+	xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
+		GFP_KERNEL);
+	if (!xattr_name)
+		return -ENOMEM;
 	for (xattr = xattr_array; xattr->name != NULL; xattr++) {
-		xattr_name_len = strlen(xattr->name);
 
-		if (xattr_name_len == 0)
+		if (!strcmp(xattr->name, ""))
 			continue;
 
-		if (xattr_name_len + XATTR_SECURITY_PREFIX_LEN >
-				HFSPLUS_ATTR_MAX_STRLEN)
-			return -EOPNOTSUPP;
-
 		strcpy(xattr_name, XATTR_SECURITY_PREFIX);
 		strcpy(xattr_name +
 			XATTR_SECURITY_PREFIX_LEN, xattr->name);
 		memset(xattr_name +
-			XATTR_SECURITY_PREFIX_LEN + xattr_name_len, 0, 1);
+			XATTR_SECURITY_PREFIX_LEN + strlen(xattr->name), 0, 1);
 
 		err = __hfsplus_setxattr(inode, xattr_name,
 					xattr->value, xattr->value_len, 0);
 		if (err)
 			break;
 	}
+	kfree(xattr_name);
 	return err;
 }
 
diff --git a/fs/hfsplus/xattr_trusted.c b/fs/hfsplus/xattr_trusted.c
index 426cee2..d2b1c1c 100644
--- a/fs/hfsplus/xattr_trusted.c
+++ b/fs/hfsplus/xattr_trusted.c
@@ -12,37 +12,43 @@
 static int hfsplus_trusted_getxattr(struct dentry *dentry, const char *name,
 					void *buffer, size_t size, int type)
 {
-	char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
-	size_t len = strlen(name);
+	char *xattr_name;
+	int res;
 
 	if (!strcmp(name, ""))
 		return -EINVAL;
 
-	if (len + XATTR_TRUSTED_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
-		return -EOPNOTSUPP;
-
+	xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
+		GFP_KERNEL);
+	if (!xattr_name)
+		return -ENOMEM;
 	strcpy(xattr_name, XATTR_TRUSTED_PREFIX);
 	strcpy(xattr_name + XATTR_TRUSTED_PREFIX_LEN, name);
 
-	return hfsplus_getxattr(dentry, xattr_name, buffer, size);
+	res = hfsplus_getxattr(dentry, xattr_name, buffer, size);
+	kfree(xattr_name);
+	return res;
 }
 
 static int hfsplus_trusted_setxattr(struct dentry *dentry, const char *name,
 		const void *buffer, size_t size, int flags, int type)
 {
-	char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
-	size_t len = strlen(name);
+	char *xattr_name;
+	int res;
 
 	if (!strcmp(name, ""))
 		return -EINVAL;
 
-	if (len + XATTR_TRUSTED_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
-		return -EOPNOTSUPP;
-
+	xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
+		GFP_KERNEL);
+	if (!xattr_name)
+		return -ENOMEM;
 	strcpy(xattr_name, XATTR_TRUSTED_PREFIX);
 	strcpy(xattr_name + XATTR_TRUSTED_PREFIX_LEN, name);
 
-	return hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
+	res = hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
+	kfree(xattr_name);
+	return res;
 }
 
 static size_t hfsplus_trusted_listxattr(struct dentry *dentry, char *list,
diff --git a/fs/hfsplus/xattr_user.c b/fs/hfsplus/xattr_user.c
index e340165..fc70f98 100644
--- a/fs/hfsplus/xattr_user.c
+++ b/fs/hfsplus/xattr_user.c
@@ -12,37 +12,43 @@
 static int hfsplus_user_getxattr(struct dentry *dentry, const char *name,
 					void *buffer, size_t size, int type)
 {
-	char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
-	size_t len = strlen(name);
+	char *xattr_name;
+	int res;
 
 	if (!strcmp(name, ""))
 		return -EINVAL;
 
-	if (len + XATTR_USER_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
-		return -EOPNOTSUPP;
-
+	xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
+		GFP_KERNEL);
+	if (!xattr_name)
+		return -ENOMEM;
 	strcpy(xattr_name, XATTR_USER_PREFIX);
 	strcpy(xattr_name + XATTR_USER_PREFIX_LEN, name);
 
-	return hfsplus_getxattr(dentry, xattr_name, buffer, size);
+	res = hfsplus_getxattr(dentry, xattr_name, buffer, size);
+	kfree(xattr_name);
+	return res;
 }
 
 static int hfsplus_user_setxattr(struct dentry *dentry, const char *name,
 		const void *buffer, size_t size, int flags, int type)
 {
-	char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
-	size_t len = strlen(name);
+	char *xattr_name;
+	int res;
 
 	if (!strcmp(name, ""))
 		return -EINVAL;
 
-	if (len + XATTR_USER_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
-		return -EOPNOTSUPP;
-
+	xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
+		GFP_KERNEL);
+	if (!xattr_name)
+		return -ENOMEM;
 	strcpy(xattr_name, XATTR_USER_PREFIX);
 	strcpy(xattr_name + XATTR_USER_PREFIX_LEN, name);
 
-	return hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
+	res = hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
+	kfree(xattr_name);
+	return res;
 }
 
 static size_t hfsplus_user_listxattr(struct dentry *dentry, char *list,
-- 
1.9.0

[PATCH 3/3] hfsplus: remove unused routine hfsplus_attr_build_key_uni

[Hin-Tak Leung]

From: Hin-Tak Leung <htl10@users.sourceforge.net>

The directory/file catalog b-tree equivalent, hfsplus_build_key_uni(),
is used by hfsplus_find_cat() for internal referencing between catalog
records. There is no corresponding usage for attributes - attribute
records do not refer to one another.

Signed-off-by: Hin-Tak Leung <htl10@users.sourceforge.net>
CC: Sougata Santra <sougata@tuxera.com>
---
 fs/hfsplus/attributes.c | 25 -------------------------
 fs/hfsplus/hfsplus_fs.h |  3 ---
 2 files changed, 28 deletions(-)

diff --git a/fs/hfsplus/attributes.c b/fs/hfsplus/attributes.c
index f3345c0..e5b221d 100644
--- a/fs/hfsplus/attributes.c
+++ b/fs/hfsplus/attributes.c
@@ -79,31 +79,6 @@ int hfsplus_attr_build_key(struct super_block *sb, hfsplus_btree_key *key,
 	return 0;
 }
 
-void hfsplus_attr_build_key_uni(hfsplus_btree_key *key,
-					u32 cnid,
-					struct hfsplus_attr_unistr *name)
-{
-	int ustrlen;
-
-	memset(key, 0, sizeof(struct hfsplus_attr_key));
-	ustrlen = be16_to_cpu(name->length);
-	key->attr.cnid = cpu_to_be32(cnid);
-	key->attr.key_name.length = cpu_to_be16(ustrlen);
-	ustrlen *= 2;
-	memcpy(key->attr.key_name.unicode, name->unicode, ustrlen);
-
-	/* The length of the key, as stored in key_len field, does not include
-	 * the size of the key_len field itself.
-	 * So, offsetof(hfsplus_attr_key, key_name) is a trick because
-	 * it takes into consideration key_len field (__be16) of
-	 * hfsplus_attr_key structure instead of length field (__be16) of
-	 * hfsplus_attr_unistr structure.
-	 */
-	key->key_len =
-		cpu_to_be16(offsetof(struct hfsplus_attr_key, key_name) +
-				ustrlen);
-}
-
 hfsplus_attr_entry *hfsplus_alloc_attr_entry(void)
 {
 	return kmem_cache_alloc(hfsplus_attr_tree_cachep, GFP_KERNEL);
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index 83dc292..6c08ff6 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -375,9 +375,6 @@ int hfsplus_attr_bin_cmp_key(const hfsplus_btree_key *,
 		const hfsplus_btree_key *);
 int hfsplus_attr_build_key(struct super_block *, hfsplus_btree_key *,
 			u32, const char *);
-void hfsplus_attr_build_key_uni(hfsplus_btree_key *key,
-					u32 cnid,
-					struct hfsplus_attr_unistr *name);
 int hfsplus_find_attr(struct super_block *, u32,
 			const char *, struct hfs_find_data *);
 int hfsplus_attr_exists(struct inode *inode, const char *name);
-- 
1.9.0

Re: [PATCH 1/3] hfsplus: fixes worst-case unicode to char conversion of file names and attributes

[Andrew Morton]

On Thu, 17 Apr 2014 20:30:20 +0100 Hin-Tak Leung <hintak.leung@gmail.com> wrote:

> From: Hin-Tak Leung <htl10@users.sourceforge.net>
> 
> The HFS Plus Volume Format specification (TN1150) states that
> file names are stored internally as a maximum of 255 unicode
> characters, as defined by The Unicode Standard, Version 2.0
> [Unicode, Inc. ISBN 0-201-48345-9]. File names are converted by
> the NLS system on Linux before presented to the user.
> 
> 255 CJK characters converts to UTF-8 with 1 unicode character
> to up to 3 bytes, and to GB18030 with 1 unicode character to
> up to 4 bytes. Thus, trying in a UTF-8 locale to list files
> with names of more than 85 CJK characters results in:
> 
>     $ ls /mnt
>     ls: reading directory /mnt: File name too long
> 
> The receiving buffer to hfsplus_uni2asc() needs to be 255 x
> NLS_MAX_CHARSET_SIZE bytes, not 255 bytes as the code has always been.
> 
> Similar consideration applies to attributes, which are stored
> internally as a maximum of 127 UTF-16BE units. See XNU source for
> an up-to-date reference on attributes.
> 
> Strictly speaking, the maximum value of NLS_MAX_CHARSET_SIZE = 6
> is not attainable in the case of conversion to UTF-8, as going
> beyond 3 bytes requires the use of surrogate pairs, i.e. consuming
> two input units.
> 
> Thanks Anton Altaparmakov for reviewing an earlier version of this
> change.
> 
> This patch fixes all callers of hfsplus_uni2asc(), and also enables
> the use of long non-English file names in HFS+. The getting and
> setting, and general usage of long non-English attributes
> requires further forthcoming work, in the following patches of this
> series.
> 

fs/hfsplus/xattr.c: In function 'hfsplus_listxattr':
fs/hfsplus/xattr.c:673: error: label 'out' used but not defined

--- a/fs/hfsplus/xattr.c~hfsplus-fixes-worst-case-unicode-to-char-conversion-of-file-names-and-attributes-fix
+++ a/fs/hfsplus/xattr.c
@@ -726,6 +726,7 @@ ssize_t hfsplus_listxattr(struct dentry
 
 end_listxattr:
 	kfree(strbuf);
+out:
 	hfs_find_exit(&fd);
 	return res;
 }

But obviously the patches you sent were not the patches you tested. 
What happened here?

Re: [PATCH 1/3] hfsplus: fixes worst-case unicode to char conversion of file names and attributes

[Hin-Tak Leung]

------------------------------
On Tue, Apr 22, 2014 9:28 PM BST Andrew Morton wrote:

>On Thu, 17 Apr 2014 20:30:20 +0100 Hin-Tak Leung <hintak.leung@gmail.com> wrote:
>
>> From: Hin-Tak Leung <htl10@users.sourceforge.net>
>> 
>> The HFS Plus Volume Format specification (TN1150) states that
>> file names are stored internally as a maximum of 255 unicode
>> characters, as defined by The Unicode Standard, Version 2.0
>> [Unicode, Inc. ISBN 0-201-48345-9]. File names are converted by
>> the NLS system on Linux before presented to the user.
>> 
>> 255 CJK characters converts to UTF-8 with 1 unicode character
>> to up to 3 bytes, and to GB18030 with 1 unicode character to
>> up to 4 bytes. Thus, trying in a UTF-8 locale to list files
>> with names of more than 85 CJK characters results in:
>> 
>>     $ ls /mnt
>>     ls: reading directory /mnt: File name too long
>> 
>> The receiving buffer to hfsplus_uni2asc() needs to be 255 x
>> NLS_MAX_CHARSET_SIZE bytes, not 255 bytes as the code has always been.
>> 
>> Similar consideration applies to attributes, which are stored
>> internally as a maximum of 127 UTF-16BE units. See XNU source for
>> an up-to-date reference on attributes.
>> 
>> Strictly speaking, the maximum value of NLS_MAX_CHARSET_SIZE = 6
>> is not attainable in the case of conversion to UTF-8, as going
>> beyond 3 bytes requires the use of surrogate pairs, i.e. consuming
>> two input units.
>> 
>> Thanks Anton Altaparmakov for reviewing an earlier version of this
>> change.
>> 
>> This patch fixes all callers of hfsplus_uni2asc(), and also enables
>> the use of long non-English file names in HFS+. The getting and
>> setting, and general usage of long non-English attributes
>> requires further forthcoming work, in the following patches of this
>> series.
>> 
>
>fs/hfsplus/xattr.c: In function 'hfsplus_listxattr':
>fs/hfsplus/xattr.c:673: error: label 'out' used but not defined
>
>--- a/fs/hfsplus/xattr.c~hfsplus-fixes-worst-case-unicode-to-char-conversion-of-file-names-and-attributes-fix
>+++ a/fs/hfsplus/xattr.c
>@@ -726,6 +726,7 @@ ssize_t hfsplus_listxattr(struct dentry
> 
> end_listxattr:
>     kfree(strbuf);
>+out:
>     hfs_find_exit(&fd);
>     return res;
> }
>
>But obviously the patches you sent were not the patches you tested. 
>What happened here?
>

Hi Andrew,

Am terribly sorry - my fault of using/working/testing under one kernel (3.13.9),
then cherry-picking onto Linus' HEAD and collapsing there. A couple of small
changes got dropped/messed up. There is the missing headers
(#include <linux/nls.h>) in xattr*.c you spotted in the other mail; the above
is meant to be corrected by re-using the label nearby (as kfree(null) is okay...), rather than adding
a new label.

diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
index 3ddf50c..2a38647 100644
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -670,7 +670,7 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
                        XATTR_MAC_OSX_PREFIX_LEN + 1, GFP_KERNEL);
        if (!strbuf) {
                res = -ENOMEM;
-               goto out;
+               goto end_listxattr;
        }
 
        err = hfsplus_find_attr(inode->i_sb, inode->i_ino, NULL, &fd);

Do you want me to re-submit? I checked the diff of the two branches, it
is just these two issues.

Really sorry about that.

Hin-Tak




--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] hfsplus: fixes worst-case unicode to char conversion of file names and attributes

[Andrew Morton]

On Wed, 23 Apr 2014 02:43:00 +0100 (BST) Hin-Tak Leung <htl10@users.sourceforge.net> wrote:

> >+++ a/fs/hfsplus/xattr.c
> >@@ -726,6 +726,7 @@ ssize_t hfsplus_listxattr(struct dentry
> > 
> > end_listxattr:
> > ______ kfree(strbuf);
> >+out:
> > ______ hfs_find_exit(&fd);
> > ______ return res;
> > }
> >
> >But obviously the patches you sent were not the patches you tested. 
> >What happened here?
> >
> 
> Hi Andrew,
> 
> Am terribly sorry - my fault of using/working/testing under one kernel (3.13.9),
> then cherry-picking onto Linus' HEAD and collapsing there. A couple of small
> changes got dropped/messed up. There is the missing headers
> (#include <linux/nls.h>) in xattr*.c you spotted in the other mail; the above
> is meant to be corrected by re-using the label nearby (as kfree(null) is okay...), rather than adding
> a new label.
> 
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index 3ddf50c..2a38647 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -670,7 +670,7 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
>                         XATTR_MAC_OSX_PREFIX_LEN + 1, GFP_KERNEL);
>         if (!strbuf) {
>                 res = -ENOMEM;
> -               goto out;
> +               goto end_listxattr;
>         }
>  
>         err = hfsplus_find_attr(inode->i_sb, inode->i_ino, NULL, &fd);
> 
> Do you want me to re-submit? I checked the diff of the two branches, it
> is just these two issues.
> 

Is OK - what I have now saves 30 cycles on a path which never happens ;)

It would help if you could grab tomorrow's linux-next and retest, to
check that everything landed OK.


How were you testing this anyway?  It's a pretty intricate patchset and
presumably some effort went into developing the testcases.  Perhaps you
have something which we can immortalize in tools/testing/selftests/?

Re: [PATCH 1/3] hfsplus: fixes worst-case unicode to char conversion of file names and attributes

[Hin-Tak Leung]

------------------------------
On Wed, Apr 23, 2014 2:59 AM BST Andrew Morton wrote:

>On Wed, 23 Apr 2014 02:43:00 +0100 (BST) Hin-Tak Leung <htl10@users.sourceforge.net> wrote:
>
>> >+++ a/fs/hfsplus/xattr.c
>> >@@ -726,6 +726,7 @@ ssize_t hfsplus_listxattr(struct dentry
>> > 
>> > end_listxattr:
>> > ______ kfree(strbuf);
>> >+out:
>> > ______ hfs_find_exit(&fd);
>> > ______ return res;
>> > }
>> >
>> >But obviously the patches you sent were not the patches you tested. 
>> >What happened here?
>> >
>> 
>> Hi Andrew,
>> 
>> Am terribly sorry - my fault of using/working/testing under one kernel (3.13.9),
>> then cherry-picking onto Linus' HEAD and collapsing there. A couple of small
>> changes got dropped/messed up. There is the missing headers
>> (#include <linux/nls.h>) in xattr*.c you spotted in the other mail; the above
>> is meant to be corrected by re-using the label nearby (as kfree(null) is okay...), rather than adding
>> a new label.
>> 
>> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
>> index 3ddf50c..2a38647 100644
>> --- a/fs/hfsplus/xattr.c
>> +++ b/fs/hfsplus/xattr.c
>> @@ -670,7 +670,7 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
>>                         XATTR_MAC_OSX_PREFIX_LEN + 1, GFP_KERNEL);
>>         if (!strbuf) {
>>                 res = -ENOMEM;
>> -               goto out;
>> +               goto end_listxattr;
>>         }
>>  
>>         err = hfsplus_find_attr(inode->i_sb, inode->i_ino, NULL, &fd);
>> 
>> Do you want me to re-submit? I checked the diff of the two branches, it
>> is just these two issues.
>> 
>
>Is OK - what I have now saves 30 cycles on a path which never happens ;)
>

Two mistakes (and both lead to failed compilation... ) is kind of unforgivable :-(. 

The label mistake was simply because I had it your way, then just edit out the new label... The missing header was because it was the first of a bunch, and 

git cherry-pick patch1..patchN

misses the first. I won't make the same mistake again - just possibly making new ones :-). 

>It would help if you could grab tomorrow's linux-next and retest, to
>check that everything landed OK.
>

will do. 

>
>How were you testing this anyway?  It's a pretty intricate patchset and
>presumably some effort went into developing the testcases.  Perhaps you
>have something which we can immortalize in tools/testing/selftests/?

It came able because I was staring at hfsplus_readir() to try to see how it gets confused just doing 'du' on a large directory. (It is a reproduceable problem, no solution is found yet, although it isn't likely related as others see to have similar issues of theirs doing updatedb or rm -rf)

So I see the disk to user encoding of file name was wrong - and I do have some non-english file names aleady there, though I made some new ones with just 'touch  ... '. Then Vyacheslav mentions the attributes need similar changes. So the attribute testcase was made up from scratch; I thought it would be nice to include the details for people who don't read/write chinese hence the rather extended message. 

As for immortalise it, we could just do 'touch  ... ' then remove the same thing, and make sure there is no failure with either touch or removal. I think we could adapt the long file name test from Sougata and extend it a bit? The limit to file names and attribute names is fs dependent though. (and also locale dependent, for fs which has a definite encoding within like hfs+)

btw, the linux kernel seems to have inherited a generic  limitation to attribute *values* being non-nulls, from xfs/sgi. Mac OS X attribute values can have nulls - in fact their set/get attribute tool, xattr, routinely do hex dumps, rather than print as string, for this reason, I think. 

Hin-Tak