On 8/5/20 9:27 AM, Ondrej Mosnacek wrote:
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.
I like that better. In fact, we used to have that prior to:
commit 89abd0acf0335f3f760a3c0698d43bb1eaa83e44
Author: Eric Paris <eparis@xxxxxxxxxx>
Date: Mon Jun 9 15:58:04 2008 -0400
SELinux: drop load_mutex in security_load_policy
We used to protect against races of policy load in security_load_policy
by using the load_mutex. Since then we have added a new mutex,
sel_mutex, in sel_write_load() which is always held across all calls to
security_load_policy we are covered and can safely just drop this one.
