* [PATCH] SELinux: Fix memory leak upon loading policy @ 2014-01-06 12:28 Tetsuo Handa 2014-01-06 22:12 ` Paul Moore 0 siblings, 1 reply; 6+ messages in thread From: Tetsuo Handa @ 2014-01-06 12:28 UTC (permalink / raw) To: eparis, jmorris, selinux; +Cc: linux-security-module 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@I-love.SAKURA.ne.jp> 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@I-love.SAKURA.ne.jp> --- 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; -- 1.7.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] SELinux: Fix memory leak upon loading policy 2014-01-06 12:28 [PATCH] SELinux: Fix memory leak upon loading policy Tetsuo Handa @ 2014-01-06 22:12 ` Paul Moore 2014-01-06 22:22 ` Eric Paris 0 siblings, 1 reply; 6+ messages in thread From: Paul Moore @ 2014-01-06 22:12 UTC (permalink / raw) To: Tetsuo Handa; +Cc: linux-security-module, selinux, eparis 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@I-love.SAKURA.ne.jp> > 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@I-love.SAKURA.ne.jp> > --- > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] SELinux: Fix memory leak upon loading policy 2014-01-06 22:12 ` Paul Moore @ 2014-01-06 22:22 ` Eric Paris 2014-01-06 22:54 ` Paul Moore 0 siblings, 1 reply; 6+ messages in thread From: Eric Paris @ 2014-01-06 22:22 UTC (permalink / raw) To: Paul Moore; +Cc: Tetsuo Handa, LSM List, SE-Linux, Eric Paris 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@redhat.com> On Mon, Jan 6, 2014 at 5:12 PM, Paul Moore <paul@paul-moore.com> 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@I-love.SAKURA.ne.jp> >> 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@I-love.SAKURA.ne.jp> >> --- >> 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] SELinux: Fix memory leak upon loading policy 2014-01-06 22:22 ` Eric Paris @ 2014-01-06 22:54 ` Paul Moore 2014-01-06 23:12 ` Eric Paris 0 siblings, 1 reply; 6+ messages in thread From: Paul Moore @ 2014-01-06 22:54 UTC (permalink / raw) To: Eric Paris, Tetsuo Handa; +Cc: LSM List, SE-Linux, Eric Paris 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@redhat.com> 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@paul-moore.com> 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@I-love.SAKURA.ne.jp> > >> 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@I-love.SAKURA.ne.jp> > >> --- > >> > >> 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@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] SELinux: Fix memory leak upon loading policy 2014-01-06 22:54 ` Paul Moore @ 2014-01-06 23:12 ` Eric Paris 2014-01-07 17:22 ` Paul Moore 0 siblings, 1 reply; 6+ messages in thread From: Eric Paris @ 2014-01-06 23:12 UTC (permalink / raw) To: Paul Moore; +Cc: Tetsuo Handa, LSM List, SE-Linux, Eric Paris 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@paul-moore.com> 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@redhat.com> > > 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@paul-moore.com> 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@I-love.SAKURA.ne.jp> >> >> 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@I-love.SAKURA.ne.jp> >> >> --- >> >> >> >> 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@vger.kernel.org >> > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > paul moore > www.paul-moore.com > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] SELinux: Fix memory leak upon loading policy 2014-01-06 23:12 ` Eric Paris @ 2014-01-07 17:22 ` Paul Moore 0 siblings, 0 replies; 6+ messages in thread From: Paul Moore @ 2014-01-07 17:22 UTC (permalink / raw) To: Eric Paris; +Cc: Tetsuo Handa, LSM List, SE-Linux, Eric Paris Merged the patch and everything passes the SELinux testsuite. I'll push this to James today. Thanks all. On Mon, Jan 6, 2014 at 6:12 PM, Eric Paris <eparis@parisplace.org> wrote: > 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@paul-moore.com> 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@redhat.com> >> >> 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@paul-moore.com> 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@I-love.SAKURA.ne.jp> >>> >> 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@I-love.SAKURA.ne.jp> >>> >> --- >>> >> >>> >> 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@vger.kernel.org >>> > More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> -- >> paul moore >> www.paul-moore.com >> -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-01-07 17:22 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-01-06 12:28 [PATCH] SELinux: Fix memory leak upon loading policy Tetsuo Handa 2014-01-06 22:12 ` Paul Moore 2014-01-06 22:22 ` Eric Paris 2014-01-06 22:54 ` Paul Moore 2014-01-06 23:12 ` Eric Paris 2014-01-07 17:22 ` Paul Moore
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.