On Wed, Feb 10, 2021 at 9:33 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> 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...
Aiming for a simple solution first is generally a good first approach;
things almost always get more complex as they progress :)
> > 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).
Fair point.
> > 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.
I'm not that worried about the allocation, in security_load_policy()
we're already allocating both a new policy and a new sidtab with
GFP_KERNEL so we are already looking at delays when under memory
pressure (which is okay in this case). I view this as more desirable
than the approach taken in your first patch.
If we wanted to try and reduce our calls to kzalloc() we could always
allocate the new policy, sidtab, and convert_context_args structs in
one block via one kzalloc() call. The drawback would be that we would
be allocating a bit more memory than we would need for normal usage
(the convert struct space would generally be wasted), and I guess
there might be an increased chance of the allocation taking the slow
path since the chunk would be larger. Likely not worth pursuing
unless we see real memory problems in the future.
--
paul moore
www.paul-moore.com
