On Fri, Feb 12, 2021 at 1:59 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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.
--
paul moore
www.paul-moore.com
