Re: [PATCH v2 1/3] selinux: policydb - fix memory leak in policydb_init()

[Ondrej Mosnacek <omosnace@redhat.com>]

On Tue, Jul 30, 2019 at 5:10 PM Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jul 30, 2019 at 8:20 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Tue, Jul 30, 2019 at 12:48 AM Paul Moore <paul@paul-moore.com> wrote:
> > > On Mon, Jul 29, 2019 at 4:41 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > >
> > > > Since roles_init() adds some entries to the role hash table, we need to
> > > > destroy also its keys/values on error, otherwise we get a memory leak in
> > > > the error path.
> > > >
> > > > To avoid a forward declaration and maintain a sane layout, move all the
> > > > destroy stuff above policydb_init. No changes are made to the moved code
> > > > in this patch. Note that this triggers some pre-existing checkpatch.pl
> > > > warnings - these will be fixed in follow-up patches.
> > > >
> > > > Reported-by: syzbot+fee3a14d4cdf92646287@syzkaller.appspotmail.com
> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > > ---
> > > >  security/selinux/ss/policydb.c | 976 +++++++++++++++++----------------
> > > >  1 file changed, 489 insertions(+), 487 deletions(-)
> > >
> > > Hmmm, that is one ugly patch isn't it?  If I saw this diff I'm not
> > > sure I would have suggested what I did, or rather I would have
> > > suggested something slightly different.
> > >
> > > When I ran my quick test when I was looking at your v1 patch, I only
> > > moved perm_destroy() through ocontext_destroy(), leaving out
> > > policydb_destroy(), and the diff was much more cleaner[*] (diffstat
> > > below, includes the actual fix too).  Could you try that and see if it
> > > cleans up your patch?
> >
> > Yeah, excluding policydb_destroy() from the move is what's needed to
> > get a nice patch...
>
> Good, let's just do that.
>
> > Actually, what do you think about keeping the
> > bugfix patch as before (with the forward declaration) and then doing
> > the moving around in a separate patch (removing the forward
> > declaration)?
>
> Yes, I thought about that too when looking at your patch yesterday and
> trying to sort out why it was such a messy diff.
>
> > Then we keep the patch with the actual fix small, but
> > still get a clean final result. It would also allow moving
> > policydb_destroy() up closer to the other destroy functions in another
> > separate patch (I tried it and both patches end up clean when the move
> > is split up like this). (I don't have a strong preference for this,
> > let me know what works best for you.)
>
> I'm fine with leaving policydb_destroy() where it is, but I agree that
> separating the fix is likely worthwhile.  I'll go ahead and merge your
> v1 patch into selinux/stable-5.3 (it's borderline -stable material
> IMHO, but I'm pretty sure GregKH would pull it into -stable anyway, he
> pulls everything with a "Fixes" tag it seems), and then merge the
> reorganization patch into selinux/next.  Honestly, I can go ahead and
> submit the reorg patch, it's basically already sitting in a tree on my
> disk anyway, but if you would prefer to do it that's fine too, just
> let me know.

Sure, feel free to submit the reorg yourself (I assume you will then
merge the checkpatch fixes 2-3/3 on top, right?)

>
> I'll may also merge the v1 fix into selinux/next in order to fix the
> inevitable merge conflict, but that isn't something you have to worry
> about.

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.