Re: [PATCH] selinux: fix NULL dereference in policydb_destroy()

[Paul Moore]

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