On Mon, Aug 3, 2020 at 7:40 PM Stephen Smalley <stephen.smalley.work@xxxxxxxxx> wrote: > Encapsulate the policy state in its own structure (struct > selinux_policy) that is separately allocated but referenced from the > selinux_ss structure. The policy state includes the SID table > (particularly the context structures), the policy database, and the > mapping between the kernel classes/permissions and the policy values. > Refactor the security server portion of the policy load logic to > cleanly separate loading of the new structures from committing the new > policy. Unify the initial policy load and reload code paths as much > as possible, avoiding duplicated code. Make sure we are taking the > policy read-lock prior to any dereferencing of the policy. Move the > copying of the policy capability booleans into the state structure > outside of the policy write-lock because they are separate from the > policy and are read outside of any policy lock; possibly they should > be using at least READ_ONCE/WRITE_ONCE or smp_load_acquire/store_release. > > These changes simplify the policy loading logic, reduce the size of > the critical section while holding the policy write-lock, and should > facilitate future changes to e.g. refactor the entire policy reload > logic including the selinuxfs code to make the updating of the policy > and the selinuxfs directory tree atomic and/or to convert the policy > read-write lock to RCU. > > Signed-off-by: Stephen Smalley <stephen.smalley.work@xxxxxxxxx> > --- > v3 fixes a couple of instances where we should take the read-lock for > consistency prior to dereferencing state->ss->policy, and updates > sidtab_convert() and its helpers/callbacks to use GFP_ATOMIC > allocations instead of GFP_KERNEL and to remove a call to > cond_resched() since it is now called with the read-lock held and > therefore cannot call sleeping functions. Technically we know that > state->ss->policy cannot be modified since selinuxfs is taking > fsi->mutex for all policy-modifying operations (both > security_load_policy and security_set_bools) but this provides > consistency in the handling of the policy rwlock. I must say I'm a little concerned about switching to GFP_ATOMIC here. The sidtab table can become quite large on long-running systems (since it's grow-only currently) and in such case the kernel could have a hard time allocating everything atomically. Maybe we could instead pass the fsi->mutex's lockdep_map to security_load_policy() and call lockdep_is_held() inside as a sanity check? This would document that the function is expected to be called in mutual exclusion and also detect at runtime if someone accidentally moves the call outside of the mutex's critical section. It's not perfect, since a careless programmer can still abuse it, but at least the lockdep_map argument would warn anyone calling that function (or reviewing a related patch) that there is some locking constraint that needs to be respected. Otherwise the patch looks good to me after a brief look. -- Ondrej Mosnacek Software Engineer, Platform Security - SELinux kernel Red Hat, Inc.
