On 7/30/20 6:09 PM, Stephen Smalley 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>
---
@@ -2098,10 +2104,12 @@ static int convert_context(struct context *oldc, struct context *newc, void *p)
static void security_load_policycaps(struct selinux_state *state)
{
- struct policydb *p = &state->ss->policydb;
+ struct policydb *p = &state->ss->policy->policydb;
unsigned int i;
struct ebitmap_node *node;
+ read_lock(&state->ss->policy_rwlock);
+
Oops, should have moved the dereferencing of policy after taking the
read-lock here; fixed it everywhere else I think but missed this one.
Will fix in the next version but will wait for other comments on this
version.
@@ -2132,112 +2200,58 @@ static int security_preserve_bools(struct selinux_state *state,
*/
int security_load_policy(struct selinux_state *state, void *data, size_t len)
{
<snip>
/*
* Convert the internal representations of contexts
* in the new SID table.
*/
args.state = state;
- args.oldp = policydb;
- args.newp = newpolicydb;
+ args.oldp = &state->ss->policy->policydb;
+ args.newp = &newpolicy->policydb;
convert_params.func = convert_context;
convert_params.args = &args;
- convert_params.target = newsidtab;
+ convert_params.target = &newpolicy->sidtab;
- rc = sidtab_convert(oldsidtab, &convert_params);
+ rc = sidtab_convert(&state->ss->policy->sidtab, &convert_params);
Should sidtab_convert() be called while holding policy read-lock since
we are passing state->ss->policy->policydb via the args field of
convert_params and using it within the callback? I think it happens to
be safe currently by virtue of the fact that nothing else can write to
it since selinuxfs is holding its semaphore during the entire policy
reload but it seems inconsistent. However, if we do take policy
read-lock across the call, then sidtab_convert() needs to use GFP_ATOMIC
allocations instead of GFP_KERNEL.
