lkml.org 
[lkml]   [2014]   [Feb]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 1/1] hfsplus: fix longname handling
On 02/25/2014 09:37 AM, Vyacheslav Dubeyko wrote:
> Hi Sougata,
>
> On Mon, 2014-02-24 at 21:28 +0200, Sougata Santra wrote:
>> -ENAMETOOLONG returned from hfsplus_asc2uni was not propaged to iops. This
>> allowed hfsplus to create files/directories with HFSPLUS_MAX_STRLEN and
>> incorrect keys, leaving the FS in an inconsistent state. This patch fixes
>> this issue.
>>
>
> Good fix. Thank you.

Thank you for taking time to look into it.
>
> I have some small remarks. Please, see below.
>
>> Signed-off-by: Sougata Santra <sougata@tuxera.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> ---
>> fs/hfsplus/catalog.c | 89 ++++++++++++++++++++++++++++++++++++-------------
>> fs/hfsplus/dir.c | 11 ++++--
>> fs/hfsplus/hfsplus_fs.h | 4 ++-
>> fs/hfsplus/super.c | 4 ++-
>> 4 files changed, 79 insertions(+), 29 deletions(-)
>>
>> diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
>> index 968ce41..389c474 100644
>> --- a/fs/hfsplus/catalog.c
>> +++ b/fs/hfsplus/catalog.c
>> @@ -38,21 +38,30 @@ int hfsplus_cat_bin_cmp_key(const hfsplus_btree_key *k1,
>> return hfsplus_strcmp(&k1->cat.name, &k2->cat.name);
>> }
>>
>> -void hfsplus_cat_build_key(struct super_block *sb, hfsplus_btree_key *key,
>> - u32 parent, struct qstr *str)
>> +/* Generates key for catalog file/folders record. */
>> +int hfsplus_cat_build_key(struct super_block *sb,
>> + hfsplus_btree_key *key, u32 parent, struct qstr *str)
>> {
>> - int len;
>> + int len, err;
>>
>> key->cat.parent = cpu_to_be32(parent);
>> - if (str) {
>> - hfsplus_asc2uni(sb, &key->cat.name, HFSPLUS_MAX_STRLEN,
>> - str->name, str->len);
>> - len = be16_to_cpu(key->cat.name.length);
>> - } else {
>> - key->cat.name.length = 0;
>> - len = 0;
>> - }
>> + err = hfsplus_asc2uni(sb, &key->cat.name, HFSPLUS_MAX_STRLEN,
>> + str->name, str->len);
>> + if (unlikely(err < 0))
>> + return err;
>> +
>> + len = be16_to_cpu(key->cat.name.length);
>> key->key_len = cpu_to_be16(6 + 2 * len);
>
> I think that maybe it is time to change hardcoded 6 on sensible named
> constant. What do you think?

I agree, although I think this should he done in a separate patch. Also
there are other instances of hard-coding. We can clean them with a patch. ?

>
>> + return 0;
>> +}
>> +
>> +/* Generates key for catalog thread record. */
>> +void hfsplus_cat_build_key_with_cnid(struct super_block *sb,
>> + hfsplus_btree_key *key, u32 parent)
>> +{
>> + key->cat.parent = cpu_to_be32(parent);
>> + key->cat.name.length = 0;
>> + key->key_len = cpu_to_be16(6);
>
> Ditto.
>
>> }
>>
>> static void hfsplus_cat_build_key_uni(hfsplus_btree_key *key, u32 parent,
>
> We have such code for hfsplus_cat_build_key_uni():
>
> 58 static void hfsplus_cat_build_key_uni(hfsplus_btree_key *key, u32 parent,
> 59 struct hfsplus_unistr *name)
> 60 {
> 61 int ustrlen;
> 62
> 63 ustrlen = be16_to_cpu(name->length);
>
> I suppose that it makes sense to check name->length here and return
> error. We can check possible volume corruption here.

I looked into it while writing the patch. I think this was already
handled before. Please see. catalog.c#hfsplus_find_cat the only place it
is called.

{code}
if (be16_to_cpu(tmp.thread.nodeName.length) > 255) {
pr_err("catalog name length corrupted\n");
return -EIO;
}
{code}

>
> 64 key->cat.parent = cpu_to_be32(parent);
> 65 key->cat.name.length = cpu_to_be16(ustrlen);
> 66 ustrlen *= 2;
> 67 memcpy(key->cat.name.unicode, name->unicode, ustrlen);
> 68 key->key_len = cpu_to_be16(6 + ustrlen);
> 69 }
>
> What do you think about such suggestion?
>
>> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
>> index 08846425b..66671c4 100644
>> --- a/fs/hfsplus/hfsplus_fs.h
>> +++ b/fs/hfsplus/hfsplus_fs.h
>> @@ -443,8 +443,10 @@ int hfsplus_cat_case_cmp_key(const hfsplus_btree_key *,
>> const hfsplus_btree_key *);
>> int hfsplus_cat_bin_cmp_key(const hfsplus_btree_key *,
>> const hfsplus_btree_key *);
>> -void hfsplus_cat_build_key(struct super_block *sb,
>> +int hfsplus_cat_build_key(struct super_block *sb,
>> hfsplus_btree_key *, u32, struct qstr *);
>> +void hfsplus_cat_build_key_with_cnid(struct super_block *sb,
>
> The whole style of the fix is good. But such mess looks not very good.
> But it is not critical, of course. :)

Thank you for taking time to read it.
>
> Thanks,
> Vyacheslav Dubeyko.
>
>
Best regards,
Sougata


\
 
 \ /
  Last update: 2014-02-25 11:41    [W:0.090 / U:0.168 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site