Re: [PATCH v2 2/2] selinux: fix variable scope issue in live sidtab conversion

[Ondrej Mosnacek <omosnace@redhat.com>]

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.

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.