linux-kernel - Re: [PATCH v3 07/14] selinux: check type attr map overflows

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEjxPJ4G_0m8gM5QyJ7UZmmRut6p46O0qAihDbsJ3U-N5dDKqA@mail.gmail.com>
Date: Wed, 14 May 2025 14:15:46 -0400
From: Stephen Smalley <stephen.smalley.work@...il.com>
To: cgzones@...glemail.com
Cc: selinux@...r.kernel.org, Paul Moore <paul@...l-moore.com>, 
	Ondrej Mosnacek <omosnace@...hat.com>, linux-kernel@...r.kernel.org, 
	Eric Suen <ericsu@...ux.microsoft.com>, Canfeng Guo <guocanfeng@...ontech.com>
Subject: Re: [PATCH v3 07/14] selinux: check type attr map overflows

On Sun, May 11, 2025 at 1:31 PM Christian Göttsche
<cgoettsche@...tendoof.de> wrote:
>
> From: Christian Göttsche <cgzones@...glemail.com>
>
> Validate that no types with an invalid too high ID are present in the
> attribute map.  Gaps are still not checked.
>
> Signed-off-by: Christian Göttsche <cgzones@...glemail.com>

Acked-by: Stephen Smalley <stephen.smalley.work@...il.com>

But see my commentary below.

> ---
> v3: squash with previous patch ("selinux: introduce
>     ebitmap_highest_set_bit()")
> ---
>  security/selinux/ss/ebitmap.c  | 27 +++++++++++++++++++++++++++
>  security/selinux/ss/ebitmap.h  |  1 +
>  security/selinux/ss/policydb.c |  5 +++++
>  3 files changed, 33 insertions(+)
>
> diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
> index 43bc19e21960..5d6b5b72b3e5 100644
> --- a/security/selinux/ss/ebitmap.c
> +++ b/security/selinux/ss/ebitmap.c
> @@ -257,6 +257,33 @@ int ebitmap_contains(const struct ebitmap *e1, const struct ebitmap *e2,
>         return 1;
>  }
>
> +u32 ebitmap_highest_set_bit(const struct ebitmap *e)
> +{
> +       const struct ebitmap_node *n;
> +       unsigned long unit;
> +       u32 pos = 0;
> +
> +       n = e->node;
> +       if (!n)
> +               return 0;
> +
> +       while (n->next)
> +               n = n->next;
> +
> +       for (unsigned int i = EBITMAP_UNIT_NUMS; i > 0; i--) {
> +               unit = n->maps[i - 1];
> +               if (unit == 0)
> +                       continue;
> +
> +               pos = (i - 1) * EBITMAP_UNIT_SIZE;
> +               while (unit >>= 1)
> +                       pos++;
> +               break;
> +       }
> +
> +       return n->startbit + pos;
> +}

Always fun to see new extensions to the ebitmap code that dates back
at least to 1996 in the original Flask code base. It's changed a lot
since then. That said, if we ever want to compute this frequently
we'll likely want to cache it in the ebitmap struct itself but this
seems fine for this usage. On a different but related note, I saw a
lot of time spent spinning through all-bits-set category set ebitmaps
in child SELinux namespaces due to the global SID mapping needing to
map contexts to each namespace. I solved that by introducing a cache
for the global SID mapping but was wondering if we ought to have a
more compressed representation of the all-bits-set ebitmap and more
efficient iterators for it. Just noting for future, not a request to
change this.


> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 8969f7c8637c..27f6809b562a 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -2955,6 +2955,11 @@ int policydb_read(struct policydb *p, struct policy_file *fp)
>                         if (rc)
>                                 goto bad;
>                 }
> +
> +               rc = -EINVAL;
> +               if (ebitmap_highest_set_bit(e) >= p->p_types.nprim)
> +                       goto bad;
> +
>                 /* add the type itself as the degenerate case */
>                 rc = ebitmap_set_bit(e, i, 1);
>                 if (rc)
> --
> 2.49.0
>