linux-kernel - Re: [PATCH 2/4] ubifs: Fix unlink code wrt. double hash lookups

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cd4c5713-d091-3445-071e-d880c40710d9@nod.at>
Date:   Sun, 19 Mar 2017 21:46:16 +0100
From:   Richard Weinberger <richard@....at>
To:     Hyunchul Lee <hyc.lee@...il.com>
Cc:     linux-mtd@...ts.infradead.org, david.oberhollenzer@...ma-star.at,
        linux-kernel@...r.kernel.org, dedekind1@...il.com,
        kernel-team@....com
Subject: Re: [PATCH 2/4] ubifs: Fix unlink code wrt. double hash lookups

Hyunchul,

Am 09.03.2017 um 08:04 schrieb Hyunchul Lee:
>> -	int n, err, type = key_type(c, key);
>> -	struct ubifs_znode *znode;
>> +	int err;
> 
> i guess that err should be initialized with -ENOENT to avoid the first call of  
> tnc_next(c, &znode, n). 

Yes. err is used unitialized. Happened most likely while moving the code block around.

[...]

>>  /**
>> + * ubifs_tnc_remove_dh - remove an index entry for a "double hashed" node.
>> + * @c: UBIFS file-system description object
>> + * @key: key of node
>> + * @cookie: node cookie for collision resolution
>> + *
>> + * Returns %0 on success or negative error code on failure.
>> + */
>> +int ubifs_tnc_remove_dh(struct ubifs_info *c, const union ubifs_key *key,
>> +			uint32_t cookie)
>> +{
>> +	int n, err;
>> +	struct ubifs_znode *znode;
>> +	struct ubifs_dent_node *dent;
>> +	struct ubifs_zbranch *zbr;
>> +
>> +	if (!c->double_hash)
>> +		return -EOPNOTSUPP;
>> +
>> +	mutex_lock(&c->tnc_mutex);
>> +	err = lookup_level0_dirty(c, key, &znode, &n);
>> +	if (err <= 0)
>> +		goto out_unlock;
>> +
>> +	zbr = &znode->zbranch[n];
>> +	dent = kmalloc(UBIFS_MAX_DENT_NODE_SZ, GFP_NOFS);
>> +	if (!dent) {
>> +		err = -ENOMEM;
>> +		goto out_unlock;
>> +	}
>> +
>> +	err = tnc_read_hashed_node(c, zbr, dent);
>> +	if (err)
>> +		goto out_free;
>> +
>> +	/* If the cookie does not match, we're facing a hash collision. */
>> +	if (le32_to_cpu(dent->cookie) != cookie) {
>> +		union ubifs_key start_key;
>> +
>> +		lowest_dent_key(c, &start_key, key_inum(c, key));
>> +
>> +		err = ubifs_lookup_level0(c, &start_key, &znode, &n);
>> +		if (unlikely(err < 0))
>> +			goto out_unlock;
> 
> i guess that out_unlock should be replaced with out_free to free dent.
>

Ohhh, correct.

>> +
>> +		err = search_dh_cookie(c, key, dent, cookie, &znode, &n);
>> +		if (err)
>> +			goto out_free;
>> +	}
>> +
>> +	if (znode->cnext || !ubifs_zn_dirty(znode)) {
>> +		znode = dirty_cow_bottom_up(c, znode);
>> +		if (IS_ERR(znode)) {
>> +			err = PTR_ERR(znode);
>> +			goto out_unlock;
> 
> this out_unlock should also be.

Yep. Both are copy&paste errors. :-(

Thanks a lot for pointing this out, your help is much appreciated!

Thanks,
//richard