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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Anton,

------------------------------
On Tue, Apr 8, 2014 4:06 PM BST Anton Altaparmakov wrote:

>Hi Hin-Tak,
>
>That now looks good.
>
>I agree with you that doing a major cleanup of how attribute names are handled is a separate patch.  As you say that is much more work and it is not as trivial as this.  I think doing the attribute handling properly requires reworking how it all fits together because it is imposing the name length checks at the wrong level - they need to be done after conversion to UTF-16 not whilst they are still in the current NLS (except of course you want to check that they are not too big to fit in the buffer though it is not at all clear to me why the argument is being copied into a buffer in the first place at least in one of the places I looked at - seems a waste of CPU time to me).
>

The bigger attribute change part has a mixture of prefixes (in bytes and nls local strings), and also moving/concatenating within the buffer, it needs a lot more thinking and work. But I think just to address what comes out of hfsplus_uni2asc() (from on disk to in-memory), the patch is all that is needed.

For the copying of argument, if you are referring to the len being passed into uni2asc - it is being used as both a limit and also a return value for the actual number of byte used.

>Also, I don't think it is needed to set up a kmem cache for this - kmalloc is perfect - as at any one time there will be a vanishingly small number of these buffers allocated at any one time.  After all, how many concurrent readdir() calls are there going to be on a volume at any one time?  It is not like the struct inode or other long lived memory structures where you end up with thousands of them in memory at any one time...
>

I haven't been able to find any reference to which to use (grep'ing under Documentation/* ) - there are a lot of *alloc in the kernel . Even finding kzalloc does what calloc in userland (and kcalloc is a totally different beast) took some effort.

The attribute ones seem to need kzalloc (or, just '\0' at the beginning to null terminate if empty) - they are being used in concatenating, etc. As i said, cleaning that up will be a lot more work.

>Just one thing I wanted to mention in case you did not know it: kfree(NULL) is allowed - in fact kfree() function starts by doing "if (NULL argument) return;" so in your patch you could instead of adding an additional exit label, just use the generic one that also does a kfree() which is safe as the pointer is NULL.  As this is only error handling code path it does not matter that you incur an additional function call overhead.
>

Thanks. I did have the impression and google'd to confirm that a change to make it work that way happened in 2005/2006. I have played with various ways of re-arranging the code, specifically whether strbuf should go before or after find_init, and how they could fail (independently), but in the end, the problem is simply that only one of them can do "if fail then return err". The other will have to clean up the opposite party before the returning. There might be future addition where the order is important.

>Whether you want to change the exit label or not, you can change the CC: to Reviewed-By: for me if you like and I would suggest you send it to Andrew Morton for inclusion through his patch series.

Thanks. I'll possibly change a few words in the commit message. Let's see if others have any more to say.

Hin-Tak

>On 8 Apr 2014, at 03:19, Hin-Tak Leung <hintak.leung@xxxxxxxxx> wrote:
>
>> From: Hin-Tak Leung <htl10@xxxxxxxxxxxxxxxxxxxxx>
>> 
>> The HFS Plus Volume Format specification (TN1150) states that
>> file names are stored internally as a maximum of 255 unicode
>> characters, as defined by The Unicode Standard, Version 2.0
>> [Unicode, Inc. ISBN 0-201-48345-9]. File names are converted by
>> the NLS system on Linux before presented to the user.
>> 
>> 255 CJK characters converts to UTF-8 with 1 unicode character
>> to up to 3 bytes, and to GB18030 with 1 unicode character to
>> up to 4 bytes. Thus, trying in a UTF-8 locale to list files
>> with names of more than 85 CJK characters results in:
>> 
>>    $ ls /mnt
>>    ls: reading directory /mnt: File name too long
>> 
>> The receiving buffer needs to be 255 x NLS_MAX_CHARSET_SIZE bytes,
>> not 255 bytes as the code has always been.
>> 
>> Similar consideration applies to attributes, which are stored
>> internally as a maximum of 127 UTF-16BE units in HFS+.
>> 
>> Strictly speaking, the maximum value of NLS_MAX_CHARSET_SIZE = 6
>> is not attainable in the case of conversion to UTF-8, as that
>> requires the use of surrogate pairs, i.e. consuming two storage units.
>> 
>> Thanks Anton Altaparmakov for reviewing an earlier version of
>> this change.
>> 
>> Signed-off-by: Hin-Tak Leung <htl10@xxxxxxxxxxxxxxxxxxxxx>
>> CC: Anton Altaparmakov <anton@xxxxxxxxxx>
>> CC: Vyacheslav Dubeyko <slava@xxxxxxxxxxx>
>> CC: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
>> CC: Christoph Hellwig <hch@xxxxxxxxxxxxx>
>> ---
>> fs/hfsplus/dir.c   | 12 ++++++++++--
>> fs/hfsplus/xattr.c | 15 ++++++++++++---
>> 2 files changed, 22 insertions(+), 5 deletions(-)
>> 
>> diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
>> index bdec665..f95abef 100644
>> --- a/fs/hfsplus/dir.c
>> +++ b/fs/hfsplus/dir.c
>> @@ -12,6 +12,7 @@
>> #include <linux/fs.h>
>> #include <linux/slab.h>
>> #include <linux/random.h>
>> +#include <linux/nls.h>
>> 
>> #include "hfsplus_fs.h"
>> #include "hfsplus_raw.h"
>> @@ -127,7 +128,7 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
>>     struct inode *inode = file_inode(file);
>>     struct super_block *sb = inode->i_sb;
>>     int len, err;
>> -    char strbuf[HFSPLUS_MAX_STRLEN + 1];
>> +    char *strbuf;
>>     hfsplus_cat_entry entry;
>>     struct hfs_find_data fd;
>>     struct hfsplus_readdir_data *rd;
>> @@ -139,6 +140,11 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
>>     err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
>>     if (err)
>>         return err;
>> +    strbuf = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_MAX_STRLEN + 1, GFP_KERNEL);
>> +    if (!strbuf) {
>> +        err = -ENOMEM;
>> +        goto failed_strbuf_alloc;
>> +    }
>>     hfsplus_cat_build_key(sb, fd.search_key, inode->i_ino, NULL);
>>     err = hfs_brec_find(&fd, hfs_find_rec_by_key);
>>     if (err)
>> @@ -193,7 +199,7 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
>>         hfs_bnode_read(fd.bnode, &entry, fd.entryoffset,
>>             fd.entrylength);
>>         type = be16_to_cpu(entry.type);
>> -        len = HFSPLUS_MAX_STRLEN;
>> +        len = NLS_MAX_CHARSET_SIZE * HFSPLUS_MAX_STRLEN;
>>         err = hfsplus_uni2asc(sb, &fd.key->cat.name, strbuf, &len);
>>         if (err)
>>             goto out;
>> @@ -246,6 +252,8 @@ next:
>>     }
>>     memcpy(&rd->key, fd.key, sizeof(struct hfsplus_cat_key));
>> out:
>> +    kfree(strbuf);
>> +failed_strbuf_alloc:
>>     hfs_find_exit(&fd);
>>     return err;
>> }
>> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
>> index 4e27edc..cfbb23d 100644
>> --- a/fs/hfsplus/xattr.c
>> +++ b/fs/hfsplus/xattr.c
>> @@ -8,6 +8,7 @@
>> 
>> #include "hfsplus_fs.h"
>> #include <linux/posix_acl_xattr.h>
>> +#include <linux/nls.h>
>> #include "xattr.h"
>> #include "acl.h"
>> 
>> @@ -645,8 +646,7 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
>>     struct hfs_find_data fd;
>>     u16 key_len = 0;
>>     struct hfsplus_attr_key attr_key;
>> -    char strbuf[HFSPLUS_ATTR_MAX_STRLEN +
>> -            XATTR_MAC_OSX_PREFIX_LEN + 1] = {0};
>> +    char *strbuf;
>>     int xattr_name_len;
>> 
>>     if ((!S_ISREG(inode->i_mode) &&
>> @@ -666,6 +666,13 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
>>         return err;
>>     }
>> 
>> +    strbuf = kzalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN +
>> +            XATTR_MAC_OSX_PREFIX_LEN + 1, GFP_KERNEL);
>> +    if (!strbuf) {
>> +        res = -ENOMEM;
>> +        goto failed_strbuf_alloc;
>> +    }
>> +
>>     err = hfsplus_find_attr(inode->i_sb, inode->i_ino, NULL, &fd);
>>     if (err) {
>>         if (err == -ENOENT) {
>> @@ -692,7 +699,7 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
>>         if (be32_to_cpu(attr_key.cnid) != inode->i_ino)
>>             goto end_listxattr;
>> 
>> -        xattr_name_len = HFSPLUS_ATTR_MAX_STRLEN;
>> +        xattr_name_len = NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN;
>>         if (hfsplus_uni2asc(inode->i_sb,
>>             (const struct hfsplus_unistr *)&fd.key->attr.key_name,
>>                     strbuf, &xattr_name_len)) {
>> @@ -718,6 +725,8 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
>>     }
>> 
>> end_listxattr:
>> +    kfree(strbuf);
>> +failed_strbuf_alloc:
>>     hfs_find_exit(&fd);
>>     return res;
>> }
>> -- 
>> 1.9.0
>
>-- 
>Anton Altaparmakov <anton at tuxera.com> (replace at with @)
>Senior Kernel Engineer, Tuxera Inc., http://www.tuxera.com/
>Linux NTFS maintainer
>

--
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




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux