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. 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), 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.
Am I missing something, is there a good reason why it isn't that easy?
--
paul moore
www.paul-moore.com
