[U-Boot] [PATCH v2 1/1] ubifs: avoid possible NULL dereference

[Heinrich Schuchardt]

On 11/21/2017 11:40 PM, Ladislav Michl wrote:
> On Tue, Nov 21, 2017 at 11:06:35PM +0100, Heinrich Schuchardt wrote:
>> If 'file' cannot be allocated due to an out of memory
>> situation, NULL is dereferenced.
>>
>> Variables file and dentry are not needed at all.
>> So let's eliminate them.
>>
>> When debugging this patch also avoids a misleading message
>> "cannot find next direntry, error %d" in case of an out of
>> memory situation. It is sufficent to write
>> "%s: Error, no memory for malloc!\n" in this case.
>>
>> Reported-by: Ladislav Michl <ladis at linux-mips.org>
>> Reported-by: Alex Sadovsky <nable.maininbox at googlemail.com>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>> ---
>>   fs/ubifs/ubifs.c | 25 ++-----------------------
>>   1 file changed, 2 insertions(+), 23 deletions(-)
>>
>> diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
>> index 4465523d5f..f3d190c763 100644
>> --- a/fs/ubifs/ubifs.c
>> +++ b/fs/ubifs/ubifs.c
>> @@ -393,29 +393,18 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
>>   	union ubifs_key key;
>>   	struct ubifs_dent_node *dent;
>>   	struct ubifs_info *c;
>> -	struct file *file;
>> -	struct dentry *dentry;
>>   	struct inode *dir;
>>   	int ret = 0;
>>   
>> -	file = kzalloc(sizeof(struct file), 0);
>> -	dentry = kzalloc(sizeof(struct dentry), 0);
>>   	dir = kzalloc(sizeof(struct inode), 0);
>> -	if (!file || !dentry || !dir) {
>> +	if (!dir) {
>>   		printf("%s: Error, no memory for malloc!\n", __func__);
>> -		err = -ENOMEM;
>> -		goto out;
>> +		goto out_free;
>>   	}
>>   
>>   	dir->i_sb = sb;
>> -	file->f_path.dentry = dentry;
>> -	file->f_path.dentry->d_parent = dentry;
>> -	file->f_path.dentry->d_inode = dir;
>> -	file->f_path.dentry->d_inode->i_ino = root_inum;
>>   	c = sb->s_fs_info;
>>   
>> -	dbg_gen("dir ino %lu, f_pos %#llx", dir->i_ino, file->f_pos);
>> -
>>   	/* Find the first entry in TNC and save it */
>>   	lowest_dent_key(c, &key, dir->i_ino);
>>   	nm.name = NULL;
>> @@ -425,9 +414,6 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
>>   		goto out;
>>   	}
>>   
>> -	file->f_pos = key_hash_flash(c, &dent->key);
>> -	file->private_data = dent;
>> -
>>   	while (1) {
>>   		dbg_gen("feed '%s', ino %llu, new f_pos %#x",
>>   			dent->name, (unsigned long long)le64_to_cpu(dent->inum),
>> @@ -450,10 +436,6 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
>>   			err = PTR_ERR(dent);
>>   			goto out;
>>   		}
>> -
>> -		kfree(file->private_data);
> 
> We still need to kfree allocated 'dent' as it was previously allocated:
> dent = kmalloc(zbr->len, GFP_NOFS);
> in ubifs_tnc_next_ent.

I agree that there is a memory leak. But we should put fixing that into 
a separate patch so that we can test both modifications separately.

It is not enough to kfree(dent).
ubifs_tnc_next_ent may return ERR_PTR(err) and we do not want to pass 
this value to kfree.

As Wolfgang wrote we should pass error codes to the calling chain of 
ubifs_finddir(), i.e. ubifs_findfile(), ubifs_size(), ubifs_read, 
ubifs_exists(), ubifs_ls(), ...

The code also lacks support for the driver model.

So a lot of other patches needed.


If you think this patch fixes what it promises to fix, please, add your 
review comment.

Best regards

Heinrich

> 
>> -		file->f_pos = key_hash_flash(c, &dent->key);
>> -		file->private_data = dent;
>>   		cond_resched();
>>   	}
>>   
>> @@ -462,9 +444,6 @@ out:
>>   		dbg_gen("cannot find next direntry, error %d", err);
>>   
>>   out_free:
>> -	kfree(file->private_data);
> 
> Same as above.
> 
>> -	free(file);
>> -	free(dentry);
>>   	free(dir);
>>   
>>   	return ret;
>> -- 
>> 2.11.0
>