[PATCH v2 0/2] selinux: policy load fixes

[PATCH v2 0/2] selinux: policy load fixes

[Ondrej Mosnacek]

Changes in v2:
- switch to a more minimal fix which allocates the conversion params
  dynamically and passes them between the functions
- split out the MAC_POLICY_LOAD record fix into a separate patch

Ondrej Mosnacek (2):
  selinux: don't log MAC_POLICY_LOAD record on failed policy load
  selinux: fix variable scope issue in live sidtab conversion

 security/selinux/include/security.h | 15 ++++++---
 security/selinux/selinuxfs.c        | 13 ++++----
 security/selinux/ss/services.c      | 51 +++++++++++++++++++----------
 3 files changed, 50 insertions(+), 29 deletions(-)

-- 
2.29.2

[PATCH v2 1/2] selinux: don't log MAC_POLICY_LOAD record on failed policy load

[Ondrej Mosnacek]

If sel_make_policy_nodes() fails, we should jump to 'out', not 'out1',
as the latter would incorrectly log an MAC_POLICY_LOAD audit record,
even though the policy hasn't actually been reloaded. The 'out1' jump
label now becomes unused and can be removed.

Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/selinuxfs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 01a7d50ed39b..340711e3dc9a 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -651,14 +651,13 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf,
 	length = sel_make_policy_nodes(fsi, newpolicy);
 	if (length) {
 		selinux_policy_cancel(fsi->state, newpolicy);
-		goto out1;
+		goto out;
 	}
 
 	selinux_policy_commit(fsi->state, newpolicy);
 
 	length = count;
 
-out1:
 	audit_log(audit_context(), GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
 		"auid=%u ses=%u lsm=selinux res=1",
 		from_kuid(&init_user_ns, audit_get_loginuid(current)),
-- 
2.29.2

[PATCH v2 2/2] selinux: fix variable scope issue in live sidtab conversion

[Ondrej Mosnacek]

Commit 02a52c5c8c3b ("selinux: move policy commit after updating
selinuxfs") moved the selinux_policy_commit() call out of
security_load_policy() into sel_write_load(), which caused a subtle yet
rather serious bug.

The problem is that security_load_policy() passes a reference to the
convert_params local variable to sidtab_convert(), which stores it in
the sidtab, where it may be accessed until the policy is swapped over
and RCU synchronized. Before 02a52c5c8c3b, selinux_policy_commit() was
called directly from security_load_policy(), so the convert_params
pointer remained valid all the way until the old sidtab was destroyed,
but now that's no longer the case and calls to sidtab_context_to_sid()
on the old sidtab after security_load_policy() returns may cause invalid
memory accesses.

This can be easily triggered using the stress test from commit
ee1a84fdfeed ("selinux: overhaul sidtab to fix bug and improve
performance"):
```
function rand_cat() {
	echo $(( $RANDOM % 1024 ))
}

function do_work() {
	while true; do
		echo -n "system_u:system_r:kernel_t:s0:c$(rand_cat),c$(rand_cat)" \
			>/sys/fs/selinux/context 2>/dev/null || true
	done
}

do_work >/dev/null &
do_work >/dev/null &
do_work >/dev/null &

while load_policy; do echo -n .; sleep 0.1; done

kill %1
kill %2
kill %3
```

Fix this by allocating the temporary sidtab convert structures
dynamically and passing them among the
selinux_policy_{load,cancel,commit} functions.

Note that this commit also fixes the minor issue of logging a
MAC_POLICY_LOAD audit record in case sel_make_policy_nodes() fails (in
which case the new policy isn't actually loaded).

Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/include/security.h | 15 ++++++---
 security/selinux/selinuxfs.c        | 10 +++---
 security/selinux/ss/services.c      | 51 +++++++++++++++++++----------
 3 files changed, 49 insertions(+), 27 deletions(-)

diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 765a258a899e..25db66e0ac51 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -219,14 +219,21 @@ static inline bool selinux_policycap_genfs_seclabel_symlinks(void)
 	return READ_ONCE(state->policycap[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS]);
 }
 
+struct selinux_policy_convert_data;
+
+struct selinux_load_state {
+	struct selinux_policy *policy;
+	struct selinux_policy_convert_data *convert_data;
+};
+
 int security_mls_enabled(struct selinux_state *state);
 int security_load_policy(struct selinux_state *state,
-			void *data, size_t len,
-			struct selinux_policy **newpolicyp);
+			 void *data, size_t len,
+			 struct selinux_load_state *load_state);
 void selinux_policy_commit(struct selinux_state *state,
-			struct selinux_policy *newpolicy);
+			   struct selinux_load_state *load_state);
 void selinux_policy_cancel(struct selinux_state *state,
-			struct selinux_policy *policy);
+			   struct selinux_load_state *load_state);
 int security_read_policy(struct selinux_state *state,
 			 void **data, size_t *len);
 
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 340711e3dc9a..158d44ea93f4 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -616,7 +616,7 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf,
 
 {
 	struct selinux_fs_info *fsi = file_inode(file)->i_sb->s_fs_info;
-	struct selinux_policy *newpolicy;
+	struct selinux_load_state load_state;
 	ssize_t length;
 	void *data = NULL;
 
@@ -642,19 +642,19 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf,
 	if (copy_from_user(data, buf, count) != 0)
 		goto out;
 
-	length = security_load_policy(fsi->state, data, count, &newpolicy);
+	length = security_load_policy(fsi->state, data, count, &load_state);
 	if (length) {
 		pr_warn_ratelimited("SELinux: failed to load policy\n");
 		goto out;
 	}
 
-	length = sel_make_policy_nodes(fsi, newpolicy);
+	length = sel_make_policy_nodes(fsi, load_state.policy);
 	if (length) {
-		selinux_policy_cancel(fsi->state, newpolicy);
+		selinux_policy_cancel(fsi->state, &load_state);
 		goto out;
 	}
 
-	selinux_policy_commit(fsi->state, newpolicy);
+	selinux_policy_commit(fsi->state, &load_state);
 
 	length = count;
 
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 5e08ce2c5994..fada4ebc7ef8 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2157,8 +2157,13 @@ static void selinux_policy_cond_free(struct selinux_policy *policy)
 	kfree(policy);
 }
 
+struct selinux_policy_convert_data {
+	struct convert_context_args args;
+	struct sidtab_convert_params sidtab_params;
+};
+
 void selinux_policy_cancel(struct selinux_state *state,
-			struct selinux_policy *policy)
+			   struct selinux_load_state *load_state)
 {
 	struct selinux_policy *oldpolicy;
 
@@ -2166,7 +2171,8 @@ void selinux_policy_cancel(struct selinux_state *state,
 					lockdep_is_held(&state->policy_mutex));
 
 	sidtab_cancel_convert(oldpolicy->sidtab);
-	selinux_policy_free(policy);
+	selinux_policy_free(load_state->policy);
+	kfree(load_state->convert_data);
 }
 
 static void selinux_notify_policy_change(struct selinux_state *state,
@@ -2181,9 +2187,9 @@ static void selinux_notify_policy_change(struct selinux_state *state,
 }
 
 void selinux_policy_commit(struct selinux_state *state,
-			struct selinux_policy *newpolicy)
+			   struct selinux_load_state *load_state)
 {
-	struct selinux_policy *oldpolicy;
+	struct selinux_policy *oldpolicy, *newpolicy = load_state->policy;
 	u32 seqno;
 
 	oldpolicy = rcu_dereference_protected(state->policy,
@@ -2223,6 +2229,7 @@ void selinux_policy_commit(struct selinux_state *state,
 	/* Free the old policy */
 	synchronize_rcu();
 	selinux_policy_free(oldpolicy);
+	kfree(load_state->convert_data);
 
 	/* Notify others of the policy change */
 	selinux_notify_policy_change(state, seqno);
@@ -2239,11 +2246,10 @@ void selinux_policy_commit(struct selinux_state *state,
  * loading the new policy.
  */
 int security_load_policy(struct selinux_state *state, void *data, size_t len,
-			struct selinux_policy **newpolicyp)
+			 struct selinux_load_state *load_state)
 {
 	struct selinux_policy *newpolicy, *oldpolicy;
-	struct sidtab_convert_params convert_params;
-	struct convert_context_args args;
+	struct selinux_policy_convert_data *convert_data;
 	int rc = 0;
 	struct policy_file file = { data, len }, *fp = &file;
 
@@ -2273,10 +2279,10 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len,
 		goto err_mapping;
 	}
 
-
 	if (!selinux_initialized(state)) {
 		/* First policy load, so no need to preserve state from old policy */
-		*newpolicyp = newpolicy;
+		load_state->policy = newpolicy;
+		load_state->convert_data = NULL;
 		return 0;
 	}
 
@@ -2290,29 +2296,38 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len,
 		goto err_free_isids;
 	}
 
+	convert_data = kmalloc(sizeof(*convert_data), GFP_KERNEL);
+	if (!convert_data) {
+		rc = -ENOMEM;
+		goto err_free_isids;
+	}
+
 	/*
 	 * Convert the internal representations of contexts
 	 * in the new SID table.
 	 */
-	args.state = state;
-	args.oldp = &oldpolicy->policydb;
-	args.newp = &newpolicy->policydb;
+	convert_data->args.state = state;
+	convert_data->args.oldp = &oldpolicy->policydb;
+	convert_data->args.newp = &newpolicy->policydb;
 
-	convert_params.func = convert_context;
-	convert_params.args = &args;
-	convert_params.target = newpolicy->sidtab;
+	convert_data->sidtab_params.func = convert_context;
+	convert_data->sidtab_params.args = &convert_data->args;
+	convert_data->sidtab_params.target = newpolicy->sidtab;
 
-	rc = sidtab_convert(oldpolicy->sidtab, &convert_params);
+	rc = sidtab_convert(oldpolicy->sidtab, &convert_data->sidtab_params);
 	if (rc) {
 		pr_err("SELinux:  unable to convert the internal"
 			" representation of contexts in the new SID"
 			" table\n");
-		goto err_free_isids;
+		goto err_free_convert_data;
 	}
 
-	*newpolicyp = newpolicy;
+	load_state->policy = newpolicy;
+	load_state->convert_data = convert_data;
 	return 0;
 
+err_free_convert_data:
+	kfree(convert_data);
 err_free_isids:
 	sidtab_destroy(newpolicy->sidtab);
 err_mapping:
-- 
2.29.2

Re: [PATCH v2 1/2] selinux: don't log MAC_POLICY_LOAD record on failed policy load

[Paul Moore]

On Fri, Feb 12, 2021 at 1:59 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> If sel_make_policy_nodes() fails, we should jump to 'out', not 'out1',
> as the latter would incorrectly log an MAC_POLICY_LOAD audit record,
> even though the policy hasn't actually been reloaded. The 'out1' jump
> label now becomes unused and can be removed.
>
> Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/selinuxfs.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 01a7d50ed39b..340711e3dc9a 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -651,14 +651,13 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf,
>         length = sel_make_policy_nodes(fsi, newpolicy);
>         if (length) {
>                 selinux_policy_cancel(fsi->state, newpolicy);
> -               goto out1;
> +               goto out;

This looks good, especially with AUDIT_MAC_POLICY_LOAD recording
"res=1".  However, now that I'm looking at the error path here, we
don't display anything if sel_make_policy_nodes() fails, do we?  If
security_load_policy fails we at least do a printk(), but if this
fails it silently kills the policy load; at the very least I think we
want a `pr_warn_ratelimited("SELinux: failed to load policy due to
selinuxfs failures")` or something similar.

>         }
>
>         selinux_policy_commit(fsi->state, newpolicy);
>
>         length = count;
>
> -out1:
>         audit_log(audit_context(), GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
>                 "auid=%u ses=%u lsm=selinux res=1",
>                 from_kuid(&init_user_ns, audit_get_loginuid(current)),
> --
> 2.29.2

-- 
paul moore
www.paul-moore.com

Re: [PATCH v2 2/2] selinux: fix variable scope issue in live sidtab conversion

[Paul Moore]

On Fri, Feb 12, 2021 at 1:59 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> Commit 02a52c5c8c3b ("selinux: move policy commit after updating
> selinuxfs") moved the selinux_policy_commit() call out of
> security_load_policy() into sel_write_load(), which caused a subtle yet
> rather serious bug.
>
> The problem is that security_load_policy() passes a reference to the
> convert_params local variable to sidtab_convert(), which stores it in
> the sidtab, where it may be accessed until the policy is swapped over
> and RCU synchronized. Before 02a52c5c8c3b, selinux_policy_commit() was
> called directly from security_load_policy(), so the convert_params
> pointer remained valid all the way until the old sidtab was destroyed,
> but now that's no longer the case and calls to sidtab_context_to_sid()
> on the old sidtab after security_load_policy() returns may cause invalid
> memory accesses.
>
> This can be easily triggered using the stress test from commit
> ee1a84fdfeed ("selinux: overhaul sidtab to fix bug and improve
> performance"):
> ```
> function rand_cat() {
>         echo $(( $RANDOM % 1024 ))
> }
>
> function do_work() {
>         while true; do
>                 echo -n "system_u:system_r:kernel_t:s0:c$(rand_cat),c$(rand_cat)" \
>                         >/sys/fs/selinux/context 2>/dev/null || true
>         done
> }
>
> do_work >/dev/null &
> do_work >/dev/null &
> do_work >/dev/null &
>
> while load_policy; do echo -n .; sleep 0.1; done
>
> kill %1
> kill %2
> kill %3
> ```
>
> Fix this by allocating the temporary sidtab convert structures
> dynamically and passing them among the
> selinux_policy_{load,cancel,commit} functions.
>
> Note that this commit also fixes the minor issue of logging a
> MAC_POLICY_LOAD audit record in case sel_make_policy_nodes() fails (in
> which case the new policy isn't actually loaded).

I think you forgot to remove the paragraph above :)

Other than that, and a small nit (below), this looks good to me.

> Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/include/security.h | 15 ++++++---
>  security/selinux/selinuxfs.c        | 10 +++---
>  security/selinux/ss/services.c      | 51 +++++++++++++++++++----------
>  3 files changed, 49 insertions(+), 27 deletions(-)

...

> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 5e08ce2c5994..fada4ebc7ef8 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2157,8 +2157,13 @@ static void selinux_policy_cond_free(struct selinux_policy *policy)
>         kfree(policy);
>  }
>
> +struct selinux_policy_convert_data {
> +       struct convert_context_args args;
> +       struct sidtab_convert_params sidtab_params;
> +};

I generally prefer structs up at the top of the source file, before
the forward declarations please.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v2 1/2] selinux: don't log MAC_POLICY_LOAD record on failed policy load

[Ondrej Mosnacek]

On Thu, Feb 25, 2021 at 7:15 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Feb 12, 2021 at 1:59 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > If sel_make_policy_nodes() fails, we should jump to 'out', not 'out1',
> > as the latter would incorrectly log an MAC_POLICY_LOAD audit record,
> > even though the policy hasn't actually been reloaded. The 'out1' jump
> > label now becomes unused and can be removed.
> >
> > Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  security/selinux/selinuxfs.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> > index 01a7d50ed39b..340711e3dc9a 100644
> > --- a/security/selinux/selinuxfs.c
> > +++ b/security/selinux/selinuxfs.c
> > @@ -651,14 +651,13 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf,
> >         length = sel_make_policy_nodes(fsi, newpolicy);
> >         if (length) {
> >                 selinux_policy_cancel(fsi->state, newpolicy);
> > -               goto out1;
> > +               goto out;
>
> This looks good, especially with AUDIT_MAC_POLICY_LOAD recording
> "res=1".  However, now that I'm looking at the error path here, we
> don't display anything if sel_make_policy_nodes() fails, do we?  If
> security_load_policy fails we at least do a printk(), but if this
> fails it silently kills the policy load; at the very least I think we
> want a `pr_warn_ratelimited("SELinux: failed to load policy due to
> selinuxfs failures")` or something similar.

There are error messages in some error paths in
sel_make_policy_nodes(), but not all. Those are pr_err()s, while in
sel_write_load() there is a pr_warn_ratelimited(). Could we just unify
the sel_make_policy_nodes() failure to a single message? (I don't
think the information on which part has failed is very useful as the
most likely cause here is a memory allocation failure, not bad
policy.) If so, should it be a pr_warn() or pr_err()? Ratelimited or
not?

>
> >         }
> >
> >         selinux_policy_commit(fsi->state, newpolicy);
> >
> >         length = count;
> >
> > -out1:
> >         audit_log(audit_context(), GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
> >                 "auid=%u ses=%u lsm=selinux res=1",
> >                 from_kuid(&init_user_ns, audit_get_loginuid(current)),
> > --
> > 2.29.2
>
> --
> paul moore
> www.paul-moore.com
>

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH v2 2/2] selinux: fix variable scope issue in live sidtab conversion

[Ondrej Mosnacek]

On Thu, Feb 25, 2021 at 8:20 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Feb 12, 2021 at 1:59 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > Commit 02a52c5c8c3b ("selinux: move policy commit after updating
> > selinuxfs") moved the selinux_policy_commit() call out of
> > security_load_policy() into sel_write_load(), which caused a subtle yet
> > rather serious bug.
> >
> > The problem is that security_load_policy() passes a reference to the
> > convert_params local variable to sidtab_convert(), which stores it in
> > the sidtab, where it may be accessed until the policy is swapped over
> > and RCU synchronized. Before 02a52c5c8c3b, selinux_policy_commit() was
> > called directly from security_load_policy(), so the convert_params
> > pointer remained valid all the way until the old sidtab was destroyed,
> > but now that's no longer the case and calls to sidtab_context_to_sid()
> > on the old sidtab after security_load_policy() returns may cause invalid
> > memory accesses.
> >
> > This can be easily triggered using the stress test from commit
> > ee1a84fdfeed ("selinux: overhaul sidtab to fix bug and improve
> > performance"):
> > ```
> > function rand_cat() {
> >         echo $(( $RANDOM % 1024 ))
> > }
> >
> > function do_work() {
> >         while true; do
> >                 echo -n "system_u:system_r:kernel_t:s0:c$(rand_cat),c$(rand_cat)" \
> >                         >/sys/fs/selinux/context 2>/dev/null || true
> >         done
> > }
> >
> > do_work >/dev/null &
> > do_work >/dev/null &
> > do_work >/dev/null &
> >
> > while load_policy; do echo -n .; sleep 0.1; done
> >
> > kill %1
> > kill %2
> > kill %3
> > ```
> >
> > Fix this by allocating the temporary sidtab convert structures
> > dynamically and passing them among the
> > selinux_policy_{load,cancel,commit} functions.
> >
> > Note that this commit also fixes the minor issue of logging a
> > MAC_POLICY_LOAD audit record in case sel_make_policy_nodes() fails (in
> > which case the new policy isn't actually loaded).
>
> I think you forgot to remove the paragraph above :)

Oh, good point :)

>
> Other than that, and a small nit (below), this looks good to me.
>
> > Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  security/selinux/include/security.h | 15 ++++++---
> >  security/selinux/selinuxfs.c        | 10 +++---
> >  security/selinux/ss/services.c      | 51 +++++++++++++++++++----------
> >  3 files changed, 49 insertions(+), 27 deletions(-)
>
> ...
>
> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index 5e08ce2c5994..fada4ebc7ef8 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -2157,8 +2157,13 @@ static void selinux_policy_cond_free(struct selinux_policy *policy)
> >         kfree(policy);
> >  }
> >
> > +struct selinux_policy_convert_data {
> > +       struct convert_context_args args;
> > +       struct sidtab_convert_params sidtab_params;
> > +};
>
> I generally prefer structs up at the top of the source file, before
> the forward declarations please.

Ok, I'll move it to the top.

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH v2 1/2] selinux: don't log MAC_POLICY_LOAD record on failed policy load

[Paul Moore]

On Fri, Feb 26, 2021 at 9:46 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Thu, Feb 25, 2021 at 7:15 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Fri, Feb 12, 2021 at 1:59 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > If sel_make_policy_nodes() fails, we should jump to 'out', not 'out1',
> > > as the latter would incorrectly log an MAC_POLICY_LOAD audit record,
> > > even though the policy hasn't actually been reloaded. The 'out1' jump
> > > label now becomes unused and can be removed.
> > >
> > > Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs")
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > ---
> > >  security/selinux/selinuxfs.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> > > index 01a7d50ed39b..340711e3dc9a 100644
> > > --- a/security/selinux/selinuxfs.c
> > > +++ b/security/selinux/selinuxfs.c
> > > @@ -651,14 +651,13 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf,
> > >         length = sel_make_policy_nodes(fsi, newpolicy);
> > >         if (length) {
> > >                 selinux_policy_cancel(fsi->state, newpolicy);
> > > -               goto out1;
> > > +               goto out;
> >
> > This looks good, especially with AUDIT_MAC_POLICY_LOAD recording
> > "res=1".  However, now that I'm looking at the error path here, we
> > don't display anything if sel_make_policy_nodes() fails, do we?  If
> > security_load_policy fails we at least do a printk(), but if this
> > fails it silently kills the policy load; at the very least I think we
> > want a `pr_warn_ratelimited("SELinux: failed to load policy due to
> > selinuxfs failures")` or something similar.
>
> There are error messages in some error paths in
> sel_make_policy_nodes(), but not all. Those are pr_err()s, while in
> sel_write_load() there is a pr_warn_ratelimited(). Could we just unify
> the sel_make_policy_nodes() failure to a single message? (I don't
> think the information on which part has failed is very useful as the
> most likely cause here is a memory allocation failure, not bad
> policy.) If so, should it be a pr_warn() or pr_err()? Ratelimited or
> not?

My personal opinion is that the kernel only needs to provide the error
details to userspace which can be useful in determining what wrong,
and how the user can fix it.  For example, if there is a memory
allocation failure in the kernel there is often little the user can do
(and it is often transient anyway due to loading and other factors),
so simply reporting that there was an allocation failure while
attempting X is sufficient.

Beyond that, I think things can get a little fuzzy, e.g. pr_warn() or
pr_err?  Ratelimit or always emit the message?  I also think the
answers can change as userspace behaviors change over time.  If one of
the policy load error paths uses a pr_err() then we should probably
stick with that; it also seems appropriate as failing to (re)load a
SELinux policy *is* a serious matter.  As far as the rate limiting is
concerned, I'm not sure if that is an important difference here; if
the system is getting enough requests to reload the policy, and
repeatedly failing, such that the ratelimiting matters there are
likely other, much larger, issues at play on the system.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v2 1/2] selinux: don't log MAC_POLICY_LOAD record on failed policy load

[Tyler Hicks]

On 2021-02-28 13:52:52, Paul Moore wrote:
> On Fri, Feb 26, 2021 at 9:46 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Thu, Feb 25, 2021 at 7:15 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Fri, Feb 12, 2021 at 1:59 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > >
> > > > If sel_make_policy_nodes() fails, we should jump to 'out', not 'out1',
> > > > as the latter would incorrectly log an MAC_POLICY_LOAD audit record,
> > > > even though the policy hasn't actually been reloaded. The 'out1' jump
> > > > label now becomes unused and can be removed.
> > > >
> > > > Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs")
> > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > > ---
> > > >  security/selinux/selinuxfs.c | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> > > > index 01a7d50ed39b..340711e3dc9a 100644
> > > > --- a/security/selinux/selinuxfs.c
> > > > +++ b/security/selinux/selinuxfs.c
> > > > @@ -651,14 +651,13 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf,
> > > >         length = sel_make_policy_nodes(fsi, newpolicy);
> > > >         if (length) {
> > > >                 selinux_policy_cancel(fsi->state, newpolicy);
> > > > -               goto out1;
> > > > +               goto out;
> > >
> > > This looks good, especially with AUDIT_MAC_POLICY_LOAD recording
> > > "res=1".  However, now that I'm looking at the error path here, we
> > > don't display anything if sel_make_policy_nodes() fails, do we?  If
> > > security_load_policy fails we at least do a printk(), but if this
> > > fails it silently kills the policy load; at the very least I think we
> > > want a `pr_warn_ratelimited("SELinux: failed to load policy due to
> > > selinuxfs failures")` or something similar.
> >
> > There are error messages in some error paths in
> > sel_make_policy_nodes(), but not all. Those are pr_err()s, while in
> > sel_write_load() there is a pr_warn_ratelimited(). Could we just unify
> > the sel_make_policy_nodes() failure to a single message? (I don't
> > think the information on which part has failed is very useful as the
> > most likely cause here is a memory allocation failure, not bad
> > policy.) If so, should it be a pr_warn() or pr_err()? Ratelimited or
> > not?
> 
> My personal opinion is that the kernel only needs to provide the error
> details to userspace which can be useful in determining what wrong,
> and how the user can fix it.  For example, if there is a memory
> allocation failure in the kernel there is often little the user can do
> (and it is often transient anyway due to loading and other factors),
> so simply reporting that there was an allocation failure while
> attempting X is sufficient.
> 
> Beyond that, I think things can get a little fuzzy, e.g. pr_warn() or
> pr_err?  Ratelimit or always emit the message?  I also think the
> answers can change as userspace behaviors change over time.  If one of
> the policy load error paths uses a pr_err() then we should probably
> stick with that; it also seems appropriate as failing to (re)load a
> SELinux policy *is* a serious matter.  As far as the rate limiting is
> concerned, I'm not sure if that is an important difference here; if
> the system is getting enough requests to reload the policy, and
> repeatedly failing, such that the ratelimiting matters there are
> likely other, much larger, issues at play on the system.

I was a little surprised to see pr_warn_ratelimited() (from both the
KERN_WARNING and ratelimited perspectives) used in the policy loading error
path so I poked around a bit. The description of commit 4262fb51c9f5 ("selinux:
log errors when loading new policy") explains the reasoning:

    If the policy fails to be loaded from userspace then a warning message is
    printed, whereas if a failure occurs after loading policy from userspace an
    error message will be printed with details on where policy loading failed
    (recreating one of /classes/, /policy_capabilities/, /booleans/ in the
    SELinux fs).

This seems like sound logic and would result in Ondrej using pr_err() in the
sel_make_policy_nodes() error path.

Tyler

> 
> -- 
> paul moore
> www.paul-moore.com

Re: [PATCH v2 2/2] selinux: fix variable scope issue in live sidtab conversion

[Tyler Hicks]

On 2021-02-12 19:59:30, Ondrej Mosnacek wrote:
> Commit 02a52c5c8c3b ("selinux: move policy commit after updating
> selinuxfs") moved the selinux_policy_commit() call out of
> security_load_policy() into sel_write_load(), which caused a subtle yet
> rather serious bug.
> 
> The problem is that security_load_policy() passes a reference to the
> convert_params local variable to sidtab_convert(), which stores it in
> the sidtab, where it may be accessed until the policy is swapped over
> and RCU synchronized. Before 02a52c5c8c3b, selinux_policy_commit() was
> called directly from security_load_policy(), so the convert_params
> pointer remained valid all the way until the old sidtab was destroyed,
> but now that's no longer the case and calls to sidtab_context_to_sid()
> on the old sidtab after security_load_policy() returns may cause invalid
> memory accesses.
> 
> This can be easily triggered using the stress test from commit
> ee1a84fdfeed ("selinux: overhaul sidtab to fix bug and improve
> performance"):
> ```
> function rand_cat() {
> 	echo $(( $RANDOM % 1024 ))
> }
> 
> function do_work() {
> 	while true; do
> 		echo -n "system_u:system_r:kernel_t:s0:c$(rand_cat),c$(rand_cat)" \
> 			>/sys/fs/selinux/context 2>/dev/null || true
> 	done
> }
> 
> do_work >/dev/null &
> do_work >/dev/null &
> do_work >/dev/null &
> 
> while load_policy; do echo -n .; sleep 0.1; done
> 
> kill %1
> kill %2
> kill %3
> ```
> 
> Fix this by allocating the temporary sidtab convert structures
> dynamically and passing them among the
> selinux_policy_{load,cancel,commit} functions.
> 
> Note that this commit also fixes the minor issue of logging a
> MAC_POLICY_LOAD audit record in case sel_make_policy_nodes() fails (in
> which case the new policy isn't actually loaded).
> 
> Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

Tested-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Reviewed-by: Tyler Hicks <tyhicks@linux.microsoft.com>

Feel free to leave those tags on your v3 submission after making the two
small changes requested by Paul.

Tyler

> ---
>  security/selinux/include/security.h | 15 ++++++---
>  security/selinux/selinuxfs.c        | 10 +++---
>  security/selinux/ss/services.c      | 51 +++++++++++++++++++----------
>  3 files changed, 49 insertions(+), 27 deletions(-)
> 
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index 765a258a899e..25db66e0ac51 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -219,14 +219,21 @@ static inline bool selinux_policycap_genfs_seclabel_symlinks(void)
>  	return READ_ONCE(state->policycap[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS]);
>  }
>  
> +struct selinux_policy_convert_data;
> +
> +struct selinux_load_state {
> +	struct selinux_policy *policy;
> +	struct selinux_policy_convert_data *convert_data;
> +};
> +
>  int security_mls_enabled(struct selinux_state *state);
>  int security_load_policy(struct selinux_state *state,
> -			void *data, size_t len,
> -			struct selinux_policy **newpolicyp);
> +			 void *data, size_t len,
> +			 struct selinux_load_state *load_state);
>  void selinux_policy_commit(struct selinux_state *state,
> -			struct selinux_policy *newpolicy);
> +			   struct selinux_load_state *load_state);
>  void selinux_policy_cancel(struct selinux_state *state,
> -			struct selinux_policy *policy);
> +			   struct selinux_load_state *load_state);
>  int security_read_policy(struct selinux_state *state,
>  			 void **data, size_t *len);
>  
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 340711e3dc9a..158d44ea93f4 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -616,7 +616,7 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf,
>  
>  {
>  	struct selinux_fs_info *fsi = file_inode(file)->i_sb->s_fs_info;
> -	struct selinux_policy *newpolicy;
> +	struct selinux_load_state load_state;
>  	ssize_t length;
>  	void *data = NULL;
>  
> @@ -642,19 +642,19 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf,
>  	if (copy_from_user(data, buf, count) != 0)
>  		goto out;
>  
> -	length = security_load_policy(fsi->state, data, count, &newpolicy);
> +	length = security_load_policy(fsi->state, data, count, &load_state);
>  	if (length) {
>  		pr_warn_ratelimited("SELinux: failed to load policy\n");
>  		goto out;
>  	}
>  
> -	length = sel_make_policy_nodes(fsi, newpolicy);
> +	length = sel_make_policy_nodes(fsi, load_state.policy);
>  	if (length) {
> -		selinux_policy_cancel(fsi->state, newpolicy);
> +		selinux_policy_cancel(fsi->state, &load_state);
>  		goto out;
>  	}
>  
> -	selinux_policy_commit(fsi->state, newpolicy);
> +	selinux_policy_commit(fsi->state, &load_state);
>  
>  	length = count;
>  
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 5e08ce2c5994..fada4ebc7ef8 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2157,8 +2157,13 @@ static void selinux_policy_cond_free(struct selinux_policy *policy)
>  	kfree(policy);
>  }
>  
> +struct selinux_policy_convert_data {
> +	struct convert_context_args args;
> +	struct sidtab_convert_params sidtab_params;
> +};
> +
>  void selinux_policy_cancel(struct selinux_state *state,
> -			struct selinux_policy *policy)
> +			   struct selinux_load_state *load_state)
>  {
>  	struct selinux_policy *oldpolicy;
>  
> @@ -2166,7 +2171,8 @@ void selinux_policy_cancel(struct selinux_state *state,
>  					lockdep_is_held(&state->policy_mutex));
>  
>  	sidtab_cancel_convert(oldpolicy->sidtab);
> -	selinux_policy_free(policy);
> +	selinux_policy_free(load_state->policy);
> +	kfree(load_state->convert_data);
>  }
>  
>  static void selinux_notify_policy_change(struct selinux_state *state,
> @@ -2181,9 +2187,9 @@ static void selinux_notify_policy_change(struct selinux_state *state,
>  }
>  
>  void selinux_policy_commit(struct selinux_state *state,
> -			struct selinux_policy *newpolicy)
> +			   struct selinux_load_state *load_state)
>  {
> -	struct selinux_policy *oldpolicy;
> +	struct selinux_policy *oldpolicy, *newpolicy = load_state->policy;
>  	u32 seqno;
>  
>  	oldpolicy = rcu_dereference_protected(state->policy,
> @@ -2223,6 +2229,7 @@ void selinux_policy_commit(struct selinux_state *state,
>  	/* Free the old policy */
>  	synchronize_rcu();
>  	selinux_policy_free(oldpolicy);
> +	kfree(load_state->convert_data);
>  
>  	/* Notify others of the policy change */
>  	selinux_notify_policy_change(state, seqno);
> @@ -2239,11 +2246,10 @@ void selinux_policy_commit(struct selinux_state *state,
>   * loading the new policy.
>   */
>  int security_load_policy(struct selinux_state *state, void *data, size_t len,
> -			struct selinux_policy **newpolicyp)
> +			 struct selinux_load_state *load_state)
>  {
>  	struct selinux_policy *newpolicy, *oldpolicy;
> -	struct sidtab_convert_params convert_params;
> -	struct convert_context_args args;
> +	struct selinux_policy_convert_data *convert_data;
>  	int rc = 0;
>  	struct policy_file file = { data, len }, *fp = &file;
>  
> @@ -2273,10 +2279,10 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len,
>  		goto err_mapping;
>  	}
>  
> -
>  	if (!selinux_initialized(state)) {
>  		/* First policy load, so no need to preserve state from old policy */
> -		*newpolicyp = newpolicy;
> +		load_state->policy = newpolicy;
> +		load_state->convert_data = NULL;
>  		return 0;
>  	}
>  
> @@ -2290,29 +2296,38 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len,
>  		goto err_free_isids;
>  	}
>  
> +	convert_data = kmalloc(sizeof(*convert_data), GFP_KERNEL);
> +	if (!convert_data) {
> +		rc = -ENOMEM;
> +		goto err_free_isids;
> +	}
> +
>  	/*
>  	 * Convert the internal representations of contexts
>  	 * in the new SID table.
>  	 */
> -	args.state = state;
> -	args.oldp = &oldpolicy->policydb;
> -	args.newp = &newpolicy->policydb;
> +	convert_data->args.state = state;
> +	convert_data->args.oldp = &oldpolicy->policydb;
> +	convert_data->args.newp = &newpolicy->policydb;
>  
> -	convert_params.func = convert_context;
> -	convert_params.args = &args;
> -	convert_params.target = newpolicy->sidtab;
> +	convert_data->sidtab_params.func = convert_context;
> +	convert_data->sidtab_params.args = &convert_data->args;
> +	convert_data->sidtab_params.target = newpolicy->sidtab;
>  
> -	rc = sidtab_convert(oldpolicy->sidtab, &convert_params);
> +	rc = sidtab_convert(oldpolicy->sidtab, &convert_data->sidtab_params);
>  	if (rc) {
>  		pr_err("SELinux:  unable to convert the internal"
>  			" representation of contexts in the new SID"
>  			" table\n");
> -		goto err_free_isids;
> +		goto err_free_convert_data;
>  	}
>  
> -	*newpolicyp = newpolicy;
> +	load_state->policy = newpolicy;
> +	load_state->convert_data = convert_data;
>  	return 0;
>  
> +err_free_convert_data:
> +	kfree(convert_data);
>  err_free_isids:
>  	sidtab_destroy(newpolicy->sidtab);
>  err_mapping:
> -- 
> 2.29.2
> 

Re: [PATCH v2 1/2] selinux: don't log MAC_POLICY_LOAD record on failed policy load

[Ondrej Mosnacek]

On Wed, Mar 3, 2021 at 3:56 AM Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
> On 2021-02-28 13:52:52, Paul Moore wrote:
> > On Fri, Feb 26, 2021 at 9:46 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > On Thu, Feb 25, 2021 at 7:15 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Fri, Feb 12, 2021 at 1:59 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > >
> > > > > If sel_make_policy_nodes() fails, we should jump to 'out', not 'out1',
> > > > > as the latter would incorrectly log an MAC_POLICY_LOAD audit record,
> > > > > even though the policy hasn't actually been reloaded. The 'out1' jump
> > > > > label now becomes unused and can be removed.
> > > > >
> > > > > Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs")
> > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > > > ---
> > > > >  security/selinux/selinuxfs.c | 3 +--
> > > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> > > > > index 01a7d50ed39b..340711e3dc9a 100644
> > > > > --- a/security/selinux/selinuxfs.c
> > > > > +++ b/security/selinux/selinuxfs.c
> > > > > @@ -651,14 +651,13 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf,
> > > > >         length = sel_make_policy_nodes(fsi, newpolicy);
> > > > >         if (length) {
> > > > >                 selinux_policy_cancel(fsi->state, newpolicy);
> > > > > -               goto out1;
> > > > > +               goto out;
> > > >
> > > > This looks good, especially with AUDIT_MAC_POLICY_LOAD recording
> > > > "res=1".  However, now that I'm looking at the error path here, we
> > > > don't display anything if sel_make_policy_nodes() fails, do we?  If
> > > > security_load_policy fails we at least do a printk(), but if this
> > > > fails it silently kills the policy load; at the very least I think we
> > > > want a `pr_warn_ratelimited("SELinux: failed to load policy due to
> > > > selinuxfs failures")` or something similar.
> > >
> > > There are error messages in some error paths in
> > > sel_make_policy_nodes(), but not all. Those are pr_err()s, while in
> > > sel_write_load() there is a pr_warn_ratelimited(). Could we just unify
> > > the sel_make_policy_nodes() failure to a single message? (I don't
> > > think the information on which part has failed is very useful as the
> > > most likely cause here is a memory allocation failure, not bad
> > > policy.) If so, should it be a pr_warn() or pr_err()? Ratelimited or
> > > not?
> >
> > My personal opinion is that the kernel only needs to provide the error
> > details to userspace which can be useful in determining what wrong,
> > and how the user can fix it.  For example, if there is a memory
> > allocation failure in the kernel there is often little the user can do
> > (and it is often transient anyway due to loading and other factors),
> > so simply reporting that there was an allocation failure while
> > attempting X is sufficient.
> >
> > Beyond that, I think things can get a little fuzzy, e.g. pr_warn() or
> > pr_err?  Ratelimit or always emit the message?  I also think the
> > answers can change as userspace behaviors change over time.  If one of
> > the policy load error paths uses a pr_err() then we should probably
> > stick with that; it also seems appropriate as failing to (re)load a
> > SELinux policy *is* a serious matter.  As far as the rate limiting is
> > concerned, I'm not sure if that is an important difference here; if
> > the system is getting enough requests to reload the policy, and
> > repeatedly failing, such that the ratelimiting matters there are
> > likely other, much larger, issues at play on the system.
>
> I was a little surprised to see pr_warn_ratelimited() (from both the
> KERN_WARNING and ratelimited perspectives) used in the policy loading error
> path so I poked around a bit. The description of commit 4262fb51c9f5 ("selinux:
> log errors when loading new policy") explains the reasoning:
>
>     If the policy fails to be loaded from userspace then a warning message is
>     printed, whereas if a failure occurs after loading policy from userspace an
>     error message will be printed with details on where policy loading failed
>     (recreating one of /classes/, /policy_capabilities/, /booleans/ in the
>     SELinux fs).
>
> This seems like sound logic and would result in Ondrej using pr_err() in the
> sel_make_policy_nodes() error path.

The situation has changed a bit since that was written, though... Back
then after the policy had been loaded there was no way to turn back
and if sel_make_policy_nodes() failed, the new policy would stay and
selinuxfs would have been left behind in an inconsistent/broken state.
Now this issue is fixed and the new policy isn't actually applied
until the selinuxfs preparation succeeds. So from a certain POV, the
selinuxfs failure is no longer that fatal and could just print a
warning like the other error path, because the result is the same
after both failures (active policy and selinuxfs state remains
unchanged).

Paul (or Stephen if you are reading this and have time to comment),
what do you think?

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH v2 2/2] selinux: fix variable scope issue in live sidtab conversion

[Ondrej Mosnacek]

On Wed, Mar 3, 2021 at 3:57 AM Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
> On 2021-02-12 19:59:30, Ondrej Mosnacek wrote:
> > Commit 02a52c5c8c3b ("selinux: move policy commit after updating
> > selinuxfs") moved the selinux_policy_commit() call out of
> > security_load_policy() into sel_write_load(), which caused a subtle yet
> > rather serious bug.
> >
> > The problem is that security_load_policy() passes a reference to the
> > convert_params local variable to sidtab_convert(), which stores it in
> > the sidtab, where it may be accessed until the policy is swapped over
> > and RCU synchronized. Before 02a52c5c8c3b, selinux_policy_commit() was
> > called directly from security_load_policy(), so the convert_params
> > pointer remained valid all the way until the old sidtab was destroyed,
> > but now that's no longer the case and calls to sidtab_context_to_sid()
> > on the old sidtab after security_load_policy() returns may cause invalid
> > memory accesses.
> >
> > This can be easily triggered using the stress test from commit
> > ee1a84fdfeed ("selinux: overhaul sidtab to fix bug and improve
> > performance"):
> > ```
> > function rand_cat() {
> >       echo $(( $RANDOM % 1024 ))
> > }
> >
> > function do_work() {
> >       while true; do
> >               echo -n "system_u:system_r:kernel_t:s0:c$(rand_cat),c$(rand_cat)" \
> >                       >/sys/fs/selinux/context 2>/dev/null || true
> >       done
> > }
> >
> > do_work >/dev/null &
> > do_work >/dev/null &
> > do_work >/dev/null &
> >
> > while load_policy; do echo -n .; sleep 0.1; done
> >
> > kill %1
> > kill %2
> > kill %3
> > ```
> >
> > Fix this by allocating the temporary sidtab convert structures
> > dynamically and passing them among the
> > selinux_policy_{load,cancel,commit} functions.
> >
> > Note that this commit also fixes the minor issue of logging a
> > MAC_POLICY_LOAD audit record in case sel_make_policy_nodes() fails (in
> > which case the new policy isn't actually loaded).
> >
> > Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> Tested-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> Reviewed-by: Tyler Hicks <tyhicks@linux.microsoft.com>
>
> Feel free to leave those tags on your v3 submission after making the two
> small changes requested by Paul.

Thanks!

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH v2 2/2] selinux: fix variable scope issue in live sidtab conversion

[Stephen Smalley]

On Wed, Mar 3, 2021 at 4:01 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Wed, Mar 3, 2021 at 3:57 AM Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
> > On 2021-02-12 19:59:30, Ondrej Mosnacek wrote:
> > > Commit 02a52c5c8c3b ("selinux: move policy commit after updating
> > > selinuxfs") moved the selinux_policy_commit() call out of
> > > security_load_policy() into sel_write_load(), which caused a subtle yet
> > > rather serious bug.
> > >
> > > The problem is that security_load_policy() passes a reference to the
> > > convert_params local variable to sidtab_convert(), which stores it in
> > > the sidtab, where it may be accessed until the policy is swapped over
> > > and RCU synchronized. Before 02a52c5c8c3b, selinux_policy_commit() was
> > > called directly from security_load_policy(), so the convert_params
> > > pointer remained valid all the way until the old sidtab was destroyed,
> > > but now that's no longer the case and calls to sidtab_context_to_sid()
> > > on the old sidtab after security_load_policy() returns may cause invalid
> > > memory accesses.
> > >
> > > This can be easily triggered using the stress test from commit
> > > ee1a84fdfeed ("selinux: overhaul sidtab to fix bug and improve
> > > performance"):
> > > ```
> > > function rand_cat() {
> > >       echo $(( $RANDOM % 1024 ))
> > > }
> > >
> > > function do_work() {
> > >       while true; do
> > >               echo -n "system_u:system_r:kernel_t:s0:c$(rand_cat),c$(rand_cat)" \
> > >                       >/sys/fs/selinux/context 2>/dev/null || true
> > >       done
> > > }
> > >
> > > do_work >/dev/null &
> > > do_work >/dev/null &
> > > do_work >/dev/null &
> > >
> > > while load_policy; do echo -n .; sleep 0.1; done
> > >
> > > kill %1
> > > kill %2
> > > kill %3
> > > ```
> > >
> > > Fix this by allocating the temporary sidtab convert structures
> > > dynamically and passing them among the
> > > selinux_policy_{load,cancel,commit} functions.
> > >
> > > Note that this commit also fixes the minor issue of logging a
> > > MAC_POLICY_LOAD audit record in case sel_make_policy_nodes() fails (in
> > > which case the new policy isn't actually loaded).
> > >
> > > Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs")
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> >
> > Tested-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > Reviewed-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> >
> > Feel free to leave those tags on your v3 submission after making the two
> > small changes requested by Paul.
>
> Thanks!

I haven't seen a final version of these patches yet.  Did I miss it?

Re: [PATCH v2 2/2] selinux: fix variable scope issue in live sidtab conversion

[Ondrej Mosnacek]

On Thu, Mar 18, 2021 at 12:22 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Wed, Mar 3, 2021 at 4:01 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > On Wed, Mar 3, 2021 at 3:57 AM Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
> > > On 2021-02-12 19:59:30, Ondrej Mosnacek wrote:
> > > > Commit 02a52c5c8c3b ("selinux: move policy commit after updating
> > > > selinuxfs") moved the selinux_policy_commit() call out of
> > > > security_load_policy() into sel_write_load(), which caused a subtle yet
> > > > rather serious bug.
> > > >
> > > > The problem is that security_load_policy() passes a reference to the
> > > > convert_params local variable to sidtab_convert(), which stores it in
> > > > the sidtab, where it may be accessed until the policy is swapped over
> > > > and RCU synchronized. Before 02a52c5c8c3b, selinux_policy_commit() was
> > > > called directly from security_load_policy(), so the convert_params
> > > > pointer remained valid all the way until the old sidtab was destroyed,
> > > > but now that's no longer the case and calls to sidtab_context_to_sid()
> > > > on the old sidtab after security_load_policy() returns may cause invalid
> > > > memory accesses.
> > > >
> > > > This can be easily triggered using the stress test from commit
> > > > ee1a84fdfeed ("selinux: overhaul sidtab to fix bug and improve
> > > > performance"):
> > > > ```
> > > > function rand_cat() {
> > > >       echo $(( $RANDOM % 1024 ))
> > > > }
> > > >
> > > > function do_work() {
> > > >       while true; do
> > > >               echo -n "system_u:system_r:kernel_t:s0:c$(rand_cat),c$(rand_cat)" \
> > > >                       >/sys/fs/selinux/context 2>/dev/null || true
> > > >       done
> > > > }
> > > >
> > > > do_work >/dev/null &
> > > > do_work >/dev/null &
> > > > do_work >/dev/null &
> > > >
> > > > while load_policy; do echo -n .; sleep 0.1; done
> > > >
> > > > kill %1
> > > > kill %2
> > > > kill %3
> > > > ```
> > > >
> > > > Fix this by allocating the temporary sidtab convert structures
> > > > dynamically and passing them among the
> > > > selinux_policy_{load,cancel,commit} functions.
> > > >
> > > > Note that this commit also fixes the minor issue of logging a
> > > > MAC_POLICY_LOAD audit record in case sel_make_policy_nodes() fails (in
> > > > which case the new policy isn't actually loaded).
> > > >
> > > > Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs")
> > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > >
> > > Tested-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > > Reviewed-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > >
> > > Feel free to leave those tags on your v3 submission after making the two
> > > small changes requested by Paul.
> >
> > Thanks!
>
> I haven't seen a final version of these patches yet.  Did I miss it?

No, I've been waiting for a reply regarding pr_warn() vs. pr_err(),
etc. on patch 1/2 (and then it slipped off my mind, so I didn't follow
up...)

--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH v2 1/2] selinux: don't log MAC_POLICY_LOAD record on failed policy load

[Paul Moore]

On Wed, Mar 3, 2021 at 3:54 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Wed, Mar 3, 2021 at 3:56 AM Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
> > On 2021-02-28 13:52:52, Paul Moore wrote:
> > > On Fri, Feb 26, 2021 at 9:46 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > On Thu, Feb 25, 2021 at 7:15 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > > On Fri, Feb 12, 2021 at 1:59 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > > >
> > > > > > If sel_make_policy_nodes() fails, we should jump to 'out', not 'out1',
> > > > > > as the latter would incorrectly log an MAC_POLICY_LOAD audit record,
> > > > > > even though the policy hasn't actually been reloaded. The 'out1' jump
> > > > > > label now becomes unused and can be removed.
> > > > > >
> > > > > > Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs")
> > > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > > > > ---
> > > > > >  security/selinux/selinuxfs.c | 3 +--
> > > > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> > > > > > index 01a7d50ed39b..340711e3dc9a 100644
> > > > > > --- a/security/selinux/selinuxfs.c
> > > > > > +++ b/security/selinux/selinuxfs.c
> > > > > > @@ -651,14 +651,13 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf,
> > > > > >         length = sel_make_policy_nodes(fsi, newpolicy);
> > > > > >         if (length) {
> > > > > >                 selinux_policy_cancel(fsi->state, newpolicy);
> > > > > > -               goto out1;
> > > > > > +               goto out;
> > > > >
> > > > > This looks good, especially with AUDIT_MAC_POLICY_LOAD recording
> > > > > "res=1".  However, now that I'm looking at the error path here, we
> > > > > don't display anything if sel_make_policy_nodes() fails, do we?  If
> > > > > security_load_policy fails we at least do a printk(), but if this
> > > > > fails it silently kills the policy load; at the very least I think we
> > > > > want a `pr_warn_ratelimited("SELinux: failed to load policy due to
> > > > > selinuxfs failures")` or something similar.
> > > >
> > > > There are error messages in some error paths in
> > > > sel_make_policy_nodes(), but not all. Those are pr_err()s, while in
> > > > sel_write_load() there is a pr_warn_ratelimited(). Could we just unify
> > > > the sel_make_policy_nodes() failure to a single message? (I don't
> > > > think the information on which part has failed is very useful as the
> > > > most likely cause here is a memory allocation failure, not bad
> > > > policy.) If so, should it be a pr_warn() or pr_err()? Ratelimited or
> > > > not?
> > >
> > > My personal opinion is that the kernel only needs to provide the error
> > > details to userspace which can be useful in determining what wrong,
> > > and how the user can fix it.  For example, if there is a memory
> > > allocation failure in the kernel there is often little the user can do
> > > (and it is often transient anyway due to loading and other factors),
> > > so simply reporting that there was an allocation failure while
> > > attempting X is sufficient.
> > >
> > > Beyond that, I think things can get a little fuzzy, e.g. pr_warn() or
> > > pr_err?  Ratelimit or always emit the message?  I also think the
> > > answers can change as userspace behaviors change over time.  If one of
> > > the policy load error paths uses a pr_err() then we should probably
> > > stick with that; it also seems appropriate as failing to (re)load a
> > > SELinux policy *is* a serious matter.  As far as the rate limiting is
> > > concerned, I'm not sure if that is an important difference here; if
> > > the system is getting enough requests to reload the policy, and
> > > repeatedly failing, such that the ratelimiting matters there are
> > > likely other, much larger, issues at play on the system.
> >
> > I was a little surprised to see pr_warn_ratelimited() (from both the
> > KERN_WARNING and ratelimited perspectives) used in the policy loading error
> > path so I poked around a bit. The description of commit 4262fb51c9f5 ("selinux:
> > log errors when loading new policy") explains the reasoning:
> >
> >     If the policy fails to be loaded from userspace then a warning message is
> >     printed, whereas if a failure occurs after loading policy from userspace an
> >     error message will be printed with details on where policy loading failed
> >     (recreating one of /classes/, /policy_capabilities/, /booleans/ in the
> >     SELinux fs).
> >
> > This seems like sound logic and would result in Ondrej using pr_err() in the
> > sel_make_policy_nodes() error path.
>
> The situation has changed a bit since that was written, though... Back
> then after the policy had been loaded there was no way to turn back
> and if sel_make_policy_nodes() failed, the new policy would stay and
> selinuxfs would have been left behind in an inconsistent/broken state.
> Now this issue is fixed and the new policy isn't actually applied
> until the selinuxfs preparation succeeds. So from a certain POV, the
> selinuxfs failure is no longer that fatal and could just print a
> warning like the other error path, because the result is the same
> after both failures (active policy and selinuxfs state remains
> unchanged).
>
> Paul (or Stephen if you are reading this and have time to comment),
> what do you think?

Sorry for the late reply, I lost this in my inbox and since I already
marked the patchset as "changes requested" in patchwork it fell off my
radar ...

Anyway, back to your question ... it does seem like pr_warn() is the
right answer here for the reasons that Ondrej mentioned above, and I
personally feel it is in keeping with the original patch's intention
as well; "If the policy fails to be loaded from userspace then a
warning message is printed ..."  However, I'm not going to lose a lot
of sleep over differences between pr_warn() and pr_err() here, if
someone feels strongly that it should be pr_err() and can back that up
with some solid reasoning and/or precedence then so be it.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v2 2/2] selinux: fix variable scope issue in live sidtab conversion

[Paul Moore]

On Thu, Mar 18, 2021 at 7:45 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> No, I've been waiting for a reply regarding pr_warn() vs. pr_err(),
> etc. on patch 1/2 (and then it slipped off my mind, so I didn't follow
> up...)

Sorry about that, it looks like it was a casualty of my inbox.  I just
replied back on that thread.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v2 1/2] selinux: don't log MAC_POLICY_LOAD record on failed policy load

[Stephen Smalley]

On Thu, Mar 18, 2021 at 10:48 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, Mar 3, 2021 at 3:54 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > The situation has changed a bit since that was written, though... Back
> > then after the policy had been loaded there was no way to turn back
> > and if sel_make_policy_nodes() failed, the new policy would stay and
> > selinuxfs would have been left behind in an inconsistent/broken state.
> > Now this issue is fixed and the new policy isn't actually applied
> > until the selinuxfs preparation succeeds. So from a certain POV, the
> > selinuxfs failure is no longer that fatal and could just print a
> > warning like the other error path, because the result is the same
> > after both failures (active policy and selinuxfs state remains
> > unchanged).
> >
> > Paul (or Stephen if you are reading this and have time to comment),
> > what do you think?
>
> Sorry for the late reply, I lost this in my inbox and since I already
> marked the patchset as "changes requested" in patchwork it fell off my
> radar ...
>
> Anyway, back to your question ... it does seem like pr_warn() is the
> right answer here for the reasons that Ondrej mentioned above, and I
> personally feel it is in keeping with the original patch's intention
> as well; "If the policy fails to be loaded from userspace then a
> warning message is printed ..."  However, I'm not going to lose a lot
> of sleep over differences between pr_warn() and pr_err() here, if
> someone feels strongly that it should be pr_err() and can back that up
> with some solid reasoning and/or precedence then so be it.

That's fine with me.  FWIW, I think the rationale for using
pr_warn_ratelimited() for error returns from security_load_policy()
was that the failure could be entirely userspace-induced, i.e. just
pass the kernel an invalid policy.
The pr_err() messages on sel_make_bools/classes were in contrast
entirely kernel-internal errors and could leave the system in an
inconsistent state as you noted.