linux-kernel - Re: [PATCH] selinux: fix NULL dereference in policydb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 18 Mar 2019 12:29:48 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Ondrej Mosnacek <omosnace@...hat.com>
Cc:     selinux@...r.kernel.org, Stephen Smalley <sds@...ho.nsa.gov>,
        Kent Overstreet <kent.overstreet@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org,
        syzbot+a57b2aff60832666fc28@...kaller.appspotmail.com
Subject: Re: [PATCH] selinux: fix NULL dereference in policydb_destroy()

On Sun, Mar 17, 2019 at 9:47 AM Ondrej Mosnacek <omosnace@...hat.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@...kaller.appspotmail.com
> Fixes: acdf52d97f82 ("selinux: convert to kvmalloc")
> Signed-off-by: Ondrej Mosnacek <omosnace@...hat.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
>


-- 
paul moore
www.paul-moore.com