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.
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.
--
paul moore
www.paul-moore.com
