Re: [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 <la...@linux-mips.org>
Reported-by: Alex Sadovsky <nable.mainin...@googlemail.com>
Signed-off-by: Heinrich Schuchardt <xypron.g...@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

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot