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

[Ladislav Michl <ladis@linux-mips.org>]

On Wed, Nov 22, 2017 at 01:37:54PM +0100, Heinrich Schuchardt wrote:
> 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@linux-mips.org>
> > > Reported-by: Alex Sadovsky <nable.maininbox@googlemail.com>
> > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@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.

There was no such memory leak before above patch.

> 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.

Nobody is claiming otherwise.

> 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.

Yes, but fix should not add another bug.

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

What about (untested)?

diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
index 4465523d5f..64188d9f2d 100644
--- a/fs/ubifs/ubifs.c
+++ b/fs/ubifs/ubifs.c
@@ -388,47 +388,33 @@ out:
 static int ubifs_finddir(struct super_block *sb, char *dirname,
 			 unsigned long root_inum, unsigned long *inum)
 {
-	int err;
+	int err = 0;
 	struct qstr nm;
 	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;
+		return -ENOMEM;
 	}
 
 	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;
-	dent = ubifs_tnc_next_ent(c, &key, &nm);
-	if (IS_ERR(dent)) {
-		err = PTR_ERR(dent);
-		goto out;
-	}
 
-	file->f_pos = key_hash_flash(c, &dent->key);
-	file->private_data = dent;
+	while (!err) {
+		dent = ubifs_tnc_next_ent(c, &key, &nm);
+		if (IS_ERR(dent)) {
+			err = PTR_ERR(dent);
+			break;
+		}
 
-	while (1) {
 		dbg_gen("feed '%s', ino %llu, new f_pos %#x",
 			dent->name, (unsigned long long)le64_to_cpu(dent->inum),
 			key_hash_flash(c, &dent->key));
@@ -438,36 +424,23 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
 		if ((strncmp(dirname, (char *)dent->name, nm.len) == 0) &&
 		    (strlen(dirname) == nm.len)) {
 			*inum = le64_to_cpu(dent->inum);
-			ret = 1;
-			goto out_free;
-		}
-
-		/* Switch to the next entry */
-		key_read(c, &dent->key, &key);
-		nm.name = (char *)dent->name;
-		dent = ubifs_tnc_next_ent(c, &key, &nm);
-		if (IS_ERR(dent)) {
-			err = PTR_ERR(dent);
-			goto out;
+			err = 1;
+		} else {
+			/* Switch to the next entry */
+			key_read(c, &dent->key, &key);
+			nm.name = (char *)dent->name;
 		}
+		kfree(dent);
 
-		kfree(file->private_data);
-		file->f_pos = key_hash_flash(c, &dent->key);
-		file->private_data = dent;
 		cond_resched();
 	}
 
-out:
-	if (err != -ENOENT)
+	if (err != -ENOENT && err != 1)
 		dbg_gen("cannot find next direntry, error %d", err);
 
-out_free:
-	kfree(file->private_data);
-	free(file);
-	free(dentry);
-	free(dir);
+	kfree(dir);
 
-	return ret;
+	return err;
 }
 
 static unsigned long ubifs_findfile(struct super_block *sb, char *filename)
@@ -508,7 +481,7 @@ static unsigned long ubifs_findfile(struct super_block *sb, char *filename)
 		}
 
 		ret = ubifs_finddir(sb, name, root_inum, &inum);
-		if (!ret)
+		if (ret != 1)
 			return 0;
 		inode = ubifs_iget(sb, inum);