| Message ID | 20190317134653.26824-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | selinux: fix NULL dereference in policydb_destroy() | expand |
On 3/17/19 9:46 AM, Ondrej Mosnacek wrote: > The conversion to kvmalloc() forgot to account for the possibility that > p->type_attr_map_array might be null in policydb_destroy(). > > Fix this by destroying its contents only if it is not NULL. > > Also make sure ebitmap_init() is called on all entries before > policydb_destroy() can be called. Right now this is a no-op, because > both kvcalloc() and ebitmap_init() just zero out the whole struct, but > let's rather not rely on a specific implementation. > > Reported-by: syzbot+a57b2aff60832666fc28@syzkaller.appspotmail.com > Fixes: acdf52d97f82 ("selinux: convert to kvmalloc") > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > --- > security/selinux/ss/policydb.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > NOTE: This applies directly on top of current Linus' tree, since the > problematic commit is not present in the selinux/stable-5.1 branch. > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > index 6b576e588725..daecdfb15a9c 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -828,9 +828,11 @@ void policydb_destroy(struct policydb *p) > hashtab_map(p->range_tr, range_tr_destroy, NULL); > hashtab_destroy(p->range_tr); > > - for (i = 0; i < p->p_types.nprim; i++) > - ebitmap_destroy(&p->type_attr_map_array[i]); > - kvfree(p->type_attr_map_array); > + if (p->type_attr_map_array) { > + for (i = 0; i < p->p_types.nprim; i++) > + ebitmap_destroy(&p->type_attr_map_array[i]); > + kvfree(p->type_attr_map_array); > + } > > ebitmap_destroy(&p->filename_trans_ttypes); > ebitmap_destroy(&p->policycaps); > @@ -2496,10 +2498,13 @@ int policydb_read(struct policydb *p, void *fp) > if (!p->type_attr_map_array) > goto bad; > > + /* just in case ebitmap_init() becomes more than just a memset(0): */ > + for (i = 0; i < p->p_types.nprim; i++) > + ebitmap_init(&p->type_attr_map_array[i]); > + > for (i = 0; i < p->p_types.nprim; i++) { > struct ebitmap *e = &p->type_attr_map_array[i]; > > - ebitmap_init(e); > if (p->policyvers >= POLICYDB_VERSION_AVTAB) { > rc = ebitmap_read(e, fp); > if (rc) >
On Sun, Mar 17, 2019 at 9:47 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > The conversion to kvmalloc() forgot to account for the possibility that > p->type_attr_map_array might be null in policydb_destroy(). > > Fix this by destroying its contents only if it is not NULL. > > Also make sure ebitmap_init() is called on all entries before > policydb_destroy() can be called. Right now this is a no-op, because > both kvcalloc() and ebitmap_init() just zero out the whole struct, but > let's rather not rely on a specific implementation. > > Reported-by: syzbot+a57b2aff60832666fc28@syzkaller.appspotmail.com > Fixes: acdf52d97f82 ("selinux: convert to kvmalloc") > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > security/selinux/ss/policydb.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > NOTE: This applies directly on top of current Linus' tree, since the > problematic commit is not present in the selinux/stable-5.1 branch. Thanks for fixing this; I was busy getting the libseccomp v2.4 release out towards the end of last week and hadn't had a chance to look at this yet. The patch looks good to me. I just merged it into selinux/stable-5.1 and I'll send that up to Linus later this week once I've finished merging/testing stuff that was waiting on the merge window to close. For those on the To/CC line who haven't followed this very closely; the kvmalloc() patches were posted a while ago, but I never merged the SELinux portions as I there were some concerns brought up that were never addressed (concerns around small systems and difficulty in an RCU conversion). Evidently the higher ups felt these concerns were not significant enough and they merged the kvmalloc() changes anyway; this is why a non-trivial SELinux patch ended up in Linus' tree without going through the SELinux tree. I'm not sure I feel strongly enough about the kvmalloc() change and the objections at this point to object to the kvmalloc() conversion now that it is in Linus' tree, so this is really just a FYI. > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > index 6b576e588725..daecdfb15a9c 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -828,9 +828,11 @@ void policydb_destroy(struct policydb *p) > hashtab_map(p->range_tr, range_tr_destroy, NULL); > hashtab_destroy(p->range_tr); > > - for (i = 0; i < p->p_types.nprim; i++) > - ebitmap_destroy(&p->type_attr_map_array[i]); > - kvfree(p->type_attr_map_array); > + if (p->type_attr_map_array) { > + for (i = 0; i < p->p_types.nprim; i++) > + ebitmap_destroy(&p->type_attr_map_array[i]); > + kvfree(p->type_attr_map_array); > + } > > ebitmap_destroy(&p->filename_trans_ttypes); > ebitmap_destroy(&p->policycaps); > @@ -2496,10 +2498,13 @@ int policydb_read(struct policydb *p, void *fp) > if (!p->type_attr_map_array) > goto bad; > > + /* just in case ebitmap_init() becomes more than just a memset(0): */ > + for (i = 0; i < p->p_types.nprim; i++) > + ebitmap_init(&p->type_attr_map_array[i]); > + > for (i = 0; i < p->p_types.nprim; i++) { > struct ebitmap *e = &p->type_attr_map_array[i]; > > - ebitmap_init(e); > if (p->policyvers >= POLICYDB_VERSION_AVTAB) { > rc = ebitmap_read(e, fp); > if (rc) > -- > 2.20.1 >
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 6b576e588725..daecdfb15a9c 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -828,9 +828,11 @@ void policydb_destroy(struct policydb *p) hashtab_map(p->range_tr, range_tr_destroy, NULL); hashtab_destroy(p->range_tr); - for (i = 0; i < p->p_types.nprim; i++) - ebitmap_destroy(&p->type_attr_map_array[i]); - kvfree(p->type_attr_map_array); + if (p->type_attr_map_array) { + for (i = 0; i < p->p_types.nprim; i++) + ebitmap_destroy(&p->type_attr_map_array[i]); + kvfree(p->type_attr_map_array); + } ebitmap_destroy(&p->filename_trans_ttypes); ebitmap_destroy(&p->policycaps); @@ -2496,10 +2498,13 @@ int policydb_read(struct policydb *p, void *fp) if (!p->type_attr_map_array) goto bad; + /* just in case ebitmap_init() becomes more than just a memset(0): */ + for (i = 0; i < p->p_types.nprim; i++) + ebitmap_init(&p->type_attr_map_array[i]); + for (i = 0; i < p->p_types.nprim; i++) { struct ebitmap *e = &p->type_attr_map_array[i]; - ebitmap_init(e); if (p->policyvers >= POLICYDB_VERSION_AVTAB) { rc = ebitmap_read(e, fp); if (rc)
The conversion to kvmalloc() forgot to account for the possibility that p->type_attr_map_array might be null in policydb_destroy(). Fix this by destroying its contents only if it is not NULL. Also make sure ebitmap_init() is called on all entries before policydb_destroy() can be called. Right now this is a no-op, because both kvcalloc() and ebitmap_init() just zero out the whole struct, but let's rather not rely on a specific implementation. Reported-by: syzbot+a57b2aff60832666fc28@syzkaller.appspotmail.com Fixes: acdf52d97f82 ("selinux: convert to kvmalloc") Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- security/selinux/ss/policydb.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) NOTE: This applies directly on top of current Linus' tree, since the problematic commit is not present in the selinux/stable-5.1 branch.