| Message ID | 20210208112736.247195-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: fix variable scope issue in live sidtab conversion | expand |
On Mon, Feb 8, 2021 at 6:27 AM 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 > ``` > > There are several ways to fix this: > 1. Move the sidtab convert parameters to struct selinux_policy. > Pros: > * simple change > Cons: > * added fields not used during most of the object's lifetime > 2. Move the sidtab convert params to sel_write_load(). > Pros: > * (nothing specific) > Cons: > * layering violation, a lot of types would have to be exposed to > selinuxfs.c > 3. Merge policy load functions back into one and call > sel_make_policy_nodes() as a callback. > Pros: > * results in simpler code > Cons: > * introduces an indirect call (not in hot path, so should be okay) > > I chose to implement option (3.), because IMHO it results in the least > ugly code and has the least bad drawback. > > 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 | 10 +- > security/selinux/selinuxfs.c | 18 +--- > security/selinux/ss/services.c | 159 ++++++++++++---------------- > 3 files changed, 78 insertions(+), 109 deletions(-) My concern is that this is something that should be backported to -stable and I wonder if there is an easier way. Since the core issue appears to be the scope/lifetime of the stdtab->convert field, and since the ->convert field is a struct with only three pointers, why not either embed a copy of the sidtab_convert_params struct in the sidtab struct (net increase in two pointers), or do a memdup() (or similar) into the sidtab->convert in sidtab_convert(). There would need to be some minor additional work in the latter case, but I imagine adding a kfree() to sidtab_cancel_convert() and calling sidtab_cancel_convert() in selinux_policy_commit() should be the bulk of the changes. Am I missing something, is there a good reason why it isn't that easy?
On Tue, Feb 9, 2021 at 3:20 AM Paul Moore <paul@paul-moore.com> wrote: > On Mon, Feb 8, 2021 at 6:27 AM 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 > > ``` > > > > There are several ways to fix this: > > 1. Move the sidtab convert parameters to struct selinux_policy. > > Pros: > > * simple change > > Cons: > > * added fields not used during most of the object's lifetime > > 2. Move the sidtab convert params to sel_write_load(). > > Pros: > > * (nothing specific) > > Cons: > > * layering violation, a lot of types would have to be exposed to > > selinuxfs.c > > 3. Merge policy load functions back into one and call > > sel_make_policy_nodes() as a callback. > > Pros: > > * results in simpler code > > Cons: > > * introduces an indirect call (not in hot path, so should be okay) > > > > I chose to implement option (3.), because IMHO it results in the least > > ugly code and has the least bad drawback. > > > > 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 | 10 +- > > security/selinux/selinuxfs.c | 18 +--- > > security/selinux/ss/services.c | 159 ++++++++++++---------------- > > 3 files changed, 78 insertions(+), 109 deletions(-) > > My concern is that this is something that should be backported to > -stable and I wonder if there is an easier way. This would need to go only into 5.10 (and 5.11 depending on the timing), so I think it should still apply cleanly. But there is additional value in having small patches for stable (less likelihood of mistake), so I'll try to revisit it... > Since the core issue > appears to be the scope/lifetime of the stdtab->convert field, and > since the ->convert field is a struct with only three pointers, why > not either embed a copy of the sidtab_convert_params struct in the > sidtab struct (net increase in two pointers), This has a hidden catch - also the convert_context_args would need to be embedded and that has pointers to policydb and selinux_state, which sidtab currently doesn't "know about" (i.e. it would slightly break the abstraction). > or do a memdup() (or > similar) into the sidtab->convert in sidtab_convert(). There would > need to be some minor additional work in the latter case, but I > imagine adding a kfree() to sidtab_cancel_convert() and calling > sidtab_cancel_convert() in selinux_policy_commit() should be the bulk > of the changes. This should be possible and relatively easy. I forgot to list it in the options - my only problem with that was the unnecessary dynamic allocation, but i concur that keeping the patch small is more important in this case. I'll try to do it this way in v2.
On Wed, Feb 10, 2021 at 9:33 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Tue, Feb 9, 2021 at 3:20 AM Paul Moore <paul@paul-moore.com> wrote: > > On Mon, Feb 8, 2021 at 6:27 AM 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 > > > ``` > > > > > > There are several ways to fix this: > > > 1. Move the sidtab convert parameters to struct selinux_policy. > > > Pros: > > > * simple change > > > Cons: > > > * added fields not used during most of the object's lifetime > > > 2. Move the sidtab convert params to sel_write_load(). > > > Pros: > > > * (nothing specific) > > > Cons: > > > * layering violation, a lot of types would have to be exposed to > > > selinuxfs.c > > > 3. Merge policy load functions back into one and call > > > sel_make_policy_nodes() as a callback. > > > Pros: > > > * results in simpler code > > > Cons: > > > * introduces an indirect call (not in hot path, so should be okay) > > > > > > I chose to implement option (3.), because IMHO it results in the least > > > ugly code and has the least bad drawback. > > > > > > 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 | 10 +- > > > security/selinux/selinuxfs.c | 18 +--- > > > security/selinux/ss/services.c | 159 ++++++++++++---------------- > > > 3 files changed, 78 insertions(+), 109 deletions(-) > > > > My concern is that this is something that should be backported to > > -stable and I wonder if there is an easier way. > > This would need to go only into 5.10 (and 5.11 depending on the > timing), so I think it should still apply cleanly. But there is > additional value in having small patches for stable (less likelihood > of mistake), so I'll try to revisit it... Aiming for a simple solution first is generally a good first approach; things almost always get more complex as they progress :) > > Since the core issue > > appears to be the scope/lifetime of the stdtab->convert field, and > > since the ->convert field is a struct with only three pointers, why > > not either embed a copy of the sidtab_convert_params struct in the > > sidtab struct (net increase in two pointers), > > This has a hidden catch - also the convert_context_args would need to > be embedded and that has pointers to policydb and selinux_state, which > sidtab currently doesn't "know about" (i.e. it would slightly break > the abstraction). Fair point. > > or do a memdup() (or > > similar) into the sidtab->convert in sidtab_convert(). There would > > need to be some minor additional work in the latter case, but I > > imagine adding a kfree() to sidtab_cancel_convert() and calling > > sidtab_cancel_convert() in selinux_policy_commit() should be the bulk > > of the changes. > > This should be possible and relatively easy. I forgot to list it in > the options - my only problem with that was the unnecessary dynamic > allocation, but i concur that keeping the patch small is more > important in this case. I'll try to do it this way in v2. I'm not that worried about the allocation, in security_load_policy() we're already allocating both a new policy and a new sidtab with GFP_KERNEL so we are already looking at delays when under memory pressure (which is okay in this case). I view this as more desirable than the approach taken in your first patch. If we wanted to try and reduce our calls to kzalloc() we could always allocate the new policy, sidtab, and convert_context_args structs in one block via one kzalloc() call. The drawback would be that we would be allocating a bit more memory than we would need for normal usage (the convert struct space would generally be wasted), and I guess there might be an increased chance of the allocation taking the slow path since the chunk would be larger. Likely not worth pursuing unless we see real memory problems in the future.
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index 765a258a899e..9b1bcecad6ef 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -221,12 +221,10 @@ static inline bool selinux_policycap_genfs_seclabel_symlinks(void) 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 selinux_policy_commit(struct selinux_state *state, - struct selinux_policy *newpolicy); -void selinux_policy_cancel(struct selinux_state *state, - struct selinux_policy *policy); + void *data, size_t len, + int (*setup_func)(struct selinux_policy *newpolicy, + void *args), + void *args); 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 01a7d50ed39b..1a9ef1a010e6 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -533,9 +533,9 @@ static void sel_remove_old_bool_data(unsigned int bool_num, char **bool_names, kfree(bool_values); } -static int sel_make_policy_nodes(struct selinux_fs_info *fsi, - struct selinux_policy *newpolicy) +static int sel_make_policy_nodes(struct selinux_policy *newpolicy, void *args) { + struct selinux_fs_info *fsi = args; int ret = 0; struct dentry *tmp_parent, *tmp_bool_dir, *tmp_class_dir, *old_dentry; unsigned int tmp_bool_num, old_bool_num; @@ -616,7 +616,6 @@ 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; ssize_t length; void *data = NULL; @@ -642,27 +641,20 @@ 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, + sel_make_policy_nodes, fsi); if (length) { pr_warn_ratelimited("SELinux: failed to load policy\n"); goto out; } - length = sel_make_policy_nodes(fsi, newpolicy); - if (length) { - selinux_policy_cancel(fsi->state, newpolicy); - goto out1; - } - - 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)), audit_get_sessionid(current)); + out: mutex_unlock(&fsi->state->policy_mutex); vfree(data); diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 5e08ce2c5994..50124123d385 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -2157,18 +2157,6 @@ static void selinux_policy_cond_free(struct selinux_policy *policy) kfree(policy); } -void selinux_policy_cancel(struct selinux_state *state, - struct selinux_policy *policy) -{ - struct selinux_policy *oldpolicy; - - oldpolicy = rcu_dereference_protected(state->policy, - lockdep_is_held(&state->policy_mutex)); - - sidtab_cancel_convert(oldpolicy->sidtab); - selinux_policy_free(policy); -} - static void selinux_notify_policy_change(struct selinux_state *state, u32 seqno) { @@ -2180,54 +2168,6 @@ static void selinux_notify_policy_change(struct selinux_state *state, selinux_xfrm_notify_policyload(); } -void selinux_policy_commit(struct selinux_state *state, - struct selinux_policy *newpolicy) -{ - struct selinux_policy *oldpolicy; - u32 seqno; - - oldpolicy = rcu_dereference_protected(state->policy, - lockdep_is_held(&state->policy_mutex)); - - /* If switching between different policy types, log MLS status */ - if (oldpolicy) { - if (oldpolicy->policydb.mls_enabled && !newpolicy->policydb.mls_enabled) - pr_info("SELinux: Disabling MLS support...\n"); - else if (!oldpolicy->policydb.mls_enabled && newpolicy->policydb.mls_enabled) - pr_info("SELinux: Enabling MLS support...\n"); - } - - /* Set latest granting seqno for new policy. */ - if (oldpolicy) - newpolicy->latest_granting = oldpolicy->latest_granting + 1; - else - newpolicy->latest_granting = 1; - seqno = newpolicy->latest_granting; - - /* Install the new policy. */ - rcu_assign_pointer(state->policy, newpolicy); - - /* Load the policycaps from the new policy */ - security_load_policycaps(state, newpolicy); - - if (!selinux_initialized(state)) { - /* - * After first policy load, the security server is - * marked as initialized and ready to handle requests and - * any objects created prior to policy load are then labeled. - */ - selinux_mark_initialized(state); - selinux_complete_init(); - } - - /* Free the old policy */ - synchronize_rcu(); - selinux_policy_free(oldpolicy); - - /* Notify others of the policy change */ - selinux_notify_policy_change(state, seqno); -} - /** * security_load_policy - Load a security policy configuration. * @data: binary policy data @@ -2239,12 +2179,15 @@ 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) + int (*setup_func)(struct selinux_policy *newpolicy, + void *setup_args), + void *setup_args) { - struct selinux_policy *newpolicy, *oldpolicy; + struct selinux_policy *newpolicy, *oldpolicy = NULL; struct sidtab_convert_params convert_params; - struct convert_context_args args; + struct convert_context_args convert_args; int rc = 0; + u32 seqno; struct policy_file file = { data, len }, *fp = &file; newpolicy = kzalloc(sizeof(*newpolicy), GFP_KERNEL); @@ -2273,44 +2216,80 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len, goto err_mapping; } + if (selinux_initialized(state)) { + oldpolicy = rcu_dereference_protected(state->policy, + lockdep_is_held(&state->policy_mutex)); - if (!selinux_initialized(state)) { - /* First policy load, so no need to preserve state from old policy */ - *newpolicyp = newpolicy; - return 0; - } + /* Preserve active boolean values from the old policy */ + rc = security_preserve_bools(oldpolicy, newpolicy); + if (rc) { + pr_err("SELinux: unable to preserve booleans\n"); + goto err_free_isids; + } - oldpolicy = rcu_dereference_protected(state->policy, - lockdep_is_held(&state->policy_mutex)); + /* + * Convert the internal representations of contexts + * in the new SID table. + */ + convert_args.state = state; + convert_args.oldp = &oldpolicy->policydb; + convert_args.newp = &newpolicy->policydb; + + convert_params.func = convert_context; + convert_params.args = &convert_args; + convert_params.target = newpolicy->sidtab; + + rc = sidtab_convert(oldpolicy->sidtab, &convert_params); + if (rc) { + pr_err("SELinux: unable to convert the internal representation of contexts in the new SID table\n"); + goto err_free_isids; + } + } - /* Preserve active boolean values from the old policy */ - rc = security_preserve_bools(oldpolicy, newpolicy); + rc = setup_func(newpolicy, setup_args); if (rc) { - pr_err("SELinux: unable to preserve booleans\n"); + if (oldpolicy) + sidtab_cancel_convert(oldpolicy->sidtab); 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; + /* If switching between different policy types, log MLS status */ + if (oldpolicy) { + if (oldpolicy->policydb.mls_enabled && !newpolicy->policydb.mls_enabled) + pr_info("SELinux: Disabling MLS support...\n"); + else if (!oldpolicy->policydb.mls_enabled && newpolicy->policydb.mls_enabled) + pr_info("SELinux: Enabling MLS support...\n"); + } + + /* Set latest granting seqno for new policy. */ + if (oldpolicy) + newpolicy->latest_granting = oldpolicy->latest_granting + 1; + else + newpolicy->latest_granting = 1; + seqno = newpolicy->latest_granting; - convert_params.func = convert_context; - convert_params.args = &args; - convert_params.target = newpolicy->sidtab; + /* Install the new policy. */ + rcu_assign_pointer(state->policy, newpolicy); - rc = sidtab_convert(oldpolicy->sidtab, &convert_params); - if (rc) { - pr_err("SELinux: unable to convert the internal" - " representation of contexts in the new SID" - " table\n"); - goto err_free_isids; + /* Load the policycaps from the new policy */ + security_load_policycaps(state, newpolicy); + + if (!oldpolicy) { + /* + * After first policy load, the security server is + * marked as initialized and ready to handle requests and + * any objects created prior to policy load are then labeled. + */ + selinux_mark_initialized(state); + selinux_complete_init(); } - *newpolicyp = newpolicy; + /* Free the old policy */ + synchronize_rcu(); + selinux_policy_free(oldpolicy); + + /* Notify others of the policy change */ + selinux_notify_policy_change(state, seqno); return 0; err_free_isids:
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 ``` There are several ways to fix this: 1. Move the sidtab convert parameters to struct selinux_policy. Pros: * simple change Cons: * added fields not used during most of the object's lifetime 2. Move the sidtab convert params to sel_write_load(). Pros: * (nothing specific) Cons: * layering violation, a lot of types would have to be exposed to selinuxfs.c 3. Merge policy load functions back into one and call sel_make_policy_nodes() as a callback. Pros: * results in simpler code Cons: * introduces an indirect call (not in hot path, so should be okay) I chose to implement option (3.), because IMHO it results in the least ugly code and has the least bad drawback. 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 | 10 +- security/selinux/selinuxfs.c | 18 +--- security/selinux/ss/services.c | 159 ++++++++++++---------------- 3 files changed, 78 insertions(+), 109 deletions(-)