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>
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
_______________________________________________
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.
