All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] selinux: make declared counts account for the values
@ 2026-07-31 17:44 ` Bryam Vargas
  0 siblings, 0 replies; 17+ messages in thread
From: Bryam Vargas via B4 Relay @ 2026-07-31 17:44 UTC (permalink / raw)
  To: Paul Moore, Stephen Smalley
  Cc: linux-kernel, Ondrej Mosnacek, Kees Cook, selinux,
	Christian Göttsche

The two patches just merged [1] bound each permission value by the count
that sizes the array it indexes. That is half of what those arrays need:
they are filled by value, so every slot also has to be claimed, and claimed
once. 2/4 closes both halves for the permission array. 3/4 and 4/4 do the
same one layer up, for the class and boolean arrays, which have the same
shape and are reached from the same policy load. 1/4 is a prerequisite --
the error path 3/4 newly makes reachable already has a NULL dereference in
it.

A count check is not enough for 2/4, which is worth saying because it is the
obvious fix. symtab_insert() keys on the permission name, so two
differently-named permissions may carry one value: nel still equals nprim,
the count agrees, and one slot is written twice while another is never
written at all. What holds for a well-formed policy is that the values
present are exactly {1..nprim} -- onto and injective -- so that is what 2/4
enforces.

3/4 and 4/4 fix the same defect at different layers, deliberately. A sparse
class value is tolerated: policydb_class_isvalid() and its siblings exist to
absorb it, so rejecting one at load would undo that, and only
security_get_classes() is at fault -- it builds its own array straight from
the hash table and never picked up a predicate. Booleans have no predicate,
and no consumer that could use one: cond_evaluate_expr(),
security_get_bools(), security_get_bool_value() and security_set_bools() all
walk bool_val_to_struct[] by index and dereference every entry. Density is
already an unstated requirement of that array, so 4/4 states it once, where
the array is built, rather than adding four guards.

A/B on a KASAN build of v7.2-rc1. Arms A, B and C carry the two merged
patches, so each reproduces on top of what is already applied; D ran on the
stock build, since the merged patches are in perm_read() and class_read()
and the boolean path does not go near them. Every policy is the stock
refpolicy image (135 classes, 324 booleans) with one u32 changed, written to
/sys/fs/selinux/load in a single write:

  A -- "process" permissions.nprim 31 -> 39, leaving slots 31..38 unwritten:
    without the series, NULL dereference; with it, -EINVAL at parse.
  B -- "process" permission "getrlimit" value 31 -> 1, colliding with
    "fork". nel and nprim both stay 31, so a count check accepts it. Same
    dereference; with the series, -EINVAL.
  C -- p_classes.nprim 135 -> 139: the class array rather than a permission
    array. With the series, security_get_classes() fails the lookup.
  D -- p_bools.nprim 324 -> 328: with the series, rejected at load with
    "boolean 325 is declared but not defined".
  Control: a well-formed policy loads on the patched kernel exactly as it
    does without the series. With timestamps stripped the two control arms
    are 33 lines each and the diff between them is empty.

A, B and C give:

  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   <- A and B, via sel_make_perm_files
   sel_make_policy_nodes+0x8d5/0x1310   <- C, via sel_make_classes
   sel_write_load+0x2a5/0x490

and D faults earlier in the same function, on the array itself rather than
on a name derived from it:

  KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
  RIP: 0010:security_get_bools+0x287/0x790
  Call Trace:
   sel_make_policy_nodes+0x42f/0x1310   <- via sel_make_bools
   sel_write_load+0x2a5/0x490

Three distinct call sites in sel_make_policy_nodes(), which is what makes
these separate holes rather than one reached three times. D is first in that
function, so it is the one a sparse policy hits soonest.

1/4 came out of that A/B rather than from reading. The first build carrying
3/4 turned arm C into a general protection fault instead of a clean
rejection:

  RIP: 0010:selinux_policy_cancel+0x40/0x180
  Call Trace: sel_write_load+0x3c0/0x490 -> vfs_write -> ksys_write

sel_write_load() cancels the load when sel_make_policy_nodes() fails, and
the cancel dereferences the outgoing policy. On a first load there is none,
and sidtab is the first member of struct selinux_policy, hence the read at
offset 0. selinux_policy_commit(), which does the same
rcu_dereference_protected() a few lines away, guards with if (oldpolicy);
the cancel does not. It is reachable today through any -ENOMEM inside
sel_make_policy_nodes(); 3/4 would have made it policy-controlled, so it
goes first.

Two limits. The double-kstrdup leak 2/4 also closes is stated from the code,
not measured -- this rig has no CONFIG_DEBUG_KMEMLEAK, and the same crafted
policy oopses, which is the stronger oracle. And "conforming policies are
unaffected" rests on the patched control arm plus four classes checked by
hand, one of which is "capability": it has nprim == SEL_VEC_MAX exactly,
which is why the new rejection in 2/4 is a strict > rather than >=.

Based on selinux/stable-7.2 (9a82dcd98b6e), since 2/4 touches the functions
the merged series changed. Crafters and full logs on request.

[1] https://lore.kernel.org/selinux/CAHC9VhRt96xoRi9EcmU9cvBUiCJfrG=FBZPz92JyKmvQvesgvQ@mail.gmail.com/

---
Bryam Vargas (4):
      selinux: do not cancel a policy conversion that never started
      selinux: require a class's permission values to cover its permission count
      selinux: reject an unclaimed class value in security_get_classes()
      selinux: require every boolean value to be defined

 security/selinux/ss/policydb.c | 70 +++++++++++++++++++++++++++++++++++++++---
 security/selinux/ss/services.c | 28 +++++++++++++----
 2 files changed, 87 insertions(+), 11 deletions(-)
---
base-commit: 9a82dcd98b6e6e11cfd162410967951f12152528
change-id: 20260731-b4-disp-d32e997b-c002ecb15405

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



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

* [PATCH 0/4] selinux: make declared counts account for the values
@ 2026-07-31 17:44 ` Bryam Vargas
  0 siblings, 0 replies; 17+ messages in thread
From: Bryam Vargas @ 2026-07-31 17:44 UTC (permalink / raw)
  To: Paul Moore, Stephen Smalley
  Cc: linux-kernel, Ondrej Mosnacek, Kees Cook, selinux,
	Christian Göttsche

The two patches just merged [1] bound each permission value by the count
that sizes the array it indexes. That is half of what those arrays need:
they are filled by value, so every slot also has to be claimed, and claimed
once. 2/4 closes both halves for the permission array. 3/4 and 4/4 do the
same one layer up, for the class and boolean arrays, which have the same
shape and are reached from the same policy load. 1/4 is a prerequisite --
the error path 3/4 newly makes reachable already has a NULL dereference in
it.

A count check is not enough for 2/4, which is worth saying because it is the
obvious fix. symtab_insert() keys on the permission name, so two
differently-named permissions may carry one value: nel still equals nprim,
the count agrees, and one slot is written twice while another is never
written at all. What holds for a well-formed policy is that the values
present are exactly {1..nprim} -- onto and injective -- so that is what 2/4
enforces.

3/4 and 4/4 fix the same defect at different layers, deliberately. A sparse
class value is tolerated: policydb_class_isvalid() and its siblings exist to
absorb it, so rejecting one at load would undo that, and only
security_get_classes() is at fault -- it builds its own array straight from
the hash table and never picked up a predicate. Booleans have no predicate,
and no consumer that could use one: cond_evaluate_expr(),
security_get_bools(), security_get_bool_value() and security_set_bools() all
walk bool_val_to_struct[] by index and dereference every entry. Density is
already an unstated requirement of that array, so 4/4 states it once, where
the array is built, rather than adding four guards.

A/B on a KASAN build of v7.2-rc1. Arms A, B and C carry the two merged
patches, so each reproduces on top of what is already applied; D ran on the
stock build, since the merged patches are in perm_read() and class_read()
and the boolean path does not go near them. Every policy is the stock
refpolicy image (135 classes, 324 booleans) with one u32 changed, written to
/sys/fs/selinux/load in a single write:

  A -- "process" permissions.nprim 31 -> 39, leaving slots 31..38 unwritten:
    without the series, NULL dereference; with it, -EINVAL at parse.
  B -- "process" permission "getrlimit" value 31 -> 1, colliding with
    "fork". nel and nprim both stay 31, so a count check accepts it. Same
    dereference; with the series, -EINVAL.
  C -- p_classes.nprim 135 -> 139: the class array rather than a permission
    array. With the series, security_get_classes() fails the lookup.
  D -- p_bools.nprim 324 -> 328: with the series, rejected at load with
    "boolean 325 is declared but not defined".
  Control: a well-formed policy loads on the patched kernel exactly as it
    does without the series. With timestamps stripped the two control arms
    are 33 lines each and the diff between them is empty.

A, B and C give:

  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   <- A and B, via sel_make_perm_files
   sel_make_policy_nodes+0x8d5/0x1310   <- C, via sel_make_classes
   sel_write_load+0x2a5/0x490

and D faults earlier in the same function, on the array itself rather than
on a name derived from it:

  KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
  RIP: 0010:security_get_bools+0x287/0x790
  Call Trace:
   sel_make_policy_nodes+0x42f/0x1310   <- via sel_make_bools
   sel_write_load+0x2a5/0x490

Three distinct call sites in sel_make_policy_nodes(), which is what makes
these separate holes rather than one reached three times. D is first in that
function, so it is the one a sparse policy hits soonest.

1/4 came out of that A/B rather than from reading. The first build carrying
3/4 turned arm C into a general protection fault instead of a clean
rejection:

  RIP: 0010:selinux_policy_cancel+0x40/0x180
  Call Trace: sel_write_load+0x3c0/0x490 -> vfs_write -> ksys_write

sel_write_load() cancels the load when sel_make_policy_nodes() fails, and
the cancel dereferences the outgoing policy. On a first load there is none,
and sidtab is the first member of struct selinux_policy, hence the read at
offset 0. selinux_policy_commit(), which does the same
rcu_dereference_protected() a few lines away, guards with if (oldpolicy);
the cancel does not. It is reachable today through any -ENOMEM inside
sel_make_policy_nodes(); 3/4 would have made it policy-controlled, so it
goes first.

Two limits. The double-kstrdup leak 2/4 also closes is stated from the code,
not measured -- this rig has no CONFIG_DEBUG_KMEMLEAK, and the same crafted
policy oopses, which is the stronger oracle. And "conforming policies are
unaffected" rests on the patched control arm plus four classes checked by
hand, one of which is "capability": it has nprim == SEL_VEC_MAX exactly,
which is why the new rejection in 2/4 is a strict > rather than >=.

Based on selinux/stable-7.2 (9a82dcd98b6e), since 2/4 touches the functions
the merged series changed. Crafters and full logs on request.

[1] https://lore.kernel.org/selinux/CAHC9VhRt96xoRi9EcmU9cvBUiCJfrG=FBZPz92JyKmvQvesgvQ@mail.gmail.com/

---
Bryam Vargas (4):
      selinux: do not cancel a policy conversion that never started
      selinux: require a class's permission values to cover its permission count
      selinux: reject an unclaimed class value in security_get_classes()
      selinux: require every boolean value to be defined

 security/selinux/ss/policydb.c | 70 +++++++++++++++++++++++++++++++++++++++---
 security/selinux/ss/services.c | 28 +++++++++++++----
 2 files changed, 87 insertions(+), 11 deletions(-)
---
base-commit: 9a82dcd98b6e6e11cfd162410967951f12152528
change-id: 20260731-b4-disp-d32e997b-c002ecb15405

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


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

* [PATCH 1/4] selinux: do not cancel a policy conversion that never started
  2026-07-31 17:44 ` Bryam Vargas
@ 2026-07-31 17:44   ` Bryam Vargas
  -1 siblings, 0 replies; 17+ messages in thread
From: Bryam Vargas via B4 Relay @ 2026-07-31 17:44 UTC (permalink / raw)
  To: Paul Moore, Stephen Smalley
  Cc: linux-kernel, Ondrej Mosnacek, Kees Cook, selinux,
	Christian Göttsche

From: Bryam Vargas <hexlabsecurity@proton.me>

sel_write_load() calls selinux_policy_cancel() when sel_make_policy_nodes()
fails, and that helper dereferences the outgoing policy to cancel its
sidtab conversion. On the first policy load there is no outgoing policy:
security_load_policy() returns early for that case, before it converts
anything, and state->policy is still NULL. A first load that fails while
building the selinuxfs tree therefore takes a NULL dereference in
selinux_policy_cancel(), reached from a write(2) to /sys/fs/selinux/load.

Skip the cancel when there is no old policy, mirroring the check
security_load_policy() already makes before it converts.

Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 security/selinux/ss/services.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 2d828548f3db..90e81186cb2e 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2221,7 +2221,9 @@ void selinux_policy_cancel(struct selinux_load_state *load_state)
 	oldpolicy = rcu_dereference_protected(state->policy,
 					lockdep_is_held(&state->policy_mutex));
 
-	sidtab_cancel_convert(oldpolicy->sidtab);
+	/* a first load has no outgoing policy and converted nothing */
+	if (oldpolicy)
+		sidtab_cancel_convert(oldpolicy->sidtab);
 	selinux_policy_free(load_state->policy);
 	kfree(load_state->convert_data);
 }

-- 
2.55.0



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

* [PATCH 1/4] selinux: do not cancel a policy conversion that never started
@ 2026-07-31 17:44   ` Bryam Vargas
  0 siblings, 0 replies; 17+ messages in thread
From: Bryam Vargas @ 2026-07-31 17:44 UTC (permalink / raw)
  To: Paul Moore, Stephen Smalley
  Cc: linux-kernel, Ondrej Mosnacek, Kees Cook, selinux,
	Christian Göttsche

sel_write_load() calls selinux_policy_cancel() when sel_make_policy_nodes()
fails, and that helper dereferences the outgoing policy to cancel its
sidtab conversion. On the first policy load there is no outgoing policy:
security_load_policy() returns early for that case, before it converts
anything, and state->policy is still NULL. A first load that fails while
building the selinuxfs tree therefore takes a NULL dereference in
selinux_policy_cancel(), reached from a write(2) to /sys/fs/selinux/load.

Skip the cancel when there is no old policy, mirroring the check
security_load_policy() already makes before it converts.

Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 security/selinux/ss/services.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 2d828548f3db..90e81186cb2e 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2221,7 +2221,9 @@ void selinux_policy_cancel(struct selinux_load_state *load_state)
 	oldpolicy = rcu_dereference_protected(state->policy,
 					lockdep_is_held(&state->policy_mutex));
 
-	sidtab_cancel_convert(oldpolicy->sidtab);
+	/* a first load has no outgoing policy and converted nothing */
+	if (oldpolicy)
+		sidtab_cancel_convert(oldpolicy->sidtab);
 	selinux_policy_free(load_state->policy);
 	kfree(load_state->convert_data);
 }

-- 
2.55.0


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

* [PATCH 2/4] selinux: require a class's permission values to cover its permission count
  2026-07-31 17:44 ` Bryam Vargas
@ 2026-07-31 17:44   ` Bryam Vargas
  -1 siblings, 0 replies; 17+ messages in thread
From: Bryam Vargas via B4 Relay @ 2026-07-31 17:44 UTC (permalink / raw)
  To: Paul Moore, Stephen Smalley
  Cc: linux-kernel, Ondrej Mosnacek, Kees Cook, selinux,
	Christian Göttsche

From: Bryam Vargas <hexlabsecurity@proton.me>

security_get_permissions() sizes an array by the class's permissions.nprim
and fills it at value - 1, from the inherited common's permission table and
then the class's own. A value no permission defines leaves a NULL that
sel_make_perm_files() passes to d_alloc_name(), an oops inside
sel_write_load() that strands selinux_state.policy_mutex and leaves every
later load in uninterruptible sleep; two permissions sharing a value
overwrite the first kstrdup(). Bounding each value by nprim catches
neither, and neither would a count: the symbol table is keyed on the
permission name, so duplicates pass.

Track the values each permission table claims and require them to cover
exactly what its count declares, rejecting a count no value can reach.
Conforming 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>
---
 security/selinux/ss/policydb.c | 51 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 46 insertions(+), 5 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 69777e885ae7..d358200817bd 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1154,7 +1154,18 @@ int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
 	return 0;
 }
 
-static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
+/*
+ * Bitmap of the permission values a symtab has claimed.  Values are 1-based
+ * and bounded by SEL_VEC_MAX, the width of an access vector, so the whole set
+ * fits in a u32 and the callers reject an nprim past that width.
+ */
+static u32 perm_claimed_mask(u32 nprim)
+{
+	return nprim ? U32_MAX >> (SEL_VEC_MAX - nprim) : 0;
+}
+
+static int perm_read(struct policydb *p, struct symtab *s,
+		     struct policy_file *fp, u32 *claimed)
 {
 	char *key = NULL;
 	struct perm_datum *perdatum;
@@ -1178,6 +1189,10 @@ static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *f
 	/* indexes an nprim-sized array in security_get_permissions() */
 	if (perdatum->value > s->nprim)
 		goto bad;
+	/* two permissions cannot share one slot of that array */
+	if (*claimed & (1U << (perdatum->value - 1)))
+		goto bad;
+	*claimed |= 1U << (perdatum->value - 1);
 
 	rc = str_read(&key, GFP_KERNEL, fp, len);
 	if (rc)
@@ -1198,7 +1213,7 @@ static int common_read(struct policydb *p, struct symtab *s, struct policy_file
 	char *key = NULL;
 	struct common_datum *comdatum;
 	__le32 buf[4];
-	u32 i, len, nel;
+	u32 i, len, nel, claimed = 0;
 	int rc;
 
 	comdatum = kzalloc_obj(*comdatum);
@@ -1225,17 +1240,28 @@ static int common_read(struct policydb *p, struct symtab *s, struct policy_file
 	if (rc)
 		goto bad;
 	comdatum->permissions.nprim = le32_to_cpu(buf[2]);
+	/* no permission value can reach a slot past SEL_VEC_MAX */
+	rc = -EINVAL;
+	if (comdatum->permissions.nprim > SEL_VEC_MAX)
+		goto bad;
 
 	rc = str_read(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
 	for (i = 0; i < nel; i++) {
-		rc = perm_read(p, &comdatum->permissions, fp);
+		rc = perm_read(p, &comdatum->permissions, fp, &claimed);
 		if (rc)
 			goto bad;
 	}
 
+	rc = -EINVAL;
+	if (claimed != perm_claimed_mask(comdatum->permissions.nprim)) {
+		pr_err("SELinux:  common %s does not define every permission it declares\n",
+		       key);
+		goto bad;
+	}
+
 	hash_eval(&comdatum->permissions.table, "common_permissions", key);
 
 	rc = symtab_insert(s, key, comdatum);
@@ -1369,7 +1395,7 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
 	char *key = NULL;
 	struct class_datum *cladatum;
 	__le32 buf[6];
-	u32 i, len, len2, ncons, nel, val;
+	u32 i, len, len2, ncons, nel, val, claimed = 0, inherited = 0;
 	int rc;
 
 	cladatum = kzalloc_obj(*cladatum);
@@ -1402,6 +1428,10 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
 	if (rc)
 		goto bad;
 	cladatum->permissions.nprim = le32_to_cpu(buf[3]);
+	/* no permission value can reach a slot past SEL_VEC_MAX */
+	rc = -EINVAL;
+	if (cladatum->permissions.nprim > SEL_VEC_MAX)
+		goto bad;
 
 	ncons = le32_to_cpu(buf[5]);
 
@@ -1436,11 +1466,22 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
 		}
 	}
 	for (i = 0; i < nel; i++) {
-		rc = perm_read(p, &cladatum->permissions, fp);
+		rc = perm_read(p, &cladatum->permissions, fp, &claimed);
 		if (rc)
 			goto bad;
 	}
 
+	/* the class's own permissions must claim the slots the common leaves */
+	if (cladatum->comdatum)
+		inherited = cladatum->comdatum->permissions.nprim;
+	rc = -EINVAL;
+	if (claimed != (perm_claimed_mask(cladatum->permissions.nprim) &
+			~perm_claimed_mask(inherited))) {
+		pr_err("SELinux:  class %s does not define every permission it declares\n",
+		       key);
+		goto bad;
+	}
+
 	hash_eval(&cladatum->permissions.table, "class_permissions", key);
 
 	rc = read_cons_helper(p, &cladatum->constraints, ncons, 0, fp);

-- 
2.55.0



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

* [PATCH 2/4] selinux: require a class's permission values to cover its permission count
@ 2026-07-31 17:44   ` Bryam Vargas
  0 siblings, 0 replies; 17+ messages in thread
From: Bryam Vargas @ 2026-07-31 17:44 UTC (permalink / raw)
  To: Paul Moore, Stephen Smalley
  Cc: linux-kernel, Ondrej Mosnacek, Kees Cook, selinux,
	Christian Göttsche

security_get_permissions() sizes an array by the class's permissions.nprim
and fills it at value - 1, from the inherited common's permission table and
then the class's own. A value no permission defines leaves a NULL that
sel_make_perm_files() passes to d_alloc_name(), an oops inside
sel_write_load() that strands selinux_state.policy_mutex and leaves every
later load in uninterruptible sleep; two permissions sharing a value
overwrite the first kstrdup(). Bounding each value by nprim catches
neither, and neither would a count: the symbol table is keyed on the
permission name, so duplicates pass.

Track the values each permission table claims and require them to cover
exactly what its count declares, rejecting a count no value can reach.
Conforming 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>
---
 security/selinux/ss/policydb.c | 51 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 46 insertions(+), 5 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 69777e885ae7..d358200817bd 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1154,7 +1154,18 @@ int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
 	return 0;
 }
 
-static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
+/*
+ * Bitmap of the permission values a symtab has claimed.  Values are 1-based
+ * and bounded by SEL_VEC_MAX, the width of an access vector, so the whole set
+ * fits in a u32 and the callers reject an nprim past that width.
+ */
+static u32 perm_claimed_mask(u32 nprim)
+{
+	return nprim ? U32_MAX >> (SEL_VEC_MAX - nprim) : 0;
+}
+
+static int perm_read(struct policydb *p, struct symtab *s,
+		     struct policy_file *fp, u32 *claimed)
 {
 	char *key = NULL;
 	struct perm_datum *perdatum;
@@ -1178,6 +1189,10 @@ static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *f
 	/* indexes an nprim-sized array in security_get_permissions() */
 	if (perdatum->value > s->nprim)
 		goto bad;
+	/* two permissions cannot share one slot of that array */
+	if (*claimed & (1U << (perdatum->value - 1)))
+		goto bad;
+	*claimed |= 1U << (perdatum->value - 1);
 
 	rc = str_read(&key, GFP_KERNEL, fp, len);
 	if (rc)
@@ -1198,7 +1213,7 @@ static int common_read(struct policydb *p, struct symtab *s, struct policy_file
 	char *key = NULL;
 	struct common_datum *comdatum;
 	__le32 buf[4];
-	u32 i, len, nel;
+	u32 i, len, nel, claimed = 0;
 	int rc;
 
 	comdatum = kzalloc_obj(*comdatum);
@@ -1225,17 +1240,28 @@ static int common_read(struct policydb *p, struct symtab *s, struct policy_file
 	if (rc)
 		goto bad;
 	comdatum->permissions.nprim = le32_to_cpu(buf[2]);
+	/* no permission value can reach a slot past SEL_VEC_MAX */
+	rc = -EINVAL;
+	if (comdatum->permissions.nprim > SEL_VEC_MAX)
+		goto bad;
 
 	rc = str_read(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
 	for (i = 0; i < nel; i++) {
-		rc = perm_read(p, &comdatum->permissions, fp);
+		rc = perm_read(p, &comdatum->permissions, fp, &claimed);
 		if (rc)
 			goto bad;
 	}
 
+	rc = -EINVAL;
+	if (claimed != perm_claimed_mask(comdatum->permissions.nprim)) {
+		pr_err("SELinux:  common %s does not define every permission it declares\n",
+		       key);
+		goto bad;
+	}
+
 	hash_eval(&comdatum->permissions.table, "common_permissions", key);
 
 	rc = symtab_insert(s, key, comdatum);
@@ -1369,7 +1395,7 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
 	char *key = NULL;
 	struct class_datum *cladatum;
 	__le32 buf[6];
-	u32 i, len, len2, ncons, nel, val;
+	u32 i, len, len2, ncons, nel, val, claimed = 0, inherited = 0;
 	int rc;
 
 	cladatum = kzalloc_obj(*cladatum);
@@ -1402,6 +1428,10 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
 	if (rc)
 		goto bad;
 	cladatum->permissions.nprim = le32_to_cpu(buf[3]);
+	/* no permission value can reach a slot past SEL_VEC_MAX */
+	rc = -EINVAL;
+	if (cladatum->permissions.nprim > SEL_VEC_MAX)
+		goto bad;
 
 	ncons = le32_to_cpu(buf[5]);
 
@@ -1436,11 +1466,22 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
 		}
 	}
 	for (i = 0; i < nel; i++) {
-		rc = perm_read(p, &cladatum->permissions, fp);
+		rc = perm_read(p, &cladatum->permissions, fp, &claimed);
 		if (rc)
 			goto bad;
 	}
 
+	/* the class's own permissions must claim the slots the common leaves */
+	if (cladatum->comdatum)
+		inherited = cladatum->comdatum->permissions.nprim;
+	rc = -EINVAL;
+	if (claimed != (perm_claimed_mask(cladatum->permissions.nprim) &
+			~perm_claimed_mask(inherited))) {
+		pr_err("SELinux:  class %s does not define every permission it declares\n",
+		       key);
+		goto bad;
+	}
+
 	hash_eval(&cladatum->permissions.table, "class_permissions", key);
 
 	rc = read_cons_helper(p, &cladatum->constraints, ncons, 0, fp);

-- 
2.55.0


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

* [PATCH 3/4] selinux: reject an unclaimed class value in security_get_classes()
  2026-07-31 17:44 ` Bryam Vargas
@ 2026-07-31 17:44   ` Bryam Vargas
  -1 siblings, 0 replies; 17+ messages in thread
From: Bryam Vargas via B4 Relay @ 2026-07-31 17:44 UTC (permalink / raw)
  To: Paul Moore, Stephen Smalley
  Cc: linux-kernel, Ondrej Mosnacek, Kees Cook, selinux,
	Christian Göttsche

From: Bryam Vargas <hexlabsecurity@proton.me>

security_get_classes() sizes an array by p_classes.nprim and fills it at
value - 1, so a class value the policy never defines leaves a NULL.
sel_make_classes() passes every entry to sel_make_dir(), reaching the same
d_alloc_name() dereference as the permission array. The class symbol table
is allowed to be sparse (policydb_class_isvalid() exists to absorb that),
but this getter builds its own array straight from the hash table and has
no such predicate.

Fail the lookup when a value went unclaimed instead of handing out the
NULL. Conforming policies define every class they declare and 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/services.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 90e81186cb2e..7afce975436e 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -3304,6 +3304,7 @@ int security_get_classes(struct selinux_policy *policy,
 			 char ***classes, u32 *nclasses)
 {
 	struct policydb *policydb;
+	u32 i;
 	int rc;
 
 	policydb = &policy->policydb;
@@ -3316,16 +3317,29 @@ int security_get_classes(struct selinux_policy *policy,
 
 	rc = hashtab_map(&policydb->p_classes.table, get_classes_callback,
 			 *classes);
-	if (rc) {
-		u32 i;
+	if (rc)
+		goto err;
 
-		for (i = 0; i < *nclasses; i++)
-			kfree((*classes)[i]);
-		kfree(*classes);
+	/*
+	 * The class symtab may be sparse, which policydb_class_isvalid() exists
+	 * to absorb; the callback fills this array by value, so an unclaimed
+	 * one leaves a NULL that sel_make_classes() hands to sel_make_dir().
+	 */
+	for (i = 0; i < *nclasses; i++) {
+		if (!(*classes)[i]) {
+			rc = -EINVAL;
+			goto err;
+		}
 	}
 
 out:
 	return rc;
+
+err:
+	for (i = 0; i < *nclasses; i++)
+		kfree((*classes)[i]);
+	kfree(*classes);
+	return rc;
 }
 
 static int get_permissions_callback(void *k, void *d, void *args)

-- 
2.55.0



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

* [PATCH 3/4] selinux: reject an unclaimed class value in security_get_classes()
@ 2026-07-31 17:44   ` Bryam Vargas
  0 siblings, 0 replies; 17+ messages in thread
From: Bryam Vargas @ 2026-07-31 17:44 UTC (permalink / raw)
  To: Paul Moore, Stephen Smalley
  Cc: linux-kernel, Ondrej Mosnacek, Kees Cook, selinux,
	Christian Göttsche

security_get_classes() sizes an array by p_classes.nprim and fills it at
value - 1, so a class value the policy never defines leaves a NULL.
sel_make_classes() passes every entry to sel_make_dir(), reaching the same
d_alloc_name() dereference as the permission array. The class symbol table
is allowed to be sparse (policydb_class_isvalid() exists to absorb that),
but this getter builds its own array straight from the hash table and has
no such predicate.

Fail the lookup when a value went unclaimed instead of handing out the
NULL. Conforming policies define every class they declare and 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/services.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 90e81186cb2e..7afce975436e 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -3304,6 +3304,7 @@ int security_get_classes(struct selinux_policy *policy,
 			 char ***classes, u32 *nclasses)
 {
 	struct policydb *policydb;
+	u32 i;
 	int rc;
 
 	policydb = &policy->policydb;
@@ -3316,16 +3317,29 @@ int security_get_classes(struct selinux_policy *policy,
 
 	rc = hashtab_map(&policydb->p_classes.table, get_classes_callback,
 			 *classes);
-	if (rc) {
-		u32 i;
+	if (rc)
+		goto err;
 
-		for (i = 0; i < *nclasses; i++)
-			kfree((*classes)[i]);
-		kfree(*classes);
+	/*
+	 * The class symtab may be sparse, which policydb_class_isvalid() exists
+	 * to absorb; the callback fills this array by value, so an unclaimed
+	 * one leaves a NULL that sel_make_classes() hands to sel_make_dir().
+	 */
+	for (i = 0; i < *nclasses; i++) {
+		if (!(*classes)[i]) {
+			rc = -EINVAL;
+			goto err;
+		}
 	}
 
 out:
 	return rc;
+
+err:
+	for (i = 0; i < *nclasses; i++)
+		kfree((*classes)[i]);
+	kfree(*classes);
+	return rc;
 }
 
 static int get_permissions_callback(void *k, void *d, void *args)

-- 
2.55.0


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

* [PATCH 4/4] selinux: require every boolean value to be defined
  2026-07-31 17:44 ` Bryam Vargas
@ 2026-07-31 17:44   ` Bryam Vargas
  -1 siblings, 0 replies; 17+ messages in thread
From: Bryam Vargas via B4 Relay @ 2026-07-31 17:44 UTC (permalink / raw)
  To: Paul Moore, Stephen Smalley
  Cc: linux-kernel, Ondrej Mosnacek, Kees Cook, selinux,
	Christian Göttsche

From: Bryam Vargas <hexlabsecurity@proton.me>

p_bools.nprim comes from the policy image independently of how many
booleans follow it, and cond_index_bool() fills bool_val_to_struct[] at
value - 1, so a count larger than the values present leaves NULL entries.
Every user of that array then walks it by index and dereferences each
entry: cond_evaluate_expr() on the access-vector path,
security_get_bools() and security_get_bool_value() behind selinuxfs, and
security_set_bools(). A sparse class value is absorbed by
policydb_class_isvalid() and its siblings; booleans have no such
predicate, and no consumer that could use one.

Reject a boolean value that no boolean defines, once, where the array is
built. Conforming policies define every boolean they declare and are
unaffected.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 security/selinux/ss/policydb.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index d358200817bd..d88713201be9 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -719,6 +719,7 @@ static inline void symtab_hash_eval(struct symtab *s)
 static int policydb_index(struct policydb *p)
 {
 	int i, rc;
+	u32 v;
 
 	if (p->mls_enabled)
 		pr_debug(
@@ -769,6 +770,24 @@ static int policydb_index(struct policydb *p)
 		if (rc)
 			goto out;
 	}
+
+	/*
+	 * A sparse class value is absorbed by policydb_class_isvalid() and
+	 * its siblings, but no such predicate exists for booleans: every
+	 * user of bool_val_to_struct[] walks it by index and dereferences
+	 * each entry -- cond_evaluate_expr(), the two getters and
+	 * security_set_bools() -- so an unclaimed one has no consumer that
+	 * can tolerate it.
+	 */
+	for (v = 0; v < p->p_bools.nprim; v++) {
+		if (!p->bool_val_to_struct[v]) {
+			pr_err("SELinux:  boolean %u is declared but not defined\n",
+			       v + 1);
+			rc = -EINVAL;
+			goto out;
+		}
+	}
+
 	rc = 0;
 out:
 	return rc;

-- 
2.55.0



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

* [PATCH 4/4] selinux: require every boolean value to be defined
@ 2026-07-31 17:44   ` Bryam Vargas
  0 siblings, 0 replies; 17+ messages in thread
From: Bryam Vargas @ 2026-07-31 17:44 UTC (permalink / raw)
  To: Paul Moore, Stephen Smalley
  Cc: linux-kernel, Ondrej Mosnacek, Kees Cook, selinux,
	Christian Göttsche

p_bools.nprim comes from the policy image independently of how many
booleans follow it, and cond_index_bool() fills bool_val_to_struct[] at
value - 1, so a count larger than the values present leaves NULL entries.
Every user of that array then walks it by index and dereferences each
entry: cond_evaluate_expr() on the access-vector path,
security_get_bools() and security_get_bool_value() behind selinuxfs, and
security_set_bools(). A sparse class value is absorbed by
policydb_class_isvalid() and its siblings; booleans have no such
predicate, and no consumer that could use one.

Reject a boolean value that no boolean defines, once, where the array is
built. Conforming policies define every boolean they declare and are
unaffected.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 security/selinux/ss/policydb.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index d358200817bd..d88713201be9 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -719,6 +719,7 @@ static inline void symtab_hash_eval(struct symtab *s)
 static int policydb_index(struct policydb *p)
 {
 	int i, rc;
+	u32 v;
 
 	if (p->mls_enabled)
 		pr_debug(
@@ -769,6 +770,24 @@ static int policydb_index(struct policydb *p)
 		if (rc)
 			goto out;
 	}
+
+	/*
+	 * A sparse class value is absorbed by policydb_class_isvalid() and
+	 * its siblings, but no such predicate exists for booleans: every
+	 * user of bool_val_to_struct[] walks it by index and dereferences
+	 * each entry -- cond_evaluate_expr(), the two getters and
+	 * security_set_bools() -- so an unclaimed one has no consumer that
+	 * can tolerate it.
+	 */
+	for (v = 0; v < p->p_bools.nprim; v++) {
+		if (!p->bool_val_to_struct[v]) {
+			pr_err("SELinux:  boolean %u is declared but not defined\n",
+			       v + 1);
+			rc = -EINVAL;
+			goto out;
+		}
+	}
+
 	rc = 0;
 out:
 	return rc;

-- 
2.55.0


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

* Re: [PATCH 2/4] selinux: require a class's permission values to cover its permission count
  2026-07-31 17:44   ` Bryam Vargas
  (?)
@ 2026-07-31 17:50   ` sashiko-bot
  -1 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-07-31 17:50 UTC (permalink / raw)
  Cc: selinux

> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> security_get_permissions() sizes an array by the class's permissions.nprim
> and fills it at value - 1, from the inherited common's permission table and
> then the class's own. A value no permission defines leaves a NULL that
> sel_make_perm_files() passes to d_alloc_name(), an oops inside
> sel_write_load() that strands selinux_state.policy_mutex and leaves every
> later load in uninterruptible sleep; two permissions sharing a value
> overwrite the first kstrdup(). Bounding each value by nprim catches
> neither, and neither would a count: the symbol table is keyed on the
> permission name, so duplicates pass.
> 
> Track the values each permission table claims and require them to cover
> exactly what its count declares, rejecting a count no value can reach.
> Conforming 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-b4-disp-d32e997b-v1-0-0341d038ee47@proton.me?part=2


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

* Re: [PATCH 1/4] selinux: do not cancel a policy conversion that never started
  2026-07-31 17:44   ` Bryam Vargas
  (?)
@ 2026-07-31 17:58   ` sashiko-bot
  -1 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-07-31 17:58 UTC (permalink / raw)
  Cc: selinux

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

Pre-existing issues:
- [High] Partial failure in `sel_make_policy_nodes()` fails to roll back the boolean directory swap, leaving selinuxfs in a corrupted torn state.
--

--- Patch [1]: [PATCH 1/4] selinux: do not cancel a policy conversion that never started ---
commit 60502da4772313c1b4e1e77b979bed50ee026bc3
Author: Bryam Vargas <hexlabsecurity@proton.me>

selinux: do not cancel a policy conversion that never started

This commit prevents a NULL pointer dereference in selinux_policy_cancel()
by checking if the old policy exists before attempting to cancel its
sidtab conversion. This handles the case where a first policy load fails
during selinuxfs tree creation.

> sel_write_load() calls selinux_policy_cancel() when sel_make_policy_nodes()
> fails, and that helper dereferences the outgoing policy to cancel its
> sidtab conversion.

[Severity: High]
This is a pre-existing issue, but looking at sel_make_policy_nodes() in
security/selinux/selinuxfs.c, if a partial failure occurs, could it leave
selinuxfs in a corrupted torn state?

If the class directory rename fails:

security/selinux/selinuxfs.c:sel_make_policy_nodes() {
    ...
	/* classes */
	ret = start_renaming_two_dentries(&rd, tmp_class_dir, fsi->class_dir);
	if (ret)
		goto out;
    ...
}

The function jumps to the error path after already committing the boolean
directory swap.

Is there a missing rollback for the VFS boolean directory swap and the
selinux_fs_info boolean array swaps on this error path?

Without a rollback, could reading or committing booleans via selinuxfs
operate on freed memory or scrambled indices since the old policy data is
freed in the cleanup path?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-b4-disp-d32e997b-v1-0-0341d038ee47@proton.me?part=1

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

* Re: [PATCH 4/4] selinux: require every boolean value to be defined
  2026-07-31 17:44   ` Bryam Vargas
  (?)
@ 2026-07-31 18:01   ` sashiko-bot
  -1 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-07-31 18:01 UTC (permalink / raw)
  Cc: selinux

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

Pre-existing issues:
- [High] Missing validation for sparse types and users in `policydb_context_isvalid()` allows a crafted SELinux policy to trigger kernel panics.
--

--- Patch [4]: [PATCH 4/4] selinux: require every boolean value to be defined ---
commit 44c8d8193cd1a1c62c2bebd114870d7131ceb5ab
Author: Bryam Vargas <hexlabsecurity@proton.me>

selinux: require every boolean value to be defined

This commit adds validation during SELinux policy loading to ensure every
declared boolean is defined. This prevents NULL pointer dereferences when
consumers iterate over the bool_val_to_struct array.

Cc: stable@vger.kernel.org

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index d358200817bd9..d88713201be92 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c

[ ... ]

> @@ -769,6 +770,24 @@ static int policydb_index(struct policydb *p)
>  		if (rc)
>  			goto out;
>  	}
> +
> +	/*
> +	 * A sparse class value is absorbed by policydb_class_isvalid() and
> +	 * its siblings, but no such predicate exists for booleans: every
> +	 * user of bool_val_to_struct[] walks it by index and dereferences
> +	 * each entry -- cond_evaluate_expr(), the two getters and
> +	 * security_set_bools() -- so an unclaimed one has no consumer that
> +	 * can tolerate it.
> +	 */
> +	for (v = 0; v < p->p_bools.nprim; v++) {
> +		if (!p->bool_val_to_struct[v]) {
> +			pr_err("SELinux:  boolean %u is declared but not defined\n",
> +			       v + 1);
> +			rc = -EINVAL;
> +			goto out;
> +		}
> +	}
> +

[Severity: High]
This is a pre-existing issue, but since this patch adds validation for
sparse booleans, is it necessary to add similar validation for sparse types
and users?

Context validation skips checking if the type is actually defined when
c->role == OBJECT_R_VAL:

security/selinux/ss/policydb.c:policydb_context_isvalid() {
    ...
    if (!c->type || c->type > p->p_types.nprim)
        return false;

    if (c->role != OBJECT_R_VAL) {
    ...
}

If a crafted policy assigns a sparse type to an object context, it is
accepted. Later, when the context is evaluated, does this trigger a kernel
panic when fetching the NULL struct?

security/selinux/ss/services.c:type_attribute_bounds_av() {
    ...
    target = policydb->type_val_to_struct[tcontext->type - 1];
    BUG_ON(!target);
    ...
}

It also looks like this could cause a NULL pointer dereference during string
conversion (such as in audit logs) because sym_name() returns NULL:

security/selinux/ss/services.c:context_struct_to_string() {
    ...
    *scontext_len += strlen(sym_name(p, SYM_TYPES, context->type - 1)) + 1;
    ...
}

Could a malicious container policy exploit this missing type validation to
crash the host system?

>  	rc = 0;
>  out:
>  	return rc;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-b4-disp-d32e997b-v1-0-0341d038ee47@proton.me?part=4

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

* Re: [PATCH 3/4] selinux: reject an unclaimed class value in security_get_classes()
  2026-07-31 17:44   ` Bryam Vargas
  (?)
@ 2026-07-31 18:03   ` sashiko-bot
  -1 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-07-31 18:03 UTC (permalink / raw)
  Cc: selinux

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

Pre-existing issues:
- [High] The boolean symbol table can be sparse, leading to unconditional NULL pointer dereferences in `security_get_bools()`, `security_set_bools()`, and `cond_evaluate_expr()`.
--

--- Patch [3]: [PATCH 3/4] selinux: reject an unclaimed class value in security_get_classes() ---
commit 8c3d50aa472cc225281a590fe76e7aab43766da5
Author: Bryam Vargas <hexlabsecurity@proton.me>

selinux: reject an unclaimed class value in security_get_classes()

This commit updates security_get_classes() to fail the lookup if a class
value went unclaimed. This prevents a NULL pointer dereference in
sel_make_classes() when the policy defines a sparse class symbol table.

> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 90e81186cb2ee..7afce975436e9 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
[ ... ]
> @@ -3316,16 +3317,29 @@ int security_get_classes(struct selinux_policy *policy,
>  
>  	rc = hashtab_map(&policydb->p_classes.table, get_classes_callback,
>  			 *classes);
> -	if (rc) {
> -		u32 i;
> +	if (rc)
> +		goto err;
>  
> -		for (i = 0; i < *nclasses; i++)
> -			kfree((*classes)[i]);
> -		kfree(*classes);
> +	/*
> +	 * The class symtab may be sparse, which policydb_class_isvalid() exists
> +	 * to absorb; the callback fills this array by value, so an unclaimed
> +	 * one leaves a NULL that sel_make_classes() hands to sel_make_dir().
> +	 */
> +	for (i = 0; i < *nclasses; i++) {
> +		if (!(*classes)[i]) {
> +			rc = -EINVAL;
> +			goto err;
> +		}
>  	}

[Severity: High]
This is a pre-existing issue, but since this patch handles sparse class
symbol tables, does the boolean symbol table have a similar vulnerability?

In security/selinux/ss/services.c:security_get_bools(), there is an
unconditional dereference of bool_val_to_struct:

security_get_bools() {
    ...
        (*values)[i] = policydb->bool_val_to_struct[i]->state;
    ...
}

Similarly in security/selinux/ss/services.c:security_set_bools():

security_set_bools() {
    ...
        int old_state = newpolicy->policydb.bool_val_to_struct[i]->state;
    ...
}

And in security/selinux/ss/conditional.c:cond_evaluate_expr():

cond_evaluate_expr() {
    ...
        s[sp] = p->bool_val_to_struct[node->boolean - 1]->state;
    ...
}

Can a malformed policy with a sparse boolean symbol table cause a NULL
pointer dereference in these locations?

>  out:
>  	return rc;
> +
> +err:
> +	for (i = 0; i < *nclasses; i++)
> +		kfree((*classes)[i]);
> +	kfree(*classes);
> +	return rc;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-b4-disp-d32e997b-v1-0-0341d038ee47@proton.me?part=3

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

* Re: [PATCH 1/4] selinux: do not cancel a policy conversion that never started
  2026-07-31 17:44   ` Bryam Vargas
  (?)
  (?)
@ 2026-07-31 19:40   ` Stephen Smalley
  -1 siblings, 0 replies; 17+ messages in thread
From: Stephen Smalley @ 2026-07-31 19:40 UTC (permalink / raw)
  To: hexlabsecurity
  Cc: Paul Moore, linux-kernel, Ondrej Mosnacek, Kees Cook, selinux,
	Christian Göttsche

On Fri, Jul 31, 2026 at 1:44 PM Bryam Vargas via B4 Relay
<devnull+hexlabsecurity.proton.me@kernel.org> wrote:
>
> From: Bryam Vargas <hexlabsecurity@proton.me>
>
> sel_write_load() calls selinux_policy_cancel() when sel_make_policy_nodes()
> fails, and that helper dereferences the outgoing policy to cancel its
> sidtab conversion. On the first policy load there is no outgoing policy:
> security_load_policy() returns early for that case, before it converts
> anything, and state->policy is still NULL. A first load that fails while
> building the selinuxfs tree therefore takes a NULL dereference in
> selinux_policy_cancel(), reached from a write(2) to /sys/fs/selinux/load.
>
> Skip the cancel when there is no old policy, mirroring the check
> security_load_policy() already makes before it converts.
>
> Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

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

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

* Re: [PATCH 2/4] selinux: require a class's permission values to cover its permission count
  2026-07-31 17:44   ` Bryam Vargas
  (?)
  (?)
@ 2026-07-31 19:44   ` Stephen Smalley
  -1 siblings, 0 replies; 17+ messages in thread
From: Stephen Smalley @ 2026-07-31 19:44 UTC (permalink / raw)
  To: hexlabsecurity
  Cc: Paul Moore, linux-kernel, Ondrej Mosnacek, Kees Cook, selinux,
	Christian Göttsche

On Fri, Jul 31, 2026 at 1:44 PM Bryam Vargas via B4 Relay
<devnull+hexlabsecurity.proton.me@kernel.org> wrote:
>
> From: Bryam Vargas <hexlabsecurity@proton.me>
>
> security_get_permissions() sizes an array by the class's permissions.nprim
> and fills it at value - 1, from the inherited common's permission table and
> then the class's own. A value no permission defines leaves a NULL that
> sel_make_perm_files() passes to d_alloc_name(), an oops inside
> sel_write_load() that strands selinux_state.policy_mutex and leaves every
> later load in uninterruptible sleep; two permissions sharing a value
> overwrite the first kstrdup(). Bounding each value by nprim catches
> neither, and neither would a count: the symbol table is keyed on the
> permission name, so duplicates pass.
>
> Track the values each permission table claims and require them to cover
> exactly what its count declares, rejecting a count no value can reach.
> Conforming 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>

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

* Re: [PATCH 3/4] selinux: reject an unclaimed class value in security_get_classes()
  2026-07-31 17:44   ` Bryam Vargas
  (?)
  (?)
@ 2026-07-31 19:46   ` Stephen Smalley
  -1 siblings, 0 replies; 17+ messages in thread
From: Stephen Smalley @ 2026-07-31 19:46 UTC (permalink / raw)
  To: hexlabsecurity
  Cc: Paul Moore, linux-kernel, Ondrej Mosnacek, Kees Cook, selinux,
	Christian Göttsche

On Fri, Jul 31, 2026 at 1:44 PM Bryam Vargas via B4 Relay
<devnull+hexlabsecurity.proton.me@kernel.org> wrote:
>
> From: Bryam Vargas <hexlabsecurity@proton.me>
>
> security_get_classes() sizes an array by p_classes.nprim and fills it at
> value - 1, so a class value the policy never defines leaves a NULL.
> sel_make_classes() passes every entry to sel_make_dir(), reaching the same
> d_alloc_name() dereference as the permission array. The class symbol table
> is allowed to be sparse (policydb_class_isvalid() exists to absorb that),
> but this getter builds its own array straight from the hash table and has
> no such predicate.
>
> Fail the lookup when a value went unclaimed instead of handing out the
> NULL. Conforming policies define every class they declare and 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>

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

end of thread, other threads:[~2026-07-31 19:46 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-31 17:44 [PATCH 0/4] selinux: make declared counts account for the values Bryam Vargas via B4 Relay
2026-07-31 17:44 ` Bryam Vargas
2026-07-31 17:44 ` [PATCH 1/4] selinux: do not cancel a policy conversion that never started Bryam Vargas via B4 Relay
2026-07-31 17:44   ` Bryam Vargas
2026-07-31 17:58   ` sashiko-bot
2026-07-31 19:40   ` Stephen Smalley
2026-07-31 17:44 ` [PATCH 2/4] selinux: require a class's permission values to cover its permission count Bryam Vargas via B4 Relay
2026-07-31 17:44   ` Bryam Vargas
2026-07-31 17:50   ` sashiko-bot
2026-07-31 19:44   ` Stephen Smalley
2026-07-31 17:44 ` [PATCH 3/4] selinux: reject an unclaimed class value in security_get_classes() Bryam Vargas via B4 Relay
2026-07-31 17:44   ` Bryam Vargas
2026-07-31 18:03   ` sashiko-bot
2026-07-31 19:46   ` Stephen Smalley
2026-07-31 17:44 ` [PATCH 4/4] selinux: require every boolean value to be defined Bryam Vargas via B4 Relay
2026-07-31 17:44   ` Bryam Vargas
2026-07-31 18:01   ` sashiko-bot

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.