On Tue, Feb 9, 2021 at 3:20 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> On Mon, Feb 8, 2021 at 6:27 AM 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
> > ```
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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.
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
