selinux: fix variable scope issue in live sidtab conversion

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>

diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 3cc8bab..63ca6e7 100644 (file)
--- 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 b6829c4..2b745ae 100644 (file)
--- 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 9704c8a..495fc86 100644 (file)
--- 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_invalid_context(
        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(struct selinux_policy *policy)
 }
 
 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 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,
@@ -2183,9 +2189,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,
@@ -2225,6 +2231,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);
@@ -2241,11 +2248,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;
 
@@ -2275,10 +2281,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;
        }
 
@@ -2292,29 +2298,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: