On Wed, Mar 3, 2021 at 4:01 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > > On Wed, Mar 3, 2021 at 3:57 AM Tyler Hicks <tyhicks@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx> > > > > Tested-by: Tyler Hicks <tyhicks@xxxxxxxxxxxxxxxxxxx> > > Reviewed-by: Tyler Hicks <tyhicks@xxxxxxxxxxxxxxxxxxx> > > > > 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?