| Message ID | 1394237487-2728-1-git-send-email-f.fainelli@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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.
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.
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.
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;
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(-)