Re: [U-Boot] [PATCH v2 1/1] ubifs: avoid possible NULL dereference
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 - mount 'volume-name' volume => ubifsmount ubi0_5 Does not work on OneNAND: => ubifsmount ubi0_5 Error reading superblock on volume 'ubi0_5' errno=-22! ubifsmount - mount UBIFS volume Usage: ubifsmount - 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 - 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/1] ubifs: avoid possible NULL dereference
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 > > > Reported-by: Alex Sadovsky > > > Signed-off-by: Heinrich Schuchardt > > > --- > > > 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; -
Re: [U-Boot] [PATCH v2 1/1] ubifs: avoid possible NULL dereference
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 Reported-by: Alex Sadovsky Signed-off-by: Heinrich Schuchardt --- 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
Re: [U-Boot] [PATCH v2 1/1] ubifs: avoid possible NULL dereference
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 > Reported-by: Alex Sadovsky > Signed-off-by: Heinrich Schuchardt > --- > 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH v2 1/1] ubifs: avoid possible NULL dereference
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 Reported-by: Alex Sadovsky Signed-off-by: Heinrich Schuchardt --- 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot