On Monday, January 06, 2014 05:22:20 PM Eric Paris wrote:
> Paul,
>
> I do not understand your objection. While we believe that userspace
> should not be sending multiple copies of the same filename trans rule
> we know it is, we know it isn't fatal, and we know we are leaking
> memory. I think this patch is doing the right thing. On EEXIST it is
> freeing the memory and moving along (instead of leaking it and moving
> along). On any other error it will cause the policy load to fail. I
> can't think of any kernel patch which makes more sense. We know these
> policies exist. Sure, we should fix the EEXIST in the toolchain, but
> we should also stop leaking memory and not break anything.
>
> Acked-by: Eric Paris <eparis@xxxxxxxxxx>
My concern/objection was that I wasn't confident the core problem was with the
toolchain; I wanted to make sure we weren't covering up a kernel bug. Since
you're confident this is a toolchain issue I'll go ahead and merge this.
As soon as I can get this build and tested, I'll push this up to James. I'm
thinking we should be able to make it before the 3.14 merge window and I also
think this is a good stable candidate.
> On Mon, Jan 6, 2014 at 5:12 PM, Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > On Monday, January 06, 2014 09:28:15 PM Tetsuo Handa wrote:
> >> Hello.
> >>
> >> I got below leak with linux-3.10.0-54.0.1.el7.x86_64 .
> >>
> >> [ 681.903890] kmemleak: 5538 new suspected memory leaks (see
> >> /sys/kernel/debug/kmemleak)
> >>
> >> Below is a patch, but I don't know whether we need special handing for
> >> undoing ebitmap_set_bit() call.
> >> ----------
> >>
> >> >From fe97527a90fe95e2239dfbaa7558f0ed559c0992 Mon Sep 17 00:00:00 2001
> >>
> >> From: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> >> Date: Mon, 6 Jan 2014 16:30:21 +0900
> >> Subject: [PATCH] SELinux: Fix memory leak upon loading policy
> >>
> >> Commit 2463c26d "SELinux: put name based create rules in a hashtable" did
> >> not check return value from hashtab_insert() in filename_trans_read(). It
> >> leaks memory if hashtab_insert() returns error.
> >>
> >> unreferenced object 0xffff88005c9160d0 (size 8):
> >> comm "systemd", pid 1, jiffies 4294688674 (age 235.265s)
> >>
> >> hex dump (first 8 bytes):
> >> 57 0b 00 00 6b 6b 6b a5 W...kkk.
> >>
> >> backtrace:
> >> [<ffffffff816604ae>] kmemleak_alloc+0x4e/0xb0
> >> [<ffffffff811cba5e>] kmem_cache_alloc_trace+0x12e/0x360
> >> [<ffffffff812aec5d>] policydb_read+0xd1d/0xf70
> >> [<ffffffff812b345c>] security_load_policy+0x6c/0x500
> >> [<ffffffff812a623c>] sel_write_load+0xac/0x750
> >> [<ffffffff811eb680>] vfs_write+0xc0/0x1f0
> >> [<ffffffff811ec08c>] SyS_write+0x4c/0xa0
> >> [<ffffffff81690419>] system_call_fastpath+0x16/0x1b
> >> [<ffffffffffffffff>] 0xffffffffffffffff
> >>
> >> However, we should not return EEXIST error to the caller, or the systemd
> >> will show below message and the boot sequence freezes.
> >>
> >> systemd[1]: Failed to load SELinux policy. Freezing.
> >>
> >> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> >> ---
> >>
> >> security/selinux/ss/policydb.c | 14 +++++++++++++-
> >> 1 files changed, 13 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/security/selinux/ss/policydb.c
> >> b/security/selinux/ss/policydb.c index f6195eb..c8985db 100644
> >> --- a/security/selinux/ss/policydb.c
> >> +++ b/security/selinux/ss/policydb.c
> >> @@ -1941,7 +1941,19 @@ static int filename_trans_read(struct policydb *p,
> >> void *fp) if (rc)
> >>
> >> goto out;
> >>
> >> - hashtab_insert(p->filename_trans, ft, otype);
> >> + rc = hashtab_insert(p->filename_trans, ft, otype);
> >> + if (rc) {
> >> + /*
> >> + * Do not return -EEXIST to the caller, or the
> >> system
> >> + * will not boot.
> >> + */
> >> + if (rc != -EEXIST)
> >> + goto out;
> >> + /* But free memory to avoid memory leak. */
> >> + kfree(ft);
> >> + kfree(name);
> >> + kfree(otype);
> >> + }
> >>
> >> }
> >> hash_eval(p->filename_trans, "filenametr");
> >> return 0;
> >
> > Thanks for sending this patch. Mimi found a similar issue a little while
> > ago and found a number of hashtab_insert() failures which were causing a
> > fair number of memory leaks.
> >
> > I'm not going to merge this patch at the moment. While we should check
> > the
> > return value of hashtab_insert(), and fix the memory leaks when it fails,
> > I
> > still want to better understand why that hashtab_insert() function is
> > failing in the first place on what would appear to be a "normal"
> > operation. Ideally we would first resolve the hashtab_insert() failures,
> > then we could check the return value, release the memory when necessary,
> > and return errors to the caller.
> >
> > --
> > paul moore
> > www.paul-moore.com
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-security-module" in the body of a message to
> > majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
paul moore
www.paul-moore.com
_______________________________________________
Selinux mailing list
Selinux@xxxxxxxxxxxxx
To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx.
To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.
