| 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 |
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.
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.
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 >
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!
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?
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.
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 --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:
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(-)