* [U-Boot] [PATCH v2 1/1] ubifs: avoid possible NULL dereference @ 2017-11-21 22:06 Heinrich Schuchardt 2017-11-21 22:40 ` Ladislav Michl 0 siblings, 1 reply; 5+ messages in thread From: Heinrich Schuchardt @ 2017-11-21 22:06 UTC (permalink / raw) To: u-boot 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); - 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); - free(file); - free(dentry); free(dir); return ret; -- 2.11.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2 1/1] ubifs: avoid possible NULL dereference 2017-11-21 22:06 [U-Boot] [PATCH v2 1/1] ubifs: avoid possible NULL dereference Heinrich Schuchardt @ 2017-11-21 22:40 ` Ladislav Michl 2017-11-22 12:37 ` Heinrich Schuchardt 0 siblings, 1 reply; 5+ messages in thread From: Ladislav Michl @ 2017-11-21 22:40 UTC (permalink / raw) To: u-boot 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. > - 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2 1/1] ubifs: avoid possible NULL dereference 2017-11-21 22:40 ` Ladislav Michl @ 2017-11-22 12:37 ` Heinrich Schuchardt 2017-11-22 13:37 ` Ladislav Michl 0 siblings, 1 reply; 5+ messages in thread From: Heinrich Schuchardt @ 2017-11-22 12:37 UTC (permalink / raw) To: u-boot 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. 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2 1/1] ubifs: avoid possible NULL dereference 2017-11-22 12:37 ` Heinrich Schuchardt @ 2017-11-22 13:37 ` Ladislav Michl 2017-11-22 15:47 ` Ladislav Michl 0 siblings, 1 reply; 5+ messages in thread From: Ladislav Michl @ 2017-11-22 13:37 UTC (permalink / raw) To: u-boot 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); ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2 1/1] ubifs: avoid possible NULL dereference 2017-11-22 13:37 ` Ladislav Michl @ 2017-11-22 15:47 ` Ladislav Michl 0 siblings, 0 replies; 5+ messages in thread From: Ladislav Michl @ 2017-11-22 15:47 UTC (permalink / raw) To: u-boot On Wed, Nov 22, 2017 at 02:37:49PM +0100, Ladislav Michl wrote: > On Wed, Nov 22, 2017 at 01:37:54PM +0100, Heinrich Schuchardt wrote: [snip] > > 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)? [snip] Okay, just tried to test it. Does ubifsmount even work for you? I cannot mount volume neither using name nor id. Does not work on NAND: => ubi part UBI => ubifsmount ubi0:rootfs UBIFS assert failed in ubifs_change_lp at 540 UBIFS assert failed in ubifs_release_lprops at 278 UBIFS assert failed in ubifs_change_lp at 540 UBIFS assert failed in ubifs_release_lprops at 278 UBIFS assert failed in ubifs_change_lp at 540 UBIFS assert failed in ubifs_release_lprops at 278 UBIFS assert failed in ubifs_change_lp at 540 UBIFS assert failed in ubifs_release_lprops at 278 => ubifsmount ubi5 UBIFS assert failed in ubifs_change_lp at 540 UBIFS assert failed in ubifs_release_lprops at 278 UBIFS assert failed in ubifs_change_lp at 540 UBIFS assert failed in ubifs_release_lprops at 278 UBIFS assert failed in ubifs_change_lp at 540 UBIFS assert failed in ubifs_release_lprops at 278 UBIFS assert failed in ubifs_change_lp at 540 UBIFS assert failed in ubifs_release_lprops at 278 => ubifsmount ubi0_5 UBIFS error (pid: 1): cannot open "ubi0_5", error -19Error reading superblock on volume 'ubi0_5' errno=-19! ubifsmount - mount UBIFS volume Usage: ubifsmount <volume-name> - mount 'volume-name' volume => ubifsmount ubi0_5 <hangs here> Does not work on OneNAND: => ubifsmount ubi0_5 Error reading superblock on volume 'ubi0_5' errno=-22! ubifsmount - mount UBIFS volume Usage: ubifsmount <volume-name> - mount 'volume-name' volume => ubifsmount ubi0_5 UBIFS error (ubi0:5 pid 0): check_lpt_crc: invalid crc in LPT node: crc d514 calc 6695 UBIFS error (ubi0:5 pid 0): read_pnode: error -22 reading pnode at 8:16424 (pid 1) dumping pnode: address 9dfb2dc0 parent 9dfb2d40 cnext 0 flags 0 iip 1 level 0 num 0 0: free 0 dirty 97240 flags 1 lnum 0 1: free 57344 dirty 39752 flags 34 lnum 0 2: free 0 dirty 111296 flags 1 lnum 0 3: free 0 dirty 85952 flags 34 lnum 0 UBIFS error (ubi0:5 pid 0): read_pnode: calc num: 89 UBIFS assert failed in ubifs_release_lprops at 278 Error reading superblock on volume 'ubi0_5' errno=-22! ubifsmount - mount UBIFS volume Usage: ubifsmount <volume-name> - mount 'volume-name' volume See? It even gives different output for the same command. Tried every possible options to mount volume found in open_ubi... So given above unfortunate state, I'm considering fixing $subject mentioned a bit pointless as I'm not able to test it. I'll look at mount problems later. ladis ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-11-22 15:47 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-11-21 22:06 [U-Boot] [PATCH v2 1/1] ubifs: avoid possible NULL dereference Heinrich Schuchardt 2017-11-21 22:40 ` Ladislav Michl 2017-11-22 12:37 ` Heinrich Schuchardt 2017-11-22 13:37 ` Ladislav Michl 2017-11-22 15:47 ` Ladislav Michl
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox