linux-kernel - Re: [PATCH 1/1] hfsplus: fix longname handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <530C6AD5.90905@tuxera.com>
Date:	Tue, 25 Feb 2014 12:05:09 +0200
From:	sougata santra <sougata@...era.com>
To:	Vyacheslav Dubeyko <slava@...eyko.com>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	Joe Perches <joe@...ches.com>, Christoph Hellwig <hch@....de>,
	Al Viro <viro@...iv.linux.org.uk>,
	<linux-fsdevel@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [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@...era.com>
>> Reviewed-by: Christoph Hellwig <hch@....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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/