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.
Otherwise the patch looks good to me after a brief look.
