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

[Ladislav Michl]

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

[Ladislav Michl]

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, , 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, >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;
-   

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

[Ladislav Michl]

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