[PATCH v2 2/5] selinux: restore the context string on -ESTALE retry

[PATCH v2 2/5] selinux: restore the context string on -ESTALE retry

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

security_context_to_sid_core() retries the conversion when
sidtab_context_to_sid() returns -ESTALE because a policy reload froze
the sidtab in the meantime.  The retry passes scontext2 to
string_to_context_struct() again, but that function mutates its input:
it replaces the ':' separators with NUL bytes while parsing.  On the
second pass the user field is followed by a NUL instead of a ':', so the
parse fails with -EINVAL for any context, including a perfectly valid
one.

Without force this makes the caller see -EINVAL for a valid context.
With force the (pristine) saved copy gets stored as an uninterpreted,
unmapped context, so a valid context is treated as unlabeled until the
next policy reload.

Restore scontext2 from the caller's buffer before retrying, as
services_convert_context() already does for the same reason.  The
buffer was allocated by kmemdup_nul() with scontext_len + 1 bytes and
the parser never writes past the terminating NUL, so copying
scontext_len bytes restores it exactly.

Fixes: 9ad6e9cb39c6 ("selinux: fix race between old and new sidtab")
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
---
 security/selinux/ss/services.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index a6e4bf262a9e..b1987367e8b0 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1614,6 +1614,8 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
 			context.str = NULL;
 		}
 		context_destroy(&context);
+		/* string_to_context_struct() mutated scontext2, restore it */
+		memcpy(scontext2, scontext, scontext_len);
 		goto retry;
 	}
 	context_destroy(&context);
-- 
2.55.0

[PATCH v2 3/5] selinux: prevent stale AVC insertions after policy changes

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

A cache miss can compute a decision under the old policy and pass the
latest_notif check in avc_insert(), then be delayed until after a policy
change has flushed the AVC and updated latest_notif.  Inserting the old
decision at that point allows subsequent cache hits to keep using the
old permissions, including permissions revoked by the new policy.

Keep security_compute_av() and avc_insert() in one RCU read-side
critical section.  Both selinux_policy_commit() and security_set_bools()
publish the new policy and wait for an RCU grace period before flushing
the AVC.  This ensures that insertions based on the old policy finish
before the flush.  The sequence-number check alone cannot provide that
guarantee, even when it takes the notification lock.

The insertion path uses nonblocking allocations and already runs under
RCU when called from avc_has_extended_perms(), where the new read-side
critical section simply nests within the existing one.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com> (code wise,
  inline comments updated in v2)
---
v2:
  - document the grace period ordering requirement at the
    synchronize_rcu() call sites in selinux_policy_commit() and
    security_set_bools()
---
 security/selinux/avc.c         | 7 +++++++
 security/selinux/ss/services.c | 9 +++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 2a9d4888384b..6dcb4b354d7c 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -993,8 +993,15 @@ static noinline void avc_compute_av(u32 ssid, u32 tsid, u16 tclass,
 				    struct avc_xperms_node *xp_node)
 {
 	INIT_LIST_HEAD(&xp_node->xpd_head);
+	/*
+	 * Compute and insert under one RCU read lock: policy changes wait
+	 * for a grace period before flushing the AVC, so a decision based
+	 * on the old policy cannot be inserted after the flush.
+	 */
+	rcu_read_lock();
 	security_compute_av(ssid, tsid, tclass, avd, &xp_node->xp);
 	avc_insert(ssid, tsid, tclass, avd, xp_node);
+	rcu_read_unlock();
 }
 
 static noinline int avc_denied(u32 ssid, u32 tsid, u16 tclass, u32 requested,
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index b1987367e8b0..79b24500ca7b 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2288,7 +2288,10 @@ void selinux_policy_commit(struct selinux_load_state *load_state)
 		selinux_complete_init();
 	}
 
-	/* Free the old policy */
+	/*
+	 * Free the old policy.  The grace period must also precede the AVC
+	 * flush (see avc_compute_av()); do not convert to call_rcu().
+	 */
 	synchronize_rcu();
 	selinux_policy_free(oldpolicy);
 	kfree(load_state->convert_data);
@@ -3037,7 +3040,9 @@ int security_set_bools(u32 len, const int *values)
 	/*
 	 * Free the conditional portions of the old policydb
 	 * that were copied for the new policy, and the oldpolicy
-	 * structure itself but not what it references.
+	 * structure itself but not what it references.  The grace
+	 * period must also precede the AVC flush (see avc_compute_av());
+	 * do not convert to call_rcu().
 	 */
 	synchronize_rcu();
 	selinux_policy_cond_free(oldpolicy);
-- 
2.55.0

Re: [PATCH v2 3/5] selinux: prevent stale AVC insertions after policy changes

[sashiko-bot]

> From: Christian Göttsche <cgzones@googlemail.com>
> 
> A cache miss can compute a decision under the old policy and pass the
> latest_notif check in avc_insert(), then be delayed until after a policy
> change has flushed the AVC and updated latest_notif.  Inserting the old
> decision at that point allows subsequent cache hits to keep using the
> old permissions, including permissions revoked by the new policy.
> 
> Keep security_compute_av() and avc_insert() in one RCU read-side
> critical section.  Both selinux_policy_commit() and security_set_bools()
> publish the new policy and wait for an RCU grace period before flushing
> the AVC.  This ensures that insertions based on the old policy finish
> before the flush.  The sequence-number check alone cannot provide that
> guarantee, even when it takes the notification lock.
> 
> The insertion path uses nonblocking allocations and already runs under
> RCU when called from avc_has_extended_perms(), where the new read-side
> critical section simply nests within the existing one.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109153802.76176-5-cgoettsche@seltendoof.de?part=3

[PATCH v2 4/5] selinux: keep pending boolean values across policy loads

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Setting a boolean via selinuxfs takes two steps: writing the pending
value to /sys/fs/selinux/booleans/<name> and then committing all pending
values via /sys/fs/selinux/commit_pending_bools.  A policy load in
between recreates the pending values from the committed ones of the new
policy, so the subsequent commit silently drops the change, while both
writes report success.  With concurrent policy loads, setsebool(8)
therefore occasionally loses updates.

Carry over the pending values of the booleans present in both the old
and the new policy.  For booleans without a pending change this is a
no-op, since security_preserve_bools() already transfers the committed
values to the new policy.  Both the pending values and the policy load
are serialized by the policy mutex.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
---
v2: drop Fixes tag since Stephen mentioned this was not an API guarantee
---
 security/selinux/include/conditional.h |  2 ++
 security/selinux/selinuxfs.c           | 23 +++++++++++++++++++++++
 security/selinux/ss/services.c         | 16 ++++++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/security/selinux/include/conditional.h b/security/selinux/include/conditional.h
index 060833e2dba2..aa29189179fa 100644
--- a/security/selinux/include/conditional.h
+++ b/security/selinux/include/conditional.h
@@ -20,4 +20,6 @@ int security_set_bools(u32 len, const int *values);
 
 int security_get_bool_value(u32 index);
 
+int security_get_bool_index(struct selinux_policy *policy, const char *name);
+
 #endif
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 310e19efff1d..96a33b7d2b7b 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -447,6 +447,27 @@ static void sel_remove_old_bool_data(unsigned int bool_num, char **bool_names,
 	kfree(bool_values);
 }
 
+/*
+ * The pending boolean values of a new policy start out as its committed
+ * values.  Carry over the pending values of the booleans that exist in both
+ * policies, so a value written but not yet committed is not silently lost
+ * if a policy load happens in between.
+ */
+static void sel_keep_pending_bools(const struct selinux_fs_info *fsi,
+				   struct selinux_policy *newpolicy,
+				   int *bool_values)
+{
+	unsigned int i;
+	int index;
+
+	for (i = 0; i < fsi->bool_num; i++) {
+		index = security_get_bool_index(newpolicy,
+						fsi->bool_pending_names[i]);
+		if (index >= 0)
+			bool_values[index] = fsi->bool_pending_values[i];
+	}
+}
+
 static int sel_make_policy_nodes(struct selinux_fs_info *fsi,
 				struct selinux_policy *newpolicy)
 {
@@ -481,6 +502,8 @@ static int sel_make_policy_nodes(struct selinux_fs_info *fsi,
 	if (ret)
 		goto out;
 
+	sel_keep_pending_bools(fsi, newpolicy, bool_values);
+
 	ret = sel_make_classes(newpolicy, tmp_class_dir,
 			       &fsi->last_class_ino);
 	if (ret)
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 79b24500ca7b..19acbb1ee436 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -3077,6 +3077,22 @@ int security_get_bool_value(u32 index)
 	return rc;
 }
 
+/*
+ * security_get_bool_index() - look up the index of a boolean by name in the
+ * given policy, e.g. one not yet committed.  Returns -ENOENT if the policy
+ * does not define the boolean.
+ */
+int security_get_bool_index(struct selinux_policy *policy, const char *name)
+{
+	struct cond_bool_datum *booldatum;
+
+	booldatum = symtab_search(&policy->policydb.p_bools, name);
+	if (!booldatum)
+		return -ENOENT;
+
+	return booldatum->value - 1;
+}
+
 static int security_preserve_bools(struct selinux_policy *oldpolicy,
 				struct selinux_policy *newpolicy)
 {
-- 
2.55.0

Re: [PATCH v2 4/5] selinux: keep pending boolean values across policy loads

[sashiko-bot]

> From: Christian Göttsche <cgzones@googlemail.com>
> 
> Setting a boolean via selinuxfs takes two steps: writing the pending
> value to /sys/fs/selinux/booleans/<name> and then committing all pending
> values via /sys/fs/selinux/commit_pending_bools.  A policy load in
> between recreates the pending values from the committed ones of the new
> policy, so the subsequent commit silently drops the change, while both
> writes report success.  With concurrent policy loads, setsebool(8)
> therefore occasionally loses updates.
> 
> Carry over the pending values of the booleans present in both the old
> and the new policy.  For booleans without a pending change this is a
> no-op, since security_preserve_bools() already transfers the committed
> values to the new policy.  Both the pending values and the policy load
> are serialized by the policy mutex.
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109153802.76176-5-cgoettsche@seltendoof.de?part=4

[PATCH v2 5/5] selinux: do not cache permissive grants after enabling enforcing mode

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

In permissive mode avc_denied() allows a denied access and records the
grant in the AVC node via avc_update_node(AVC_CALLBACK_GRANT).  Enabling
enforcing mode relies on the subsequent avc_ss_reset() to flush such
grants.  The enforcing check and the node update are not atomic though:

  CPU A (permissive check)     CPU B (setenforce 1)  CPU C (same access)
  avc_denied()
    enforcing_enabled() false
                               enforcing_set(1)
                               avc_flush()
                                                     cache miss,
                                                     avc_insert()
    avc_update_node(GRANT)
      finds the new node

Toggling enforcing mode does not change the policy seqno, so the node
inserted by CPU C has the same seqno as the decision CPU A checked, and
the seqno check in avc_update_node() does not catch it.  The grant then
stays cached in enforcing mode, and subsequent checks allow the access
without a denial until the node is flushed or reclaimed.  The same
applies to extended permission grants.

Recheck the enforcing mode under the bucket lock before applying a
grant, unless the domain is permissive.  avc_flush() takes every bucket
lock after enforcing_set(), so an update that takes the lock after the
flush of its bucket observes enforcing mode, while any grant applied
before is removed by the flush.  The current access is still allowed,
as it was checked before the switch; only the grant is not cached.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
---
 security/selinux/avc.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 6dcb4b354d7c..9064f688f553 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -872,6 +872,19 @@ static int avc_update_node(u32 event, u32 perms, u8 driver, u8 base_perm,
 		goto out_unlock;
 	}
 
+	/*
+	 * A grant made in permissive mode must not be cached once enforcing
+	 * mode is enabled.  A concurrent switch to enforcing mode can flush
+	 * the node checked by avc_denied(), after which a new node with the
+	 * same seqno can be inserted.  avc_flush() takes every bucket lock
+	 * after enforcing_set(), so checking again under the lock suffices.
+	 */
+	if (event == AVC_CALLBACK_GRANT && enforcing_enabled() &&
+	    !(orig->ae.avd.flags & AVD_FLAGS_PERMISSIVE)) {
+		avc_node_kill(node);
+		goto out_unlock;
+	}
+
 	/*
 	 * Copy and replace original node.
 	 */
-- 
2.55.0

Re: [PATCH v2 5/5] selinux: do not cache permissive grants after enabling enforcing mode

[sashiko-bot]

> From: Christian Göttsche <cgzones@googlemail.com>
> 
> In permissive mode avc_denied() allows a denied access and records the
> grant in the AVC node via avc_update_node(AVC_CALLBACK_GRANT).  Enabling
> enforcing mode relies on the subsequent avc_ss_reset() to flush such
> grants.  The enforcing check and the node update are not atomic though:
> 
>   CPU A (permissive check)     CPU B (setenforce 1)  CPU C (same access)
>   avc_denied()
>     enforcing_enabled() false
>                                enforcing_set(1)
>                                avc_flush()
>                                                      cache miss,
>                                                      avc_insert()
>     avc_update_node(GRANT)
> [ ... ]
> before is removed by the flush.  The current access is still allowed,
> as it was checked before the switch; only the grant is not cached.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109153802.76176-5-cgoettsche@seltendoof.de?part=5

[PATCH v2 1/5] selinux: zero-initialize permission name array in masked av dump

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

security_dump_masked_av() declares an on-stack array

	char *permission_names[SEL_VEC_MAX];

and only fills the entries that correspond to permissions actually
defined for the target class (and its common) via hashtab_map() and
dump_masked_av_helper().  It then walks the set bits of the masked
access vector and logs, for each bit, permission_names[index] or the
"????" fallback when the slot is NULL.

The array is never initialized, so any set bit whose index has no
defined permission for the class reads an uninitialized stack pointer,
which is then passed to audit_log_format("%s").

Zero-initialize the array so undefined slots are NULL and are reported
as "????", exactly as the existing fallback intends.  Rejecting such AV
rules at load time is deliberately not done here: it would break
policies that older toolchains legitimately produced.

Fixes: 44c2d9bdd702 ("Add audit messages on type boundary violations")
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
---
v2: cc public mailing list
---
 security/selinux/ss/services.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index da77ad769c83..a6e4bf262a9e 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -464,7 +464,7 @@ static void security_dump_masked_av(struct policydb *policydb,
 	const char *tclass_name;
 	char *scontext_name = NULL;
 	char *tcontext_name = NULL;
-	char *permission_names[SEL_VEC_MAX];
+	char *permission_names[SEL_VEC_MAX] = {};
 	int index;
 	u32 length;
 	bool need_comma = false;
-- 
2.55.0

Re: [PATCH v2 1/5] selinux: zero-initialize permission name array in masked av dump

[sashiko-bot]

> From: Christian Göttsche <cgzones@googlemail.com>
> 
> security_dump_masked_av() declares an on-stack array
> 
> 	char *permission_names[SEL_VEC_MAX];
> 
> and only fills the entries that correspond to permissions actually
> defined for the target class (and its common) via hashtab_map() and
> dump_masked_av_helper().  It then walks the set bits of the masked
> access vector and logs, for each bit, permission_names[index] or the
> "????" fallback when the slot is NULL.
> 
> The array is never initialized, so any set bit whose index has no
> defined permission for the class reads an uninitialized stack pointer,
> which is then passed to audit_log_format("%s").
> 
> Zero-initialize the array so undefined slots are NULL and are reported
> as "????", exactly as the existing fallback intends.  Rejecting such AV
> rules at load time is deliberately not done here: it would break
> policies that older toolchains legitimately produced.
> 
> Fixes: 44c2d9bdd702 ("Add audit messages on type boundary violations")
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109153802.76176-5-cgoettsche@seltendoof.de?part=1

Re: [PATCH v2 1/5] selinux: zero-initialize permission name array in masked av dump

[Stephen Smalley]

On Mon, Nov 9, 2026 at 10:38 AM Christian Göttsche
<cgoettsche@seltendoof.de> wrote:
>
> From: Christian Göttsche <cgzones@googlemail.com>
>
> security_dump_masked_av() declares an on-stack array
>
>         char *permission_names[SEL_VEC_MAX];
>
> and only fills the entries that correspond to permissions actually
> defined for the target class (and its common) via hashtab_map() and
> dump_masked_av_helper().  It then walks the set bits of the masked
> access vector and logs, for each bit, permission_names[index] or the
> "????" fallback when the slot is NULL.
>
> The array is never initialized, so any set bit whose index has no
> defined permission for the class reads an uninitialized stack pointer,
> which is then passed to audit_log_format("%s").
>
> Zero-initialize the array so undefined slots are NULL and are reported
> as "????", exactly as the existing fallback intends.  Rejecting such AV
> rules at load time is deliberately not done here: it would break
> policies that older toolchains legitimately produced.
>
> Fixes: 44c2d9bdd702 ("Add audit messages on type boundary violations")
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> ---
> v2: cc public mailing list
> ---

Just FTR, my Acked-by for this series remains valid.

Re: [PATCH v2 2/5] selinux: restore the context string on -ESTALE retry

[sashiko-bot]

> From: Christian Göttsche <cgzones@googlemail.com>
> 
> security_context_to_sid_core() retries the conversion when
> sidtab_context_to_sid() returns -ESTALE because a policy reload froze
> the sidtab in the meantime.  The retry passes scontext2 to
> string_to_context_struct() again, but that function mutates its input:
> it replaces the ':' separators with NUL bytes while parsing.  On the
> second pass the user field is followed by a NUL instead of a ':', so the
> parse fails with -EINVAL for any context, including a perfectly valid
> one.
> 
> Without force this makes the caller see -EINVAL for a valid context.
> With force the (pristine) saved copy gets stored as an uninterpreted,
> unmapped context, so a valid context is treated as unlabeled until the
> next policy reload.
> 
> Restore scontext2 from the caller's buffer before retrying, as
> services_convert_context() already does for the same reason.  The
> buffer was allocated by kmemdup_nul() with scontext_len + 1 bytes and
> the parser never writes past the terminating NUL, so copying
> scontext_len bytes restores it exactly.
> 
> Fixes: 9ad6e9cb39c6 ("selinux: fix race between old and new sidtab")
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109153802.76176-5-cgoettsche@seltendoof.de?part=2