On Wed, Aug 5, 2020 at 2:19 PM Stephen Smalley <stephen.smalley.work@xxxxxxxxx> wrote: > On 8/5/20 4:29 AM, Ondrej Mosnacek wrote: > > > 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. > Is that done anywhere else in the kernel currently? If so, then I have > no problem with doing so. Otherwise, I'd rather just drop the > read_lock() here and add a comment explaining why it is currently safe > without it. I guess the closest thing is the "c" argument of rcu_dereference_protected(), which should be passed a lock[dep]_is_held() check on the lock/mutex that provides the mutual exclusion that is needed for the operation to be safe. If we ever do conver the rwlock to RCU, we will need something to pass to rcu_dereference_protected() anyway. (I mean we could cheat and just pass 1 + a comment, but it would be nicer to put an actual lock[dep]_held() check there.) But it just occurred to me that we could make it even more explicit by simply adding another "policy_load" mutex to security_ss and do most of security_load_policy() with that mutex locked. Since there will never be any contention on it, the extra overhead should be negligible. -- Ondrej Mosnacek Software Engineer, Platform Security - SELinux kernel Red Hat, Inc.
