* [PATCH RFC V1] hfsplus: Correct usage of HFSPLUS_ATTR_MAX_STRLEN for non-English attributes
@ 2014-04-12 3:26 Hin-Tak Leung
2014-04-12 23:01 ` Anton Altaparmakov
0 siblings, 1 reply; 4+ messages in thread
From: Hin-Tak Leung @ 2014-04-12 3:26 UTC (permalink / raw)
To: linux-fsdevel; +Cc: Hin-Tak Leung
From: Hin-Tak Leung <htl10@users.sourceforge.net>
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).
Signed-off-by: Hin-Tak Leung <htl10@users.sourceforge.net>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH RFC V1] hfsplus: Correct usage of HFSPLUS_ATTR_MAX_STRLEN for non-English attributes
@ 2014-04-12 11:34 Hin-Tak Leung
0 siblings, 0 replies; 4+ messages in thread
From: Hin-Tak Leung @ 2014-04-12 11:34 UTC (permalink / raw)
To: linux-fsdevel
------------------------------
On Sat, Apr 12, 2014 4:26 AM BST Hin-Tak Leung wrote:
>From: Hin-Tak Leung <htl10@users.sourceforge.net>
>
>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@users.sourceforge.net>
>---
> 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RFC V1] hfsplus: Correct usage of HFSPLUS_ATTR_MAX_STRLEN for non-English attributes
2014-04-12 3:26 [PATCH RFC V1] hfsplus: Correct usage of HFSPLUS_ATTR_MAX_STRLEN for non-English attributes Hin-Tak Leung
@ 2014-04-12 23:01 ` Anton Altaparmakov
0 siblings, 0 replies; 4+ messages in thread
From: Anton Altaparmakov @ 2014-04-12 23:01 UTC (permalink / raw)
To: Hin-Tak Leung; +Cc: linux-fsdevel, Hin-Tak Leung
Hi Hin-Tak,
I think you are taking a sub-optimal approach! Think about it: you are now performing the hfsplus_asc2uni() twice for each name being handled! Once to check if the length is ok and once to get the actual result. This is a waste of time.
Instead, just do the conversion. If the name is longer than allowed, hfsplus_asc2uni() will return -ENAMETOOLONG and at that point you can bail out. There is no need to do a "test conversion" first the result of which you through away (and even worse you do a kmalloc() twice - once for the "test" and once for the real thing)...
In fact you do it correctly in your proposed patch in hfsplus_attr_build_key(): You thrown out the length check and replace it with a call to hfsplus_asc2uni() that returns -ENAMETOOLONG if the name is too long and thus hfsplus_attr_build_key() returns -ENAMETOOLONG. All you need to do is change the code to do the Right Thing when it gets back -ENAMETOOLONG and you are done (and you throw away all the bogus length checks in the upper layers).
If you definitely want to do checking in advance, then that is fine, too - but don't do it with a test conversion - instead, convert it, check the length of the converted string and then use the converted name from there onwards.
Both my suggestions are perhaps more involved changes than what you are doing but I think they are much more worthwhile as they do not introduce double the allocations and conversions.
Best regards,
Anton
On 12 Apr 2014, at 04:26, Hin-Tak Leung <hintak.leung@gmail.com> wrote:
> From: Hin-Tak Leung <htl10@users.sourceforge.net>
>
> 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).
>
> Signed-off-by: Hin-Tak Leung <htl10@users.sourceforge.net>
> ---
> 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Best regards,
Anton
--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RFC V1] hfsplus: Correct usage of HFSPLUS_ATTR_MAX_STRLEN for non-English attributes
@ 2014-04-13 0:52 Hin-Tak Leung
0 siblings, 0 replies; 4+ messages in thread
From: Hin-Tak Leung @ 2014-04-13 0:52 UTC (permalink / raw)
To: aia21; +Cc: linux-fsdevel
Hi Anton,
Thanks again for a rather thoughtful review.
I do completely agree, that the bound checks are almost completely useless before and after - before
the change they were simply wrong, after the change they are simply doing twice the work, just earlier.
There is no need to do twice the work - if hfsplus_asc2uni() works correctly, and failures are caught properly,
there is no need to try() it first. So that's why I simply removed the one in hfsplus_attr_build_key(),since
the check happens immediately before the actual call. It is definitely stupid doing a try() then immediately
the real thing.
But there is one advantage of doing it early - assuming try() is cheap (may or may not be the case):
the real hfsplus_asc2uni() is buried quite a lot further after setting up the atttribute B-tree for
searches, etc. So by the time the actual call happens, throwing away all that work B-tree
find_init, etc might be wasteful; besides that stuff also have mutex locks around it, etc, so
unwinding and catch errors there may not be as easy.
In principle, I agree that if any error from hfsplus_asc2uni() is checked and caught, there
is no need to do a try() first; the practical dificulty is just that the real
hfsplus_asc2uni() is buried rather deep, and within at least one mutex lock.
So I did think about the first approach you suggested, just thought it is much harder
- the current patch is just too boring and not doing anything risky, so it is safe to use. I think the
2nd approach (keeping the try() result) would get around my worry about that mutex lock, so
I may look into that.
Now I have an "tedious, slow but working correctly how it should be" patch, for reference, and
it is a good thing to have. - it is quite educational to try to use all 127 unicode character
limit of the attribute - and not possible to do that in a UTF-8 locale. Both the userland tools (xttr)
and many of the other file systems have a 255 byte limit for EA's (in nls, and 127
unicode char can be as bad as 127 x 3 = 381 in UTF-8). I had to use an explicit nls=<not-utf-8>
mount option and run my terminal/content at a different encoding to fully use
that 127 unit storage.
When a Mac user had already written an unusually long attribute, and we try to
read it under linux - it currently (without this patch) gives a scary I/O error. That's
really not good.
i have thought of a genuine (mis-) use of non-english attributes. All the english
alphabets have their "half/full-width" equivalent at a higher
code point. ( http://en.wikipedia.org/wiki/Halfwidth_and_fullwidth_forms ).
It is entirely possible for a japanese person to be typing what he thought
is english, and looks like english, and read like english - only stretched out
or shrunk - but not actually ascii.
I''ll have a go at ripping all the useless bound checks out, and
see how painful it gets.
Hin-Tak
------------------------------
On Sun, Apr 13, 2014 12:01 AM BST Anton Altaparmakov wrote:
>Hi Hin-Tak,
>
>I think you are taking a sub-optimal approach! Think about it: you are now performing the hfsplus_asc2uni() twice for each name being handled! Once to check if the length is ok and once to get the actual result. This is a waste of time.
>
>Instead, just do the conversion. If the name is longer than allowed, hfsplus_asc2uni() will return -ENAMETOOLONG and at that point you can bail out. There is no need to do a "test conversion" first the result of which you through away (and even worse you do a kmalloc() twice - once for the "test" and once for the real thing)...
>
>In fact you do it correctly in your proposed patch in hfsplus_attr_build_key(): You thrown out the length check and replace it with a call to hfsplus_asc2uni() that returns -ENAMETOOLONG if the name is too long and thus hfsplus_attr_build_key() returns -ENAMETOOLONG. All you need to do is change the code to do the Right Thing when it gets back -ENAMETOOLONG and you are done (and you throw away all the bogus length checks in the upper layers).
>
>If you definitely want to do checking in advance, then that is fine, too - but don't do it with a test conversion - instead, convert it, check the length of the converted string and then use the converted name from there onwards.
>
>Both my suggestions are perhaps more involved changes than what you are doing but I think they are much more worthwhile as they do not introduce double the allocations and conversions.
>
>Best regards,
>
> Anton
>
>On 12 Apr 2014, at 04:26, Hin-Tak Leung <hintak.leung@gmail.com> wrote:
>
>> From: Hin-Tak Leung <htl10@users.sourceforge.net>
>>
>> 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).
>>
>> Signed-off-by: Hin-Tak Leung <htl10@users.sourceforge.net>
>> ---
>> 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@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>Best regards,
>
> Anton
>--
>Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
>Unix Support, Computing Service, University of Cambridge
>J.J. Thomson Avenue, Cambridge, CB3 0RB, UK
>
--
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-04-13 0:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-12 3:26 [PATCH RFC V1] hfsplus: Correct usage of HFSPLUS_ATTR_MAX_STRLEN for non-English attributes Hin-Tak Leung
2014-04-12 23:01 ` Anton Altaparmakov
-- strict thread matches above, loose matches on Subject: below --
2014-04-12 11:34 Hin-Tak Leung
2014-04-13 0:52 Hin-Tak Leung
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).