On Tue, Jul 30, 2019 at 12:15 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> On Tue, Jul 30, 2019 at 5:10 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > On Tue, Jul 30, 2019 at 8:20 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> > > On Tue, Jul 30, 2019 at 12:48 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > > On Mon, Jul 29, 2019 at 4:41 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > > Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> > > > > ---
> > > > > 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?)
Yep, that's my plan (I should have mentioned that in the previous email).
--
paul moore
www.paul-moore.com
