[PATCH] UBIFS: add missing znode freeing in tcn_insert()

[PATCH] UBIFS: add missing znode freeing in tcn_insert()

[Florian Fainelli]

In case the zi allocation fails in the do_split label, we will fail
freeing zn that we allocated before, add a missing kfree.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 fs/ubifs/tnc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c
index 9083bc7ed4ae..9b84d91ea530 100644
--- a/fs/ubifs/tnc.c
+++ b/fs/ubifs/tnc.c
@@ -2105,8 +2105,10 @@ do_split:
 	dbg_tnc("creating new zroot at level %d", znode->level + 1);
 
 	zi = kzalloc(c->max_znode_sz, GFP_NOFS);
-	if (!zi)
+	if (!zi) {
+		kfree(zn);
 		return -ENOMEM;
+	}
 
 	zi->child_cnt = 2;
 	zi->level = znode->level + 1;
-- 
1.8.3.2

Re: [PATCH] UBIFS: add missing znode freeing in tcn_insert()

[Richard Weinberger]

On Sat, Mar 8, 2014 at 1:11 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> In case the zi allocation fails in the do_split label, we will fail
> freeing zn that we allocated before, add a missing kfree.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  fs/ubifs/tnc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c
> index 9083bc7ed4ae..9b84d91ea530 100644
> --- a/fs/ubifs/tnc.c
> +++ b/fs/ubifs/tnc.c
> @@ -2105,8 +2105,10 @@ do_split:
>         dbg_tnc("creating new zroot at level %d", znode->level + 1);
>
>         zi = kzalloc(c->max_znode_sz, GFP_NOFS);
> -       if (!zi)
> +       if (!zi) {
> +               kfree(zn);
>                 return -ENOMEM;
> +       }
>

I'm not sure whether this is correct.

Around line 2050 we have:
... else {
                /* Insert into new znode */
                zi = zn; <---------
                n -= keep;
                /* Re-parent */
                if (zn->level != 0)
                        zbr->znode->parent = zn;
        }

And later:
insert_zbranch(zi, zbr, n);

By freeing zn you may introduce a use after free bug.

-- 
Thanks,
//richard

Re: [PATCH] UBIFS: add missing znode freeing in tcn_insert()

[Florian Fainelli]

2014-03-08 2:46 GMT-08:00 Richard Weinberger <richard.weinberger@gmail.com>:
> On Sat, Mar 8, 2014 at 1:11 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> In case the zi allocation fails in the do_split label, we will fail
>> freeing zn that we allocated before, add a missing kfree.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  fs/ubifs/tnc.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c
>> index 9083bc7ed4ae..9b84d91ea530 100644
>> --- a/fs/ubifs/tnc.c
>> +++ b/fs/ubifs/tnc.c
>> @@ -2105,8 +2105,10 @@ do_split:
>>         dbg_tnc("creating new zroot at level %d", znode->level + 1);
>>
>>         zi = kzalloc(c->max_znode_sz, GFP_NOFS);
>> -       if (!zi)
>> +       if (!zi) {
>> +               kfree(zn);
>>                 return -ENOMEM;
>> +       }
>>
>
> I'm not sure whether this is correct.
>
> Around line 2050 we have:
> ... else {
>                 /* Insert into new znode */
>                 zi = zn; <---------
>                 n -= keep;
>                 /* Re-parent */
>                 if (zn->level != 0)
>                         zbr->znode->parent = zn;
>         }
>
> And later:
> insert_zbranch(zi, zbr, n);
>
> By freeing zn you may introduce a use after free bug.

Yes, you are right, I cannot find a good way to make sure this is not
the case, variables re-use in this function makes it particularly hard
to read BTW.
-- 
Florian

Re: [PATCH] UBIFS: add missing znode freeing in tcn_insert()

[Artem Bityutskiy]

On Mon, 2014-03-10 at 11:44 -0700, Florian Fainelli wrote:
> 2014-03-08 2:46 GMT-08:00 Richard Weinberger <richard.weinberger@gmail.com>:
> > On Sat, Mar 8, 2014 at 1:11 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> >> In case the zi allocation fails in the do_split label, we will fail
> >> freeing zn that we allocated before, add a missing kfree.
> >>
> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >> ---
> >>  fs/ubifs/tnc.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c
> >> index 9083bc7ed4ae..9b84d91ea530 100644
> >> --- a/fs/ubifs/tnc.c
> >> +++ b/fs/ubifs/tnc.c
> >> @@ -2105,8 +2105,10 @@ do_split:
> >>         dbg_tnc("creating new zroot at level %d", znode->level + 1);
> >>
> >>         zi = kzalloc(c->max_znode_sz, GFP_NOFS);
> >> -       if (!zi)
> >> +       if (!zi) {
> >> +               kfree(zn);
> >>                 return -ENOMEM;
> >> +       }
> >>
> >
> > I'm not sure whether this is correct.
> >
> > Around line 2050 we have:
> > ... else {
> >                 /* Insert into new znode */
> >                 zi = zn; <---------
> >                 n -= keep;
> >                 /* Re-parent */
> >                 if (zn->level != 0)
> >                         zbr->znode->parent = zn;
> >         }
> >
> > And later:
> > insert_zbranch(zi, zbr, n);
> >
> > By freeing zn you may introduce a use after free bug.
> 
> Yes, you are right, I cannot find a good way to make sure this is not
> the case,

I think you do not need to free it. It was allocated and inserted to
TNC. TNC is a global data structure. It gets destroyed (all elements get
freed) when the FS is unmounted. Or if mount fails, it gets freed on the
error path.

So no need to worry about that element.

>  variables re-use in this function makes it particularly hard
> to read BTW.

Yes, it could be better, I guess. But this kind of "algorithmic" code is
rarely very readable, it is just not very easy. More comments could help
too.

-- 
Best Regards,
Artem Bityutskiy