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@xxxxxxxxxx>
Tested-by: Tyler Hicks <tyhicks@xxxxxxxxxxxxxxxxxxx>
Reviewed-by: Tyler Hicks <tyhicks@xxxxxxxxxxxxxxxxxxx>
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
>
