Patchwork UBIFS: add missing znode freeing in tcn_insert()

login
register
mail settings
Submitter Florian Fainelli
Date March 8, 2014, 12:11 a.m.
Message ID <1394237487-2728-1-git-send-email-f.fainelli@gmail.com>
Download mbox | patch
Permalink /patch/328127/
State New
Headers show

Comments

Florian Fainelli - March 8, 2014, 12:11 a.m.
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(-)
richard -rw- weinberger - March 8, 2014, 10:46 a.m.
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.
Florian Fainelli - March 10, 2014, 6:44 p.m.
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.
Artem Bityutskiy - March 11, 2014, 7:55 a.m.
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.

Patch

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;