diff mbox series

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

Message ID 20210212185930.130477-3-omosnace@redhat.com (mailing list archive)
State Changes Requested
Delegated to: Paul Moore
Headers show
Series selinux: policy load fixes | expand

Commit Message

Ondrej Mosnacek Feb. 12, 2021, 6:59 p.m. UTC
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(-)

Comments

Paul Moore Feb. 25, 2021, 7:20 p.m. UTC | #1
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.
Ondrej Mosnacek Feb. 26, 2021, 2:47 p.m. UTC | #2
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.
Tyler Hicks March 3, 2021, 2:57 a.m. UTC | #3
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
>
Ondrej Mosnacek March 3, 2021, 9:01 a.m. UTC | #4
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!
Stephen Smalley March 18, 2021, 11:22 a.m. UTC | #5
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?
Ondrej Mosnacek March 18, 2021, 11:45 a.m. UTC | #6
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.
Paul Moore March 18, 2021, 2:49 p.m. UTC | #7
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.
diff mbox series

Patch

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: