The only way to get EEXIST is for h->keycmp(h, new_entry,
existing_entry) to be 0. keycmp == filenametr_cmp() which does:
static int filenametr_cmp(struct hashtab *h, const void *k1, const void *k2)
{
const struct filename_trans *ft1 = k1;
const struct filename_trans *ft2 = k2;
int v;
v = ft1->stype - ft2->stype;
if (v)
return v;
v = ft1->ttype - ft2->ttype;
if (v)
return v;
v = ft1->tclass - ft2->tclass;
if (v)
return v;
return strcmp(ft1->name, ft2->name);
}
so we know for a fact stype, ttype, tclass, and filename are all the
same. We don't know if the otype is the same. But if it is, that's a
toolchain problem. It should not have let things through.
I know we do duplicate checking at expand time and the code looks ok
to me... I dunno, but there is a real bug here somewhere.
This patch does the right thing by returning errors on error, except
for the known/currently working EEXIST case. Then we don't leak or
destroy user's systems that we know exist...
On Mon, Jan 6, 2014 at 5:54 PM, Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> 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.
