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

[Stephen Smalley <stephen.smalley.work@gmail.com>]

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?