[U-Boot] [PATCH v2 1/1] ubifs: avoid possible NULL dereference
Ladislav Michl
ladis at linux-mips.org
Wed Nov 22 13:37:49 UTC 2017
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 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.
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);
More information about the U-Boot
mailing list