| Message ID | 20190725105243.28404-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | selinux: fix memory leak in policydb_init() | expand |
On Thu, Jul 25, 2019 at 6:52 AM Ondrej Mosnacek <omosnace@redhat.com> 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. > > Reported-by: syzbot+fee3a14d4cdf92646287@syzkaller.appspotmail.com > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > security/selinux/ss/policydb.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > index daecdfb15a9c..38d0083204f1 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -274,6 +274,8 @@ static int rangetr_cmp(struct hashtab *h, const void *k1, const void *k2) > return v; > } > > +static int (*destroy_f[SYM_NUM]) (void *key, void *datum, void *datap); I'm prefer not to use forward declarations if they can be avoided, and in this particular case it looks like we can move the *_destroy functions up just below policydb_lookup_compat() and avoid the forward declaration while keeping some sanity to the layout of the file. Yes, the patch does become much larger (378 lines changed in the test patch I just did), but I think the end result is cleaner. > /* > * Initialize a policy database structure. > */ > @@ -321,8 +323,10 @@ static int policydb_init(struct policydb *p) > out: > hashtab_destroy(p->filename_trans); > hashtab_destroy(p->range_tr); > - for (i = 0; i < SYM_NUM; i++) > + for (i = 0; i < SYM_NUM; i++) { > + hashtab_map(p->symtab[i].table, destroy_f[i], NULL); > hashtab_destroy(p->symtab[i].table); > + } > return rc; > } > > -- > 2.21.0 >
On Sat, Jul 27, 2019 at 12:27 AM Paul Moore <paul@paul-moore.com> wrote: > On Thu, Jul 25, 2019 at 6:52 AM Ondrej Mosnacek <omosnace@redhat.com> 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. > > > > Reported-by: syzbot+fee3a14d4cdf92646287@syzkaller.appspotmail.com > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > --- > > security/selinux/ss/policydb.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > > index daecdfb15a9c..38d0083204f1 100644 > > --- a/security/selinux/ss/policydb.c > > +++ b/security/selinux/ss/policydb.c > > @@ -274,6 +274,8 @@ static int rangetr_cmp(struct hashtab *h, const void *k1, const void *k2) > > return v; > > } > > > > +static int (*destroy_f[SYM_NUM]) (void *key, void *datum, void *datap); > > I'm prefer not to use forward declarations if they can be avoided, and > in this particular case it looks like we can move the *_destroy > functions up just below policydb_lookup_compat() and avoid the forward > declaration while keeping some sanity to the layout of the file. > > Yes, the patch does become much larger (378 lines changed in the test > patch I just did), but I think the end result is cleaner. OK, that'ts reasonable. I'll post an updated v2. > > > /* > > * Initialize a policy database structure. > > */ > > @@ -321,8 +323,10 @@ static int policydb_init(struct policydb *p) > > out: > > hashtab_destroy(p->filename_trans); > > hashtab_destroy(p->range_tr); > > - for (i = 0; i < SYM_NUM; i++) > > + for (i = 0; i < SYM_NUM; i++) { > > + hashtab_map(p->symtab[i].table, destroy_f[i], NULL); > > hashtab_destroy(p->symtab[i].table); > > + } > > return rc; > > } > > > > -- > > 2.21.0 > > > > > -- > paul moore > www.paul-moore.com
On Thu, Jul 25, 2019 at 6:52 AM Ondrej Mosnacek <omosnace@redhat.com> 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. > > Reported-by: syzbot+fee3a14d4cdf92646287@syzkaller.appspotmail.com > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > security/selinux/ss/policydb.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) As discussed in the v2 thread, I've gone ahead and merge this patch into selinux/stable-5.3. Assuming all is well I'll send this to Linus later this week.
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index daecdfb15a9c..38d0083204f1 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -274,6 +274,8 @@ static int rangetr_cmp(struct hashtab *h, const void *k1, const void *k2) return v; } +static int (*destroy_f[SYM_NUM]) (void *key, void *datum, void *datap); + /* * Initialize a policy database structure. */ @@ -321,8 +323,10 @@ static int policydb_init(struct policydb *p) out: hashtab_destroy(p->filename_trans); hashtab_destroy(p->range_tr); - for (i = 0; i < SYM_NUM; i++) + for (i = 0; i < SYM_NUM; i++) { + hashtab_map(p->symtab[i].table, destroy_f[i], NULL); hashtab_destroy(p->symtab[i].table); + } return rc; }
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. Reported-by: syzbot+fee3a14d4cdf92646287@syzkaller.appspotmail.com Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- security/selinux/ss/policydb.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)