Commit bf29e886b242cebf6a96ca0e43873abc777e0b50

Authored by Hin-Tak Leung
Committed by Linus Torvalds
1 parent 017f8da43e

hfsplus: correct usage of HFSPLUS_ATTR_MAX_STRLEN for non-English attributes

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.

[akpm@linux-foundation.org: fix build]
[akpm@linux-foundation.org: fix build]
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>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Showing 5 changed files with 94 additions and 66 deletions Side-by-side Diff

fs/hfsplus/attributes.c
... ... @@ -54,14 +54,11 @@
54 54 memset(key, 0, sizeof(struct hfsplus_attr_key));
55 55 key->attr.cnid = cpu_to_be32(cnid);
56 56 if (name) {
57   - len = strlen(name);
58   - if (len > HFSPLUS_ATTR_MAX_STRLEN) {
59   - pr_err("invalid xattr name's length\n");
60   - return -EINVAL;
61   - }
62   - hfsplus_asc2uni(sb,
  57 + int res = hfsplus_asc2uni(sb,
63 58 (struct hfsplus_unistr *)&key->attr.key_name,
64   - HFSPLUS_ATTR_MAX_STRLEN, name, len);
  59 + HFSPLUS_ATTR_MAX_STRLEN, name, strlen(name));
  60 + if (res)
  61 + return res;
65 62 len = be16_to_cpu(key->attr.key_name.length);
66 63 } else {
67 64 key->attr.key_name.length = 0;
... ... @@ -806,47 +806,55 @@
806 806 static int hfsplus_osx_getxattr(struct dentry *dentry, const char *name,
807 807 void *buffer, size_t size, int type)
808 808 {
809   - char xattr_name[HFSPLUS_ATTR_MAX_STRLEN +
810   - XATTR_MAC_OSX_PREFIX_LEN + 1] = {0};
811   - size_t len = strlen(name);
  809 + char *xattr_name;
  810 + int res;
812 811  
813 812 if (!strcmp(name, ""))
814 813 return -EINVAL;
815 814  
816   - if (len > HFSPLUS_ATTR_MAX_STRLEN)
817   - return -EOPNOTSUPP;
818   -
819 815 /*
820 816 * Don't allow retrieving properly prefixed attributes
821 817 * by prepending them with "osx."
822 818 */
823 819 if (is_known_namespace(name))
824 820 return -EOPNOTSUPP;
  821 + xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN
  822 + + XATTR_MAC_OSX_PREFIX_LEN + 1, GFP_KERNEL);
  823 + if (!xattr_name)
  824 + return -ENOMEM;
  825 + strcpy(xattr_name, XATTR_MAC_OSX_PREFIX);
  826 + strcpy(xattr_name + XATTR_MAC_OSX_PREFIX_LEN, name);
825 827  
826   - return hfsplus_getxattr(dentry, xattr_name, buffer, size);
  828 + res = hfsplus_getxattr(dentry, xattr_name, buffer, size);
  829 + kfree(xattr_name);
  830 + return res;
827 831 }
828 832  
829 833 static int hfsplus_osx_setxattr(struct dentry *dentry, const char *name,
830 834 const void *buffer, size_t size, int flags, int type)
831 835 {
832   - char xattr_name[HFSPLUS_ATTR_MAX_STRLEN +
833   - XATTR_MAC_OSX_PREFIX_LEN + 1] = {0};
834   - size_t len = strlen(name);
  836 + char *xattr_name;
  837 + int res;
835 838  
836 839 if (!strcmp(name, ""))
837 840 return -EINVAL;
838 841  
839   - if (len > HFSPLUS_ATTR_MAX_STRLEN)
840   - return -EOPNOTSUPP;
841   -
842 842 /*
843 843 * Don't allow setting properly prefixed attributes
844 844 * by prepending them with "osx."
845 845 */
846 846 if (is_known_namespace(name))
847 847 return -EOPNOTSUPP;
  848 + xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN
  849 + + XATTR_MAC_OSX_PREFIX_LEN + 1, GFP_KERNEL);
  850 + if (!xattr_name)
  851 + return -ENOMEM;
  852 + strcpy(xattr_name, XATTR_MAC_OSX_PREFIX);
  853 + strcpy(xattr_name + XATTR_MAC_OSX_PREFIX_LEN, name);
848 854  
849   - return hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
  855 + res = hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
  856 + kfree(xattr_name);
  857 + return res;
850 858 }
851 859  
852 860 static size_t hfsplus_osx_listxattr(struct dentry *dentry, char *list,
fs/hfsplus/xattr_security.c
... ... @@ -7,6 +7,8 @@
7 7 */
8 8  
9 9 #include <linux/security.h>
  10 +#include <linux/nls.h>
  11 +
10 12 #include "hfsplus_fs.h"
11 13 #include "xattr.h"
12 14 #include "acl.h"
13 15  
14 16  
15 17  
16 18  
17 19  
... ... @@ -14,37 +16,43 @@
14 16 static int hfsplus_security_getxattr(struct dentry *dentry, const char *name,
15 17 void *buffer, size_t size, int type)
16 18 {
17   - char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
18   - size_t len = strlen(name);
  19 + char *xattr_name;
  20 + int res;
19 21  
20 22 if (!strcmp(name, ""))
21 23 return -EINVAL;
22 24  
23   - if (len + XATTR_SECURITY_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
24   - return -EOPNOTSUPP;
25   -
  25 + xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
  26 + GFP_KERNEL);
  27 + if (!xattr_name)
  28 + return -ENOMEM;
26 29 strcpy(xattr_name, XATTR_SECURITY_PREFIX);
27 30 strcpy(xattr_name + XATTR_SECURITY_PREFIX_LEN, name);
28 31  
29   - return hfsplus_getxattr(dentry, xattr_name, buffer, size);
  32 + res = hfsplus_getxattr(dentry, xattr_name, buffer, size);
  33 + kfree(xattr_name);
  34 + return res;
30 35 }
31 36  
32 37 static int hfsplus_security_setxattr(struct dentry *dentry, const char *name,
33 38 const void *buffer, size_t size, int flags, int type)
34 39 {
35   - char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
36   - size_t len = strlen(name);
  40 + char *xattr_name;
  41 + int res;
37 42  
38 43 if (!strcmp(name, ""))
39 44 return -EINVAL;
40 45  
41   - if (len + XATTR_SECURITY_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
42   - return -EOPNOTSUPP;
43   -
  46 + xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
  47 + GFP_KERNEL);
  48 + if (!xattr_name)
  49 + return -ENOMEM;
44 50 strcpy(xattr_name, XATTR_SECURITY_PREFIX);
45 51 strcpy(xattr_name + XATTR_SECURITY_PREFIX_LEN, name);
46 52  
47   - return hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
  53 + res = hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
  54 + kfree(xattr_name);
  55 + return res;
48 56 }
49 57  
50 58 static size_t hfsplus_security_listxattr(struct dentry *dentry, char *list,
51 59  
52 60  
53 61  
54 62  
55 63  
56 64  
... ... @@ -62,31 +70,30 @@
62 70 void *fs_info)
63 71 {
64 72 const struct xattr *xattr;
65   - char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
66   - size_t xattr_name_len;
  73 + char *xattr_name;
67 74 int err = 0;
68 75  
  76 + xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
  77 + GFP_KERNEL);
  78 + if (!xattr_name)
  79 + return -ENOMEM;
69 80 for (xattr = xattr_array; xattr->name != NULL; xattr++) {
70   - xattr_name_len = strlen(xattr->name);
71 81  
72   - if (xattr_name_len == 0)
  82 + if (!strcmp(xattr->name, ""))
73 83 continue;
74 84  
75   - if (xattr_name_len + XATTR_SECURITY_PREFIX_LEN >
76   - HFSPLUS_ATTR_MAX_STRLEN)
77   - return -EOPNOTSUPP;
78   -
79 85 strcpy(xattr_name, XATTR_SECURITY_PREFIX);
80 86 strcpy(xattr_name +
81 87 XATTR_SECURITY_PREFIX_LEN, xattr->name);
82 88 memset(xattr_name +
83   - XATTR_SECURITY_PREFIX_LEN + xattr_name_len, 0, 1);
  89 + XATTR_SECURITY_PREFIX_LEN + strlen(xattr->name), 0, 1);
84 90  
85 91 err = __hfsplus_setxattr(inode, xattr_name,
86 92 xattr->value, xattr->value_len, 0);
87 93 if (err)
88 94 break;
89 95 }
  96 + kfree(xattr_name);
90 97 return err;
91 98 }
92 99  
fs/hfsplus/xattr_trusted.c
... ... @@ -6,43 +6,51 @@
6 6 * Handler for trusted extended attributes.
7 7 */
8 8  
  9 +#include <linux/nls.h>
  10 +
9 11 #include "hfsplus_fs.h"
10 12 #include "xattr.h"
11 13  
12 14 static int hfsplus_trusted_getxattr(struct dentry *dentry, const char *name,
13 15 void *buffer, size_t size, int type)
14 16 {
15   - char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
16   - size_t len = strlen(name);
  17 + char *xattr_name;
  18 + int res;
17 19  
18 20 if (!strcmp(name, ""))
19 21 return -EINVAL;
20 22  
21   - if (len + XATTR_TRUSTED_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
22   - return -EOPNOTSUPP;
23   -
  23 + xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
  24 + GFP_KERNEL);
  25 + if (!xattr_name)
  26 + return -ENOMEM;
24 27 strcpy(xattr_name, XATTR_TRUSTED_PREFIX);
25 28 strcpy(xattr_name + XATTR_TRUSTED_PREFIX_LEN, name);
26 29  
27   - return hfsplus_getxattr(dentry, xattr_name, buffer, size);
  30 + res = hfsplus_getxattr(dentry, xattr_name, buffer, size);
  31 + kfree(xattr_name);
  32 + return res;
28 33 }
29 34  
30 35 static int hfsplus_trusted_setxattr(struct dentry *dentry, const char *name,
31 36 const void *buffer, size_t size, int flags, int type)
32 37 {
33   - char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
34   - size_t len = strlen(name);
  38 + char *xattr_name;
  39 + int res;
35 40  
36 41 if (!strcmp(name, ""))
37 42 return -EINVAL;
38 43  
39   - if (len + XATTR_TRUSTED_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
40   - return -EOPNOTSUPP;
41   -
  44 + xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
  45 + GFP_KERNEL);
  46 + if (!xattr_name)
  47 + return -ENOMEM;
42 48 strcpy(xattr_name, XATTR_TRUSTED_PREFIX);
43 49 strcpy(xattr_name + XATTR_TRUSTED_PREFIX_LEN, name);
44 50  
45   - return hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
  51 + res = hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
  52 + kfree(xattr_name);
  53 + return res;
46 54 }
47 55  
48 56 static size_t hfsplus_trusted_listxattr(struct dentry *dentry, char *list,
fs/hfsplus/xattr_user.c
... ... @@ -6,43 +6,51 @@
6 6 * Handler for user extended attributes.
7 7 */
8 8  
  9 +#include <linux/nls.h>
  10 +
9 11 #include "hfsplus_fs.h"
10 12 #include "xattr.h"
11 13  
12 14 static int hfsplus_user_getxattr(struct dentry *dentry, const char *name,
13 15 void *buffer, size_t size, int type)
14 16 {
15   - char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
16   - size_t len = strlen(name);
  17 + char *xattr_name;
  18 + int res;
17 19  
18 20 if (!strcmp(name, ""))
19 21 return -EINVAL;
20 22  
21   - if (len + XATTR_USER_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
22   - return -EOPNOTSUPP;
23   -
  23 + xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
  24 + GFP_KERNEL);
  25 + if (!xattr_name)
  26 + return -ENOMEM;
24 27 strcpy(xattr_name, XATTR_USER_PREFIX);
25 28 strcpy(xattr_name + XATTR_USER_PREFIX_LEN, name);
26 29  
27   - return hfsplus_getxattr(dentry, xattr_name, buffer, size);
  30 + res = hfsplus_getxattr(dentry, xattr_name, buffer, size);
  31 + kfree(xattr_name);
  32 + return res;
28 33 }
29 34  
30 35 static int hfsplus_user_setxattr(struct dentry *dentry, const char *name,
31 36 const void *buffer, size_t size, int flags, int type)
32 37 {
33   - char xattr_name[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
34   - size_t len = strlen(name);
  38 + char *xattr_name;
  39 + int res;
35 40  
36 41 if (!strcmp(name, ""))
37 42 return -EINVAL;
38 43  
39   - if (len + XATTR_USER_PREFIX_LEN > HFSPLUS_ATTR_MAX_STRLEN)
40   - return -EOPNOTSUPP;
41   -
  44 + xattr_name = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN + 1,
  45 + GFP_KERNEL);
  46 + if (!xattr_name)
  47 + return -ENOMEM;
42 48 strcpy(xattr_name, XATTR_USER_PREFIX);
43 49 strcpy(xattr_name + XATTR_USER_PREFIX_LEN, name);
44 50  
45   - return hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
  51 + res = hfsplus_setxattr(dentry, xattr_name, buffer, size, flags);
  52 + kfree(xattr_name);
  53 + return res;
46 54 }
47 55  
48 56 static size_t hfsplus_user_listxattr(struct dentry *dentry, char *list,