From: Ondrej Mosnacek omosnace@redhat.com
commit 6406887a12ee5dcdaffff1a8508d91113d545559 upstream.
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.
Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs") Cc: stable@vger.kernel.org Tested-by: Tyler Hicks tyhicks@linux.microsoft.com Reviewed-by: Tyler Hicks tyhicks@linux.microsoft.com Signed-off-by: Ondrej Mosnacek omosnace@redhat.com [PM: merge fuzz in security.h and services.c] Signed-off-by: Paul Moore paul@paul-moore.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- security/selinux/include/security.h | 15 ++++++-- security/selinux/selinuxfs.c | 10 ++--- security/selinux/ss/services.c | 65 ++++++++++++++++++++++-------------- 3 files changed, 56 insertions(+), 34 deletions(-)
--- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -219,14 +219,21 @@ static inline bool selinux_policycap_gen 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);
--- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -616,7 +616,7 @@ static ssize_t sel_write_load(struct fil
{ 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 fil 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;
--- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -66,6 +66,17 @@ #include "audit.h" #include "policycap_names.h"
+struct convert_context_args { + struct selinux_state *state; + struct policydb *oldp; + struct policydb *newp; +}; + +struct selinux_policy_convert_data { + struct convert_context_args args; + struct sidtab_convert_params sidtab_params; +}; + /* Forward declaration. */ static int context_struct_to_string(struct policydb *policydb, struct context *context, @@ -1975,12 +1986,6 @@ static inline int convert_context_handle return 0; }
-struct convert_context_args { - struct selinux_state *state; - struct policydb *oldp; - struct policydb *newp; -}; - /* * Convert the values in the security context * structure `oldc' from the values specified @@ -2160,7 +2165,7 @@ static void selinux_policy_cond_free(str }
void selinux_policy_cancel(struct selinux_state *state, - struct selinux_policy *policy) + struct selinux_load_state *load_state) { struct selinux_policy *oldpolicy;
@@ -2168,7 +2173,8 @@ void selinux_policy_cancel(struct selinu 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, @@ -2183,9 +2189,9 @@ static void selinux_notify_policy_change }
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, @@ -2225,6 +2231,7 @@ void selinux_policy_commit(struct selinu /* 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); @@ -2241,11 +2248,10 @@ void selinux_policy_commit(struct selinu * 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;
@@ -2275,10 +2281,10 @@ int security_load_policy(struct selinux_ 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; }
@@ -2292,29 +2298,38 @@ int security_load_policy(struct selinux_ 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_params.func = convert_context; - convert_params.args = &args; - convert_params.target = newpolicy->sidtab; + convert_data->args.state = state; + convert_data->args.oldp = &oldpolicy->policydb; + convert_data->args.newp = &newpolicy->policydb; + + 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: