All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] selinux: bound class/common permission counts on policy load
@ 2026-07-28  1:30 ` Bryam Vargas
  0 siblings, 0 replies; 10+ messages in thread
From: Bryam Vargas via B4 Relay @ 2026-07-28  1:30 UTC (permalink / raw)
  To: Paul Moore, Stephen Smalley
  Cc: Kees Cook, Ondrej Mosnacek, Christian Göttsche, selinux,
	linux-kernel

perm_read() bounds a permission value to [1, SEL_VEC_MAX] but not against
the owning symbol table's nprim, which security_get_permissions() uses to
size a kcalloc() array that perms[value - 1] writes into. Loading a crafted
policy image then drives an out-of-bounds heap write in
get_permissions_callback(), reached from selinuxfs while it builds the new
policy's class directories. Two independent triggers:

  1/2 (perm_read): a class's own permission value exceeds the class nprim.
      Acked by Stephen Smalley on v1 [1].
  2/2 (class_read): a class inherits a common but declares fewer
      permissions than the common provides; the common's permissions --
      valid within the common -- are mapped into the class-sized array.
      perm_read() cannot catch this (a class with no own permissions never
      calls it), which is why v1 needed the second check.

A/B on a KASAN kernel, writing each policy into /sys/fs/selinux/load in a
single write -- sel_write_load() refuses a partial one outright, so a split
write never reaches the parser:

  1/2 -- "process" (31 own perms), permissions.nprim patched to 1: without
    the series, KASAN slab-out-of-bounds in get_permissions_callback <-
    security_get_permissions; with 1/2, rejected -EINVAL at parse.
  2/2 -- "capability" (inherits common "cap", 32 perms, no own perms),
    permissions.nprim patched to 1 -- one byte of the image: without the
    series, the same slab-out-of-bounds write of size 8, this time through
    the inherited-common map; with 2/2, rejected at parse with "class
    capability has fewer permissions than common cap".
  Control: a well-formed policy loads on the patched kernel exactly as it
    does without the series, with neither check firing.

The load path needs the SELinux load_policy permission plus write access to
/sys/fs/selinux/load and is not namespaced; this hardens the parser against
malformed or corrupted policy images.

v2:
 - split into two patches, as asked [2]; squash them on merge if that reads
   better
 - 2/2 is new: the inherited-common trigger
 - 1/2 is unchanged from v1, so Stephen's ack is carried forward
 - class_read() now names the class and the common when it rejects, like
   the "unknown common" error above it

The other thing review raised is not folded in here. Permission values that
are bounded but sparse leave NULL holes in that same perms[] array, and
sel_make_perm_files() passes each entry to sel_attach_file() ->
d_alloc_name() -> hashlen_string() with no check. By inspection; I have not
reproduced it, which is why it is not in this series rather than a third
patch. It follows once I have an A/B for it.

[1] https://lore.kernel.org/selinux/20260724-b4-disp-ec8ac9f6-v1-1-d76d51a69b9d@proton.me/
[2] https://lore.kernel.org/selinux/CAHC9VhT4M2--c-ZGf+xPSRz-TiGLUnfmULNXcWMC2NgycufrDg@mail.gmail.com/

---
Bryam Vargas (2):
      selinux: reject a permission value exceeding the class permission count
      selinux: reject a class permission count below its inherited common

 security/selinux/ss/policydb.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
---
base-commit: 48a5a7ab8d6ab7090564339e039c421f315de912
change-id: 20260727-b4-disp-eed1276a-e6eb48fcc67a

Best regards,
--  
Bryam Vargas <hexlabsecurity@proton.me>



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 0/2] selinux: bound class/common permission counts on policy load
@ 2026-07-28  1:30 ` Bryam Vargas
  0 siblings, 0 replies; 10+ messages in thread
From: Bryam Vargas @ 2026-07-28  1:30 UTC (permalink / raw)
  To: Paul Moore, Stephen Smalley
  Cc: Kees Cook, Ondrej Mosnacek, Christian Göttsche, selinux,
	linux-kernel

perm_read() bounds a permission value to [1, SEL_VEC_MAX] but not against
the owning symbol table's nprim, which security_get_permissions() uses to
size a kcalloc() array that perms[value - 1] writes into. Loading a crafted
policy image then drives an out-of-bounds heap write in
get_permissions_callback(), reached from selinuxfs while it builds the new
policy's class directories. Two independent triggers:

  1/2 (perm_read): a class's own permission value exceeds the class nprim.
      Acked by Stephen Smalley on v1 [1].
  2/2 (class_read): a class inherits a common but declares fewer
      permissions than the common provides; the common's permissions --
      valid within the common -- are mapped into the class-sized array.
      perm_read() cannot catch this (a class with no own permissions never
      calls it), which is why v1 needed the second check.

A/B on a KASAN kernel, writing each policy into /sys/fs/selinux/load in a
single write -- sel_write_load() refuses a partial one outright, so a split
write never reaches the parser:

  1/2 -- "process" (31 own perms), permissions.nprim patched to 1: without
    the series, KASAN slab-out-of-bounds in get_permissions_callback <-
    security_get_permissions; with 1/2, rejected -EINVAL at parse.
  2/2 -- "capability" (inherits common "cap", 32 perms, no own perms),
    permissions.nprim patched to 1 -- one byte of the image: without the
    series, the same slab-out-of-bounds write of size 8, this time through
    the inherited-common map; with 2/2, rejected at parse with "class
    capability has fewer permissions than common cap".
  Control: a well-formed policy loads on the patched kernel exactly as it
    does without the series, with neither check firing.

The load path needs the SELinux load_policy permission plus write access to
/sys/fs/selinux/load and is not namespaced; this hardens the parser against
malformed or corrupted policy images.

v2:
 - split into two patches, as asked [2]; squash them on merge if that reads
   better
 - 2/2 is new: the inherited-common trigger
 - 1/2 is unchanged from v1, so Stephen's ack is carried forward
 - class_read() now names the class and the common when it rejects, like
   the "unknown common" error above it

The other thing review raised is not folded in here. Permission values that
are bounded but sparse leave NULL holes in that same perms[] array, and
sel_make_perm_files() passes each entry to sel_attach_file() ->
d_alloc_name() -> hashlen_string() with no check. By inspection; I have not
reproduced it, which is why it is not in this series rather than a third
patch. It follows once I have an A/B for it.

[1] https://lore.kernel.org/selinux/20260724-b4-disp-ec8ac9f6-v1-1-d76d51a69b9d@proton.me/
[2] https://lore.kernel.org/selinux/CAHC9VhT4M2--c-ZGf+xPSRz-TiGLUnfmULNXcWMC2NgycufrDg@mail.gmail.com/

---
Bryam Vargas (2):
      selinux: reject a permission value exceeding the class permission count
      selinux: reject a class permission count below its inherited common

 security/selinux/ss/policydb.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
---
base-commit: 48a5a7ab8d6ab7090564339e039c421f315de912
change-id: 20260727-b4-disp-eed1276a-e6eb48fcc67a

Best regards,
--  
Bryam Vargas <hexlabsecurity@proton.me>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/2] selinux: reject a permission value exceeding the class permission count
  2026-07-28  1:30 ` Bryam Vargas
@ 2026-07-28  1:30   ` Bryam Vargas
  -1 siblings, 0 replies; 10+ messages in thread
From: Bryam Vargas via B4 Relay @ 2026-07-28  1:30 UTC (permalink / raw)
  To: Paul Moore, Stephen Smalley
  Cc: Kees Cook, Ondrej Mosnacek, Christian Göttsche, selinux,
	linux-kernel

From: Bryam Vargas <hexlabsecurity@proton.me>

perm_read() bounds a permission value by SEL_VEC_MAX but never by the
nprim of the owning class or common, which is taken verbatim from the
policy image.  security_get_permissions() then writes perms[value - 1]
into an nprim-sized kcalloc() array, so a class declaring fewer
permissions than its largest permission value drives an out-of-bounds
heap write.  The top-level symbol tables are validated this way; the
nested per-class permission table is not.

Reject a permission whose value exceeds nprim, which is already set when
perm_read() runs.  Well-formed policies are unaffected.

Fixes: 55fcf09b3fe4 ("selinux: add support for querying object classes and permissions from the running policy")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
---
 security/selinux/ss/policydb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index ead504a639e3..6973b68d9782 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1175,6 +1175,9 @@ static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *f
 	rc = -EINVAL;
 	if (perdatum->value < 1 || perdatum->value > SEL_VEC_MAX)
 		goto bad;
+	/* the value indexes an nprim-sized array in security_get_permissions() */
+	if (perdatum->value > s->nprim)
+		goto bad;
 
 	rc = str_read(&key, GFP_KERNEL, fp, len);
 	if (rc)

-- 
2.55.0



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 1/2] selinux: reject a permission value exceeding the class permission count
@ 2026-07-28  1:30   ` Bryam Vargas
  0 siblings, 0 replies; 10+ messages in thread
From: Bryam Vargas @ 2026-07-28  1:30 UTC (permalink / raw)
  To: Paul Moore, Stephen Smalley
  Cc: Kees Cook, Ondrej Mosnacek, Christian Göttsche, selinux,
	linux-kernel

perm_read() bounds a permission value by SEL_VEC_MAX but never by the
nprim of the owning class or common, which is taken verbatim from the
policy image.  security_get_permissions() then writes perms[value - 1]
into an nprim-sized kcalloc() array, so a class declaring fewer
permissions than its largest permission value drives an out-of-bounds
heap write.  The top-level symbol tables are validated this way; the
nested per-class permission table is not.

Reject a permission whose value exceeds nprim, which is already set when
perm_read() runs.  Well-formed policies are unaffected.

Fixes: 55fcf09b3fe4 ("selinux: add support for querying object classes and permissions from the running policy")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
---
 security/selinux/ss/policydb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index ead504a639e3..6973b68d9782 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1175,6 +1175,9 @@ static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *f
 	rc = -EINVAL;
 	if (perdatum->value < 1 || perdatum->value > SEL_VEC_MAX)
 		goto bad;
+	/* the value indexes an nprim-sized array in security_get_permissions() */
+	if (perdatum->value > s->nprim)
+		goto bad;
 
 	rc = str_read(&key, GFP_KERNEL, fp, len);
 	if (rc)

-- 
2.55.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/2] selinux: reject a class permission count below its inherited common
  2026-07-28  1:30 ` Bryam Vargas
@ 2026-07-28  1:30   ` Bryam Vargas
  -1 siblings, 0 replies; 10+ messages in thread
From: Bryam Vargas via B4 Relay @ 2026-07-28  1:30 UTC (permalink / raw)
  To: Paul Moore, Stephen Smalley
  Cc: Kees Cook, Ondrej Mosnacek, Christian Göttsche, selinux,
	linux-kernel

From: Bryam Vargas <hexlabsecurity@proton.me>

security_get_permissions() maps an inherited common's permissions into
an array sized by the class's own permissions.nprim, but class_read()
takes that nprim verbatim from the policy image and never checks that it
covers the common.  A class that inherits a common of N permissions while
declaring a smaller nprim is accepted, and on load the common's
permissions are written past the class-sized array -- an out-of-bounds
heap write.

Reject a class whose permission count is below its inherited common's.
Well-formed policies, where the class count already includes the
inherited permissions, are unaffected.

Fixes: 55fcf09b3fe4 ("selinux: add support for querying object classes and permissions from the running policy")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 security/selinux/ss/policydb.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 6973b68d9782..c6f55f0b4cb5 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1422,6 +1422,18 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
 			       cladatum->comkey);
 			goto bad;
 		}
+
+		/*
+		 * security_get_permissions() maps the common's permissions
+		 * into an array sized by this class's nprim, so a class must
+		 * declare at least as many as the common it inherits.
+		 */
+		if (cladatum->permissions.nprim <
+		    cladatum->comdatum->permissions.nprim) {
+			pr_err("SELinux:  class %s has fewer permissions than common %s\n",
+			       key, cladatum->comkey);
+			goto bad;
+		}
 	}
 	for (i = 0; i < nel; i++) {
 		rc = perm_read(p, &cladatum->permissions, fp);

-- 
2.55.0



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/2] selinux: reject a class permission count below its inherited common
@ 2026-07-28  1:30   ` Bryam Vargas
  0 siblings, 0 replies; 10+ messages in thread
From: Bryam Vargas @ 2026-07-28  1:30 UTC (permalink / raw)
  To: Paul Moore, Stephen Smalley
  Cc: Kees Cook, Ondrej Mosnacek, Christian Göttsche, selinux,
	linux-kernel

security_get_permissions() maps an inherited common's permissions into
an array sized by the class's own permissions.nprim, but class_read()
takes that nprim verbatim from the policy image and never checks that it
covers the common.  A class that inherits a common of N permissions while
declaring a smaller nprim is accepted, and on load the common's
permissions are written past the class-sized array -- an out-of-bounds
heap write.

Reject a class whose permission count is below its inherited common's.
Well-formed policies, where the class count already includes the
inherited permissions, are unaffected.

Fixes: 55fcf09b3fe4 ("selinux: add support for querying object classes and permissions from the running policy")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 security/selinux/ss/policydb.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 6973b68d9782..c6f55f0b4cb5 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1422,6 +1422,18 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
 			       cladatum->comkey);
 			goto bad;
 		}
+
+		/*
+		 * security_get_permissions() maps the common's permissions
+		 * into an array sized by this class's nprim, so a class must
+		 * declare at least as many as the common it inherits.
+		 */
+		if (cladatum->permissions.nprim <
+		    cladatum->comdatum->permissions.nprim) {
+			pr_err("SELinux:  class %s has fewer permissions than common %s\n",
+			       key, cladatum->comkey);
+			goto bad;
+		}
 	}
 	for (i = 0; i < nel; i++) {
 		rc = perm_read(p, &cladatum->permissions, fp);

-- 
2.55.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] selinux: reject a permission value exceeding the class permission count
  2026-07-28  1:30   ` Bryam Vargas
  (?)
@ 2026-07-28  1:45   ` sashiko-bot
  -1 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-07-28  1:45 UTC (permalink / raw)
  Cc: selinux

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete Fix / Out-of-Bounds Heap Write in `security_get_permissions` via inherited common classes.
--

--- Patch [1]: [PATCH v2 1/2] selinux: reject a permission value exceeding the class permission count ---
commit b4fe3642552433263525065c279ba6cb8b2c045e
Author: Bryam Vargas <hexlabsecurity@proton.me>

selinux: reject a permission value exceeding the class permission count

This patch adds a bounds check in perm_read() to ensure that a permission
value does not exceed the symbol table's nprim. This is intended to prevent
out-of-bounds heap writes during policy loading when
security_get_permissions() populates an nprim-sized array.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index ead504a639e36..6973b68d9782a 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1175,6 +1175,9 @@ static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *f
>  	rc = -EINVAL;
>  	if (perdatum->value < 1 || perdatum->value > SEL_VEC_MAX)
>  		goto bad;
> +	/* the value indexes an nprim-sized array in security_get_permissions() */
> +	if (perdatum->value > s->nprim)
> +		goto bad;

[Severity: High]
This is a pre-existing issue, but does this fully prevent the out-of-bounds
write when a class inherits from a common class?

The bounds check added here validates permissions against their immediate
symbol table's nprim (which is comdatum->permissions.nprim for common
permissions). It seems this misses the inheritance vector where a class's
nprim is smaller than its inherited common class.

For example, when the inheritance link is established in class_read():

security/selinux/ss/policydb.c:class_read() {
	...
	cladatum->comdatum = symtab_search(&p->p_commons, cladatum->comkey);
	...
}

There is no validation to ensure that
cladatum->permissions.nprim >= cladatum->comdatum->permissions.nprim.

Later, in security_get_permissions(), an undersized array could be allocated
using the class's nprim, and populated with the common's permissions:

security/selinux/ss/services.c:security_get_permissions() {
	...
	*nperms = match->permissions.nprim;
	*perms = kcalloc(*nperms, sizeof(**perms), GFP_ATOMIC);
	...
	if (match->comdatum) {
		rc = hashtab_map(&match->comdatum->permissions.table,
				 get_permissions_callback, *perms);
	...
}

Could a crafted policy still trigger an out-of-bounds write here if the class
declares fewer permissions than its inherited common class?

>  
>  	rc = str_read(&key, GFP_KERNEL, fp, len);
>  	if (rc)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-b4-disp-eed1276a-v2-0-82d58cf5f882@proton.me?part=1

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] selinux: reject a class permission count below its inherited common
  2026-07-28  1:30   ` Bryam Vargas
  (?)
@ 2026-07-28  1:46   ` sashiko-bot
  -1 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-07-28  1:46 UTC (permalink / raw)
  Cc: selinux

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Loading a crafted SELinux policy with sparse permission/class values or an inflated `nprim` leaves NULL pointers in symbol arrays, causing a kernel panic in selinuxfs directory creation.
- [Medium] A crafted SELinux policy with duplicate permission or class values causes unbounded memory leaks in `security_get_permissions()` and `security_get_classes()`.
--

--- Patch [2]: [PATCH v2 2/2] selinux: reject a class permission count below its inherited common ---
commit 0b87262f147a0083f6daa265c846c87ddbbf878b
Author: Bryam Vargas <hexlabsecurity@proton.me>

selinux: reject a class permission count below its inherited common

security_get_permissions() maps an inherited common's permissions into
an array sized by the class's own permissions.nprim, but class_read()
takes that nprim verbatim from the policy image and never checks that it
covers the common. A class that inherits a common of N permissions while
declaring a smaller nprim is accepted, and on load the common's
permissions are written past the class-sized array -- an out-of-bounds
heap write.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 6973b68d9782a..c6f55f0b4cb55 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1422,6 +1422,18 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
>  			       cladatum->comkey);
>  			goto bad;
>  		}
> +
> +		/*
> +		 * security_get_permissions() maps the common's permissions
> +		 * into an array sized by this class's nprim, so a class must
> +		 * declare at least as many as the common it inherits.
> +		 */
> +		if (cladatum->permissions.nprim <
> +		    cladatum->comdatum->permissions.nprim) {

[Severity: High]
This isn't a bug introduced by this patch, but does the kernel handle a policy
with sparse permission or class values correctly?

If a policy is loaded with an nprim value larger than the number of
contiguous, defined classes or permissions, security_get_classes() and
security_get_permissions() will allocate arrays of size nprim but only
populate the defined elements, leaving the rest as NULL.

Later, sel_make_classes() and sel_make_perm_files() pass these NULL pointers:

security/selinux/selinuxfs.c:sel_make_perm_files() {
    ...
    rc = sel_attach_file(dir, perms[i], inode);
    ...
}

Can this lead to a NULL pointer dereference in d_alloc_name() ->
hashlen_string() during selinuxfs directory creation?

> +			pr_err("SELinux:  class %s has fewer permissions than common %s\n",
> +			       key, cladatum->comkey);
> +			goto bad;
> +		}
>  	}
>  	for (i = 0; i < nel; i++) {
>  		rc = perm_read(p, &cladatum->permissions, fp);

[Severity: Medium]
This is a pre-existing issue, but does perm_read() enforce uniqueness of
permission and class values?

If an SELinux policy is loaded with multiple classes or permissions sharing the
same integer value, symtab_insert() only enforces uniqueness on the string
name.

During selinuxfs initialization, could get_permissions_callback() and
get_classes_callback() repeatedly execute kstrdup() to the same array index:

security/selinux/ss/services.c:get_permissions_callback() {
    ...
    perms[value] = kstrdup(name, GFP_ATOMIC);
    ...
}

Does this overwrite the previous pointer and leak the memory allocation?

>  		if (rc)
>  			goto bad;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-b4-disp-eed1276a-v2-0-82d58cf5f882@proton.me?part=2

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] selinux: reject a class permission count below its inherited common
  2026-07-28  1:30   ` Bryam Vargas
  (?)
  (?)
@ 2026-07-28 14:43   ` Stephen Smalley
  2026-07-28 17:42     ` Bryam Vargas
  -1 siblings, 1 reply; 10+ messages in thread
From: Stephen Smalley @ 2026-07-28 14:43 UTC (permalink / raw)
  To: hexlabsecurity
  Cc: Paul Moore, Kees Cook, Ondrej Mosnacek, Christian Göttsche,
	selinux, linux-kernel

On Mon, Jul 27, 2026 at 9:30 PM Bryam Vargas via B4 Relay
<devnull+hexlabsecurity.proton.me@kernel.org> wrote:
>
> From: Bryam Vargas <hexlabsecurity@proton.me>
>
> security_get_permissions() maps an inherited common's permissions into
> an array sized by the class's own permissions.nprim, but class_read()
> takes that nprim verbatim from the policy image and never checks that it
> covers the common.  A class that inherits a common of N permissions while
> declaring a smaller nprim is accepted, and on load the common's
> permissions are written past the class-sized array -- an out-of-bounds
> heap write.
>
> Reject a class whose permission count is below its inherited common's.
> Well-formed policies, where the class count already includes the
> inherited permissions, are unaffected.
>
> Fixes: 55fcf09b3fe4 ("selinux: add support for querying object classes and permissions from the running policy")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

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

Sashiko again made some interesting observations but not about your
patch per se,
https://lore.kernel.org/selinux/20260728014625.C7A391F000E9@smtp.kernel.org/T/#u

> ---
>  security/selinux/ss/policydb.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 6973b68d9782..c6f55f0b4cb5 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1422,6 +1422,18 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
>                                cladatum->comkey);
>                         goto bad;
>                 }
> +
> +               /*
> +                * security_get_permissions() maps the common's permissions
> +                * into an array sized by this class's nprim, so a class must
> +                * declare at least as many as the common it inherits.
> +                */
> +               if (cladatum->permissions.nprim <
> +                   cladatum->comdatum->permissions.nprim) {
> +                       pr_err("SELinux:  class %s has fewer permissions than common %s\n",
> +                              key, cladatum->comkey);
> +                       goto bad;
> +               }
>         }
>         for (i = 0; i < nel; i++) {
>                 rc = perm_read(p, &cladatum->permissions, fp);
>
> --
> 2.55.0
>
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] selinux: reject a class permission count below its inherited common
  2026-07-28 14:43   ` Stephen Smalley
@ 2026-07-28 17:42     ` Bryam Vargas
  0 siblings, 0 replies; 10+ messages in thread
From: Bryam Vargas @ 2026-07-28 17:42 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: Paul Moore, Ondrej Mosnacek, Christian Göttsche, Kees Cook,
	selinux, linux-kernel

On Tue, 28 Jul 2026 10:43:39 -0400, Stephen Smalley wrote:
> Sashiko again made some interesting observations but not about your patch per se,
> https://lore.kernel.org/selinux/20260728014625.C7A391F000E9@smtp.kernel.org/T/#u

The first one reproduces. security_get_permissions() sizes the array by the
class's permissions.nprim and fills it by value, so any index no perm datum
claims stays NULL, and sel_make_perm_files() passes every entry to
sel_attach_file() -> d_alloc_name() -> hashlen_string(). One u32 in the policy
image is enough: on the policy I tested, class process has nel == nprim == 31
and inherits no common, so raising its nprim to 39 leaves slots 31..38
unwritten.

  BUG: kernel NULL pointer dereference, address: 0000000000000000
  RIP: 0010:hashlen_string+0xa/0xc0
  Call Trace:
   d_alloc_name+0x61/0xa0
   sel_make_policy_nodes+0xb35/0x1310
   sel_write_load+0x2a5/0x490

load_unaligned_zeropad() doesn't absorb it: ex_handler_zeropad() rounds the
address down and bails unless fault_addr == addr + sizeof(long), so a NULL
base takes the ordinary fault.

It doesn't stop at the loading task. sel_make_policy_nodes() runs under
selinux_state.policy_mutex and the only unlock is the out_unlock label the
oops never reaches, so every later load blocks in D state; SIGKILL doesn't
clear it and only a reboot recovers. Under panic_on_oops it is a panic at
step one.

The second observation is real as stated: get_permissions_callback() does
overwrite perms[value - 1], and the cleanup loop walks i < nperms freeing
whichever pointer survived, so the first kstrdup() leaks. What makes it worth
more than its severity is that it also defeats the obvious fix for the first
one. Requiring the counts to add up -- nel plus the inherited nprim equals
permissions.nprim -- doesn't close the gap, because symtab_insert() hashes
and compares the name only, so two perms may carry the same value: the count
still matches and one slot is still never written. The property that holds is
that the values present are exactly {1..nprim}, onto and injective, where the
two patches on the list establish only "into".

The classes side has the same shape: get_classes_callback() also writes at
value - 1, and sel_make_classes() passes classes[i] to sel_make_dir(). By
inspection; I have not reproduced that one.

Policy crafter and the full log on request.

Thanks for the ack.

Bryam


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2026-07-28 17:42 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-28  1:30 [PATCH v2 0/2] selinux: bound class/common permission counts on policy load Bryam Vargas via B4 Relay
2026-07-28  1:30 ` Bryam Vargas
2026-07-28  1:30 ` [PATCH v2 1/2] selinux: reject a permission value exceeding the class permission count Bryam Vargas via B4 Relay
2026-07-28  1:30   ` Bryam Vargas
2026-07-28  1:45   ` sashiko-bot
2026-07-28  1:30 ` [PATCH v2 2/2] selinux: reject a class permission count below its inherited common Bryam Vargas via B4 Relay
2026-07-28  1:30   ` Bryam Vargas
2026-07-28  1:46   ` sashiko-bot
2026-07-28 14:43   ` Stephen Smalley
2026-07-28 17:42     ` Bryam Vargas

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.