------------------------------
On Sat, Apr 12, 2014 4:26 AM BST Hin-Tak Leung wrote:
>From: Hin-Tak Leung <htl10@xxxxxxxxxxxxxxxxxxxxx>
>
>HFSPLUS_ATTR_MAX_STRLEN (=127) is a limit 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 changed in an earlier patch),
>all the other uses are of the forms:
>
>- char buffer[size]
>
>- bound check: "if (input_length > size) return failure;"
>
>Conversion between on-disk and NLS char strings (in whichever direction)
>always needs to be in the worst, so all char buffers of that size
>need to have a NLS_MAX_CHARSET_SIZE x .
>
>The bound checks are all wrong, since they all compare
>nls_length to unicode_length_limit.
>
>A new function, hfsplus_try_asc2uni(), is introduced to
>try the conversion, to do the bound-check instead. (After review this
>will be splitted off as a separate earlier patch).
>
>Before this patch, trying to set a 55 CJK character (in a UTF-8 locale)
>+ user prefix fails with:
>
> $ setfattr -n user.`cat testing-string` -v `cat testing-string` testing-string
> setfattr: testing-string: Operation not supported
>
>and getting 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.
>
>However, I was not able to set double the size (110 + 5 is still under 127):
>
> $setfattr -n user.`cat testing-string testing-string` -v `cat testing-string testing-string` testing-string
> setfattr: testing-string: Numerical result out of range
>
>But this also fails on ext2, so this failure is not HFS+ specific. It seems
>that either the command line or the system call itself has a 256-byte limit.
>(110 CJK char in UTF-8 is 330 bytes).
>
With the mount option of nls=big5 and corresponding adjustment in my locale, I can set attributes with names in excess of 110 unicode characters (220 + 5 bytes in big5 encoding). So the 255 byte length nls limit is in userland.
Xfs and jfs documentations hints at a byte length limit of 255.
>Signed-off-by: Hin-Tak Leung <htl10@xxxxxxxxxxxxxxxxxxxxx>
>---
> fs/hfsplus/attributes.c | 11 ++++-----
> fs/hfsplus/hfsplus_fs.h | 7 ++++++
> fs/hfsplus/unicode.c | 18 +++++++++++++++
> fs/hfsplus/xattr.c | 40 ++++++++++++++++++++++----------
> fs/hfsplus/xattr_security.c | 56 ++++++++++++++++++++++++++++++---------------
> fs/hfsplus/xattr_trusted.c | 36 +++++++++++++++++++++--------
> fs/hfsplus/xattr_user.c | 36 +++++++++++++++++++++--------
> 7 files changed, 147 insertions(+), 57 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/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
>index 83dc292..80257d0 100644
>--- a/fs/hfsplus/hfsplus_fs.h
>+++ b/fs/hfsplus/hfsplus_fs.h
>@@ -507,6 +507,13 @@ int hfsplus_uni2asc(struct super_block *,
> const struct hfsplus_unistr *, char *, int *);
> int hfsplus_asc2uni(struct super_block *,
> struct hfsplus_unistr *, int, const char *, int);
>+int hfsplus_try_asc2uni_sb(struct super_block *sb,
>+ const char *nls_name, int unilimit);
>+static inline int hfsplus_try_asc2uni(const struct dentry *dentry,
>+ const char *nls_name, int unilimit)
>+{
>+ return hfsplus_try_asc2uni_sb(dentry->d_sb, nls_name, unilimit);
>+}
> int hfsplus_hash_dentry(const struct dentry *dentry, struct qstr *str);
> int hfsplus_compare_dentry(const struct dentry *parent, const struct dentry *dentry,
> unsigned int len, const char *str, const struct qstr *name);
>diff --git a/fs/hfsplus/unicode.c b/fs/hfsplus/unicode.c
>index e8ef121..cd00bd2 100644
>--- a/fs/hfsplus/unicode.c
>+++ b/fs/hfsplus/unicode.c
>@@ -330,6 +330,24 @@ int hfsplus_asc2uni(struct super_block *sb,
> }
>
> /*
>+ * Try encoding an nls_name within a unicode size limit,
>+ * and see whether it will fit.
>+ */
>+int hfsplus_try_asc2uni_sb(struct super_block *sb,
>+ const char *nls_name, int unilimit)
>+{
>+ int res = 0;
>+ struct hfsplus_unistr *ustr = kmalloc(sizeof(*ustr), GFP_KERNEL);
>+ if (!ustr)
>+ return -ENOMEM;
>+
>+ res = hfsplus_asc2uni(sb,
>+ ustr, unilimit, nls_name, strlen(nls_name));
>+ kfree(ustr);
>+ return res;
>+}
>+
>+/*
> * Hash a string to an integer as appropriate for the HFS+ filesystem.
> * Composed unicode characters are decomposed and case-folding is performed
> * if the appropriate bits are (un)set on the superblock.
>diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
>index 40c0a63..a05cab9 100644
>--- a/fs/hfsplus/xattr.c
>+++ b/fs/hfsplus/xattr.c
>@@ -805,15 +805,15 @@ 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;
>+ res = hfsplus_try_asc2uni(dentry, name, HFSPLUS_ATTR_MAX_STRLEN);
>+ if (res)
>+ return res;
>
> /*
> * Don't allow retrieving properly prefixed attributes
>@@ -821,22 +821,30 @@ 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);
>
>- 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;
>+ res = hfsplus_try_asc2uni(dentry, name, HFSPLUS_ATTR_MAX_STRLEN);
>+ if (res)
>+ return res;
>
> /*
> * Don't allow setting properly prefixed attributes
>@@ -844,8 +852,16 @@ 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);
>
>- 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..f4993e0 100644
>--- a/fs/hfsplus/xattr_security.c
>+++ b/fs/hfsplus/xattr_security.c
>@@ -14,37 +14,53 @@
> 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;
>+ res = hfsplus_try_asc2uni(dentry, name,
>+ HFSPLUS_ATTR_MAX_STRLEN - XATTR_SECURITY_PREFIX_LEN);
>+ if (res)
>+ return res;
>
>+ 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;
>+ res = hfsplus_try_asc2uni(dentry, name,
>+ HFSPLUS_ATTR_MAX_STRLEN - XATTR_SECURITY_PREFIX_LEN);
>+ if (res)
>+ return res;
>
>+ 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 +78,35 @@ 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;
>+ err = hfsplus_try_asc2uni_sb(inode->i_sb, xattr->name,
>+ HFSPLUS_ATTR_MAX_STRLEN - XATTR_SECURITY_PREFIX_LEN);
>+ if (err)
>+ break;
>
> 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..36716af 100644
>--- a/fs/hfsplus/xattr_trusted.c
>+++ b/fs/hfsplus/xattr_trusted.c
>@@ -12,37 +12,53 @@
> 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;
>+ res = hfsplus_try_asc2uni(dentry, name,
>+ HFSPLUS_ATTR_MAX_STRLEN - XATTR_TRUSTED_PREFIX_LEN);
>+ if (res)
>+ return res;
>
>+ 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;
>+ res = hfsplus_try_asc2uni(dentry, name,
>+ HFSPLUS_ATTR_MAX_STRLEN - XATTR_TRUSTED_PREFIX_LEN);
>+ if (res)
>+ return res;
>
>+ 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..26b832c 100644
>--- a/fs/hfsplus/xattr_user.c
>+++ b/fs/hfsplus/xattr_user.c
>@@ -12,37 +12,53 @@
> 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;
>+ res = hfsplus_try_asc2uni(dentry, name,
>+ HFSPLUS_ATTR_MAX_STRLEN - XATTR_USER_PREFIX_LEN);
>+ if (res)
>+ return res;
>
>+ 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;
>+ res = hfsplus_try_asc2uni(dentry, name,
>+ HFSPLUS_ATTR_MAX_STRLEN - XATTR_USER_PREFIX_LEN);
>+ if (res)
>+ return res;
>
>+ 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
