| Message ID | 20200804135352.5650-1-stephen.smalley.work@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] selinux: move policy commit after updating selinuxfs | expand |
On 8/4/20 9:53 AM, Stephen Smalley wrote: > With the refactoring of the policy load logic in the security > server from the previous change, it is now possible to split out > the committing of the new policy from security_load_policy() and > perform it only after successful updating of selinuxfs. Change > security_load_policy() to return the newly populated policy > data structures to the caller, export selinux_policy_commit() > for external callers, and introduce selinux_policy_cancel() to > provide a way to cancel the policy load in the event of an error > during updating of the selinuxfs directory tree. Further, rework > the interfaces used by selinuxfs to get information from the policy > when creating the new directory tree to take and act upon the > new policy data structure rather than the current/active policy. > Update selinuxfs to use these updated and new interfaces. While > we are here, stop re-creating the policy_capabilities directory > on each policy load since it does not depend on the policy, and > stop trying to create the booleans and classes directories during > the initial creation of selinuxfs since no information is available > until first policy load. > > After this change, a failure while updating the booleans and class > directories will cause the entire policy load to be canceled, leaving > the original policy intact, and policy load notifications to userspace > will only happen after a successful completion of updating those > directories. This does not (yet) provide full atomicity with respect > to the updating of the directory trees themselves. > > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> > --- > This patch is relative to my previous one, > https://patchwork.kernel.org/patch/11698505/. Although this does > not ensure atomicity when updating the selinuxfs directoty tree, > I suspect it will solve Daniel's original bug because systemd/dbusd > won't get the policy load notifications until the kernel is done > updating selinuxfs and therefore won't try to re-read selinuxfs > in the middle of it (because libselinux caches the class/perm > mappings and only flushes on a reload). Recognizing that re-basing the selinux namespace patches on top of these two patches might be painful, I went ahead and did so; the result can be found here: https://github.com/stephensmalley/selinux-kernel/tree/working-selinuxns-rebase The two patches that required manual fix-ups were the first one ("selinux: rename selinux state to ns (namespace)") and the third one ("selinux: dynamically allocate selinux namespace"). The rest re-based without conflicts. The resulting tree built, booted, passed the selinux-testsuite, and I could successfully follow the instructions to create a new namespace and load a policy into it. As before, the child namespace won't be usable if you switch it to enforcing mode since we haven't yet revived the per-namespace support for inode and superblock security blobs and it is still very unsafe to use in its current form.
On 8/4/20 9:53 AM, Stephen Smalley wrote: > With the refactoring of the policy load logic in the security > server from the previous change, it is now possible to split out > the committing of the new policy from security_load_policy() and > perform it only after successful updating of selinuxfs. Change > security_load_policy() to return the newly populated policy > data structures to the caller, export selinux_policy_commit() > for external callers, and introduce selinux_policy_cancel() to > provide a way to cancel the policy load in the event of an error > during updating of the selinuxfs directory tree. Further, rework > the interfaces used by selinuxfs to get information from the policy > when creating the new directory tree to take and act upon the > new policy data structure rather than the current/active policy. > Update selinuxfs to use these updated and new interfaces. While > we are here, stop re-creating the policy_capabilities directory > on each policy load since it does not depend on the policy, and > stop trying to create the booleans and classes directories during > the initial creation of selinuxfs since no information is available > until first policy load. > > After this change, a failure while updating the booleans and class > directories will cause the entire policy load to be canceled, leaving > the original policy intact, and policy load notifications to userspace > will only happen after a successful completion of updating those > directories. This does not (yet) provide full atomicity with respect > to the updating of the directory trees themselves. I have a patch series to perform the atomic updates very close to done, using vfs_rename with RENAME_EXCHANGE to update the directories out of tree and then swap them in as discussed earlier. I've just been doing some final style cleanup before sending to the list. I'll need to rebase on top of these changes of course. I didn't touch any of the error recovery portions, so I hope my series will complement this patch nicely. > > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> > --- > This patch is relative to my previous one, > https://patchwork.kernel.org/patch/11698505/. Although this does > not ensure atomicity when updating the selinuxfs directoty tree, > I suspect it will solve Daniel's original bug because systemd/dbusd > won't get the policy load notifications until the kernel is done > updating selinuxfs and therefore won't try to re-read selinuxfs > in the middle of it (because libselinux caches the class/perm > mappings and only flushes on a reload). I agree with your suspicion that this will resolve the bug we've been seeing (although only as a result of changing the timing, as you point out). Thanks for your work on this! > > security/selinux/include/conditional.h | 2 +- > security/selinux/include/security.h | 16 ++++- > security/selinux/selinuxfs.c | 69 ++++++++++---------- > security/selinux/ss/services.c | 87 +++++++++++++------------- > security/selinux/ss/sidtab.c | 10 +++ > security/selinux/ss/sidtab.h | 2 + > 6 files changed, 106 insertions(+), 80 deletions(-) > > diff --git a/security/selinux/include/conditional.h b/security/selinux/include/conditional.h > index 539ab357707d..b09343346e3f 100644 > --- a/security/selinux/include/conditional.h > +++ b/security/selinux/include/conditional.h > @@ -13,7 +13,7 @@ > > #include "security.h" > > -int security_get_bools(struct selinux_state *state, > +int security_get_bools(struct selinux_policy *policy, > u32 *len, char ***names, int **values); > > int security_set_bools(struct selinux_state *state, u32 len, int *values); > diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h > index b0e02cfe3ce1..7fa67bfb2f9f 100644 > --- a/security/selinux/include/security.h > +++ b/security/selinux/include/security.h > @@ -99,6 +99,7 @@ extern const char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX]; > > struct selinux_avc; > struct selinux_ss; > +struct selinux_policy; > > struct selinux_state { > #ifdef CONFIG_SECURITY_SELINUX_DISABLE > @@ -224,7 +225,12 @@ static inline bool selinux_policycap_genfs_seclabel_symlinks(void) > > int security_mls_enabled(struct selinux_state *state); > int security_load_policy(struct selinux_state *state, > - void *data, size_t len); > + void *data, size_t len, > + struct selinux_policy **newpolicyp); > +void selinux_policy_commit(struct selinux_state *state, > + struct selinux_policy *newpolicy); > +void selinux_policy_cancel(struct selinux_state *state, > + struct selinux_policy *policy); > int security_read_policy(struct selinux_state *state, > void **data, size_t *len); > size_t security_policydb_len(struct selinux_state *state); > @@ -358,9 +364,9 @@ int security_net_peersid_resolve(struct selinux_state *state, > u32 xfrm_sid, > u32 *peer_sid); > > -int security_get_classes(struct selinux_state *state, > +int security_get_classes(struct selinux_policy *policy, > char ***classes, int *nclasses); > -int security_get_permissions(struct selinux_state *state, > +int security_get_permissions(struct selinux_policy *policy, > char *class, char ***perms, int *nperms); > int security_get_reject_unknown(struct selinux_state *state); > int security_get_allow_unknown(struct selinux_state *state); > @@ -380,6 +386,10 @@ int security_genfs_sid(struct selinux_state *state, > const char *fstype, char *name, u16 sclass, > u32 *sid); > > +int selinux_policy_genfs_sid(struct selinux_policy *policy, > + const char *fstype, char *name, u16 sclass, > + u32 *sid); > + > #ifdef CONFIG_NETLABEL > int security_netlbl_secattr_to_sid(struct selinux_state *state, > struct netlbl_lsm_secattr *secattr, > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c > index 4781314c2510..131816878e50 100644 > --- a/security/selinux/selinuxfs.c > +++ b/security/selinux/selinuxfs.c > @@ -346,9 +346,10 @@ static const struct file_operations sel_policyvers_ops = { > }; > > /* declaration for sel_write_load */ > -static int sel_make_bools(struct selinux_fs_info *fsi); > -static int sel_make_classes(struct selinux_fs_info *fsi); > -static int sel_make_policycap(struct selinux_fs_info *fsi); > +static int sel_make_bools(struct selinux_fs_info *fsi, > + struct selinux_policy *newpolicy); > +static int sel_make_classes(struct selinux_fs_info *fsi, > + struct selinux_policy *newpolicy); > > /* declaration for sel_make_class_dirs */ > static struct dentry *sel_make_dir(struct dentry *dir, const char *name, > @@ -508,28 +509,23 @@ static const struct file_operations sel_policy_ops = { > .llseek = generic_file_llseek, > }; > > -static int sel_make_policy_nodes(struct selinux_fs_info *fsi) > +static int sel_make_policy_nodes(struct selinux_fs_info *fsi, > + struct selinux_policy *newpolicy) > { > int ret; > > - ret = sel_make_bools(fsi); > + ret = sel_make_bools(fsi, newpolicy); > if (ret) { > pr_err("SELinux: failed to load policy booleans\n"); > return ret; > } > > - ret = sel_make_classes(fsi); > + ret = sel_make_classes(fsi, newpolicy); > if (ret) { > pr_err("SELinux: failed to load policy classes\n"); > return ret; > } > > - ret = sel_make_policycap(fsi); > - if (ret) { > - pr_err("SELinux: failed to load policy capabilities\n"); > - return ret; > - } > - > return 0; > } > > @@ -538,6 +534,7 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf, > > { > struct selinux_fs_info *fsi = file_inode(file)->i_sb->s_fs_info; > + struct selinux_policy *newpolicy; > ssize_t length; > void *data = NULL; > > @@ -563,15 +560,19 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf, > if (copy_from_user(data, buf, count) != 0) > goto out; > > - length = security_load_policy(fsi->state, data, count); > + length = security_load_policy(fsi->state, data, count, &newpolicy); > if (length) { > pr_warn_ratelimited("SELinux: failed to load policy\n"); > goto out; > } > > - length = sel_make_policy_nodes(fsi); > - if (length) > + length = sel_make_policy_nodes(fsi, newpolicy); > + if (length) { > + selinux_policy_cancel(fsi->state, newpolicy); > goto out1; As things stand as of this patch, I think that this means that in the event of a failure in recreating the directories, that directory will be left unpopulated or partially populated. We could even get in a state where the booleans directory has already been updated to the new policy and the class directory has not. The full solution is of course atomic swapover, which as I mentioned above I hope to submit a series for soon, but I wonder if recreating the directories on the old policy would be a better interim state? That probably depends on what sorts of errors are possible. If we've failed because of something about the new policy, recreating the old directories should get us back to a good state. If we can't create new directories at all for whatever reason, trying to recreate might leave us worse off than before we started. > + } > + > + selinux_policy_commit(fsi->state, newpolicy); > > length = count; > > @@ -1333,7 +1334,8 @@ static void sel_remove_entries(struct dentry *de) > > #define BOOL_DIR_NAME "booleans" > > -static int sel_make_bools(struct selinux_fs_info *fsi) > +static int sel_make_bools(struct selinux_fs_info *fsi, > + struct selinux_policy *newpolicy) > { > int ret; > ssize_t len; > @@ -1362,7 +1364,7 @@ static int sel_make_bools(struct selinux_fs_info *fsi) > if (!page) > goto out; > > - ret = security_get_bools(fsi->state, &num, &names, &values); > + ret = security_get_bools(newpolicy, &num, &names, &values); > if (ret) > goto out; > > @@ -1388,7 +1390,7 @@ static int sel_make_bools(struct selinux_fs_info *fsi) > } > > isec = selinux_inode(inode); > - ret = security_genfs_sid(fsi->state, "selinuxfs", page, > + ret = selinux_policy_genfs_sid(newpolicy, "selinuxfs", page, > SECCLASS_FILE, &sid); > if (ret) { > pr_warn_ratelimited("SELinux: no sid found, defaulting to security isid for %s\n", > @@ -1791,14 +1793,14 @@ static const struct file_operations sel_policycap_ops = { > .llseek = generic_file_llseek, > }; > > -static int sel_make_perm_files(char *objclass, int classvalue, > - struct dentry *dir) > +static int sel_make_perm_files(struct selinux_policy *newpolicy, > + char *objclass, int classvalue, > + struct dentry *dir) > { > - struct selinux_fs_info *fsi = dir->d_sb->s_fs_info; > int i, rc, nperms; > char **perms; > > - rc = security_get_permissions(fsi->state, objclass, &perms, &nperms); > + rc = security_get_permissions(newpolicy, objclass, &perms, &nperms); > if (rc) > return rc; > > @@ -1831,8 +1833,9 @@ static int sel_make_perm_files(char *objclass, int classvalue, > return rc; > } > > -static int sel_make_class_dir_entries(char *classname, int index, > - struct dentry *dir) > +static int sel_make_class_dir_entries(struct selinux_policy *newpolicy, > + char *classname, int index, > + struct dentry *dir) > { > struct super_block *sb = dir->d_sb; > struct selinux_fs_info *fsi = sb->s_fs_info; > @@ -1858,12 +1861,13 @@ static int sel_make_class_dir_entries(char *classname, int index, > if (IS_ERR(dentry)) > return PTR_ERR(dentry); > > - rc = sel_make_perm_files(classname, index, dentry); > + rc = sel_make_perm_files(newpolicy, classname, index, dentry); > > return rc; > } > > -static int sel_make_classes(struct selinux_fs_info *fsi) > +static int sel_make_classes(struct selinux_fs_info *fsi, > + struct selinux_policy *newpolicy) > { > > int rc, nclasses, i; > @@ -1872,7 +1876,7 @@ static int sel_make_classes(struct selinux_fs_info *fsi) > /* delete any existing entries */ > sel_remove_entries(fsi->class_dir); > > - rc = security_get_classes(fsi->state, &classes, &nclasses); > + rc = security_get_classes(newpolicy, &classes, &nclasses); > if (rc) > return rc; > > @@ -1890,7 +1894,7 @@ static int sel_make_classes(struct selinux_fs_info *fsi) > } > > /* i+1 since class values are 1-indexed */ > - rc = sel_make_class_dir_entries(classes[i], i + 1, > + rc = sel_make_class_dir_entries(newpolicy, classes[i], i + 1, > class_name_dir); > if (rc) > goto out; > @@ -1909,8 +1913,6 @@ static int sel_make_policycap(struct selinux_fs_info *fsi) > struct dentry *dentry = NULL; > struct inode *inode = NULL; > > - sel_remove_entries(fsi->policycap_dir); > - > for (iter = 0; iter <= POLICYDB_CAPABILITY_MAX; iter++) { > if (iter < ARRAY_SIZE(selinux_policycap_names)) > dentry = d_alloc_name(fsi->policycap_dir, > @@ -2075,9 +2077,12 @@ static int sel_fill_super(struct super_block *sb, struct fs_context *fc) > goto err; > } > > - ret = sel_make_policy_nodes(fsi); > - if (ret) > + ret = sel_make_policycap(fsi); > + if (ret) { > + pr_err("SELinux: failed to load policy capabilities\n"); > goto err; > + } > + > return 0; > err: > pr_err("SELinux: %s: failed while creating inodes\n", > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > index adc11d97b3ae..07a41e8db2d3 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -2143,8 +2143,18 @@ static void selinux_policy_free(struct selinux_policy *policy) > kfree(policy); > } > > -static void selinux_policy_commit(struct selinux_state *state, > - struct selinux_policy *newpolicy) > +void selinux_policy_cancel(struct selinux_state *state, > + struct selinux_policy *policy) > +{ > + > + read_lock(&state->ss->policy_rwlock); > + sidtab_cancel_convert(&state->ss->policy->sidtab); > + read_unlock(&state->ss->policy_rwlock); > + selinux_policy_free(policy); > +} > + > +void selinux_policy_commit(struct selinux_state *state, > + struct selinux_policy *newpolicy) > { > struct selinux_policy *oldpolicy; > u32 seqno; > @@ -2200,7 +2210,8 @@ static void selinux_policy_commit(struct selinux_state *state, > * This function will flush the access vector cache after > * loading the new policy. > */ > -int security_load_policy(struct selinux_state *state, void *data, size_t len) > +int security_load_policy(struct selinux_state *state, void *data, size_t len, > + struct selinux_policy **newpolicyp) > { > struct selinux_policy *newpolicy; > struct sidtab_convert_params convert_params; > @@ -2230,7 +2241,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > > if (!selinux_initialized(state)) { > /* First policy load, so no need to preserve state from old policy */ > - selinux_policy_commit(state, newpolicy); > + *newpolicyp = newpolicy; > return 0; > } > > @@ -2264,7 +2275,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > goto err; > } > > - selinux_policy_commit(state, newpolicy); > + *newpolicyp = newpolicy; > return 0; > > err: > @@ -2691,17 +2702,15 @@ int security_get_user_sids(struct selinux_state *state, > * Obtain a SID to use for a file in a filesystem that > * cannot support xattr or use a fixed labeling behavior like > * transition SIDs or task SIDs. > - * > - * The caller must acquire the policy_rwlock before calling this function. > */ > -static inline int __security_genfs_sid(struct selinux_state *state, > +static inline int __security_genfs_sid(struct selinux_policy *policy, > const char *fstype, > char *path, > u16 orig_sclass, > u32 *sid) > { > - struct policydb *policydb = &state->ss->policy->policydb; > - struct sidtab *sidtab = &state->ss->policy->sidtab; > + struct policydb *policydb = &policy->policydb; > + struct sidtab *sidtab = &policy->sidtab; > int len; > u16 sclass; > struct genfs *genfs; > @@ -2711,7 +2720,7 @@ static inline int __security_genfs_sid(struct selinux_state *state, > while (path[0] == '/' && path[1] == '/') > path++; > > - sclass = unmap_class(&state->ss->policy->map, orig_sclass); > + sclass = unmap_class(&policy->map, orig_sclass); > *sid = SECINITSID_UNLABELED; > > for (genfs = policydb->genfs; genfs; genfs = genfs->next) { > @@ -2766,11 +2775,22 @@ int security_genfs_sid(struct selinux_state *state, > int retval; > > read_lock(&state->ss->policy_rwlock); > - retval = __security_genfs_sid(state, fstype, path, orig_sclass, sid); > + retval = __security_genfs_sid(state->ss->policy, > + fstype, path, orig_sclass, sid); > read_unlock(&state->ss->policy_rwlock); > return retval; > } > > +int selinux_policy_genfs_sid(struct selinux_policy *policy, > + const char *fstype, > + char *path, > + u16 orig_sclass, > + u32 *sid) > +{ > + /* no lock required, policy is not yet accessible by other threads */ > + return __security_genfs_sid(policy, fstype, path, orig_sclass, sid); > +} > + > /** > * security_fs_use - Determine how to handle labeling for a filesystem. > * @sb: superblock in question > @@ -2806,8 +2826,8 @@ int security_fs_use(struct selinux_state *state, struct super_block *sb) > } > sbsec->sid = c->sid[0]; > } else { > - rc = __security_genfs_sid(state, fstype, "/", SECCLASS_DIR, > - &sbsec->sid); > + rc = __security_genfs_sid(state->ss->policy, fstype, "/", > + SECCLASS_DIR, &sbsec->sid); > if (rc) { > sbsec->behavior = SECURITY_FS_USE_NONE; > rc = 0; > @@ -2821,23 +2841,14 @@ int security_fs_use(struct selinux_state *state, struct super_block *sb) > return rc; > } > > -int security_get_bools(struct selinux_state *state, > +int security_get_bools(struct selinux_policy *policy, > u32 *len, char ***names, int **values) > { > struct policydb *policydb; > u32 i; > int rc; > > - if (!selinux_initialized(state)) { > - *len = 0; > - *names = NULL; > - *values = NULL; > - return 0; > - } > - > - read_lock(&state->ss->policy_rwlock); > - > - policydb = &state->ss->policy->policydb; > + policydb = &policy->policydb; > > *names = NULL; > *values = NULL; > @@ -2868,7 +2879,6 @@ int security_get_bools(struct selinux_state *state, > } > rc = 0; > out: > - read_unlock(&state->ss->policy_rwlock); > return rc; > err: > if (*names) { > @@ -2957,7 +2967,9 @@ static int security_preserve_bools(struct selinux_state *state, > struct cond_bool_datum *booldatum; > u32 i, nbools = 0; > > - rc = security_get_bools(state, &nbools, &bnames, &bvalues); > + read_lock(&state->ss->policy_rwlock); > + rc = security_get_bools(state->ss->policy, &nbools, &bnames, &bvalues); > + read_unlock(&state->ss->policy_rwlock); > if (rc) > goto out; > for (i = 0; i < nbools; i++) { > @@ -3168,21 +3180,13 @@ static int get_classes_callback(void *k, void *d, void *args) > return 0; > } > > -int security_get_classes(struct selinux_state *state, > +int security_get_classes(struct selinux_policy *policy, > char ***classes, int *nclasses) > { > struct policydb *policydb; > int rc; > > - if (!selinux_initialized(state)) { > - *nclasses = 0; > - *classes = NULL; > - return 0; > - } > - > - read_lock(&state->ss->policy_rwlock); > - > - policydb = &state->ss->policy->policydb; > + policydb = &policy->policydb; > > rc = -ENOMEM; > *nclasses = policydb->p_classes.nprim; > @@ -3200,7 +3204,6 @@ int security_get_classes(struct selinux_state *state, > } > > out: > - read_unlock(&state->ss->policy_rwlock); > return rc; > } > > @@ -3217,16 +3220,14 @@ static int get_permissions_callback(void *k, void *d, void *args) > return 0; > } > > -int security_get_permissions(struct selinux_state *state, > +int security_get_permissions(struct selinux_policy *policy, > char *class, char ***perms, int *nperms) > { > struct policydb *policydb; > int rc, i; > struct class_datum *match; > > - read_lock(&state->ss->policy_rwlock); > - > - policydb = &state->ss->policy->policydb; > + policydb = &policy->policydb; > > rc = -EINVAL; > match = symtab_search(&policydb->p_classes, class); > @@ -3255,11 +3256,9 @@ int security_get_permissions(struct selinux_state *state, > goto err; > > out: > - read_unlock(&state->ss->policy_rwlock); > return rc; > > err: > - read_unlock(&state->ss->policy_rwlock); > for (i = 0; i < *nperms; i++) > kfree((*perms)[i]); > kfree(*perms); > diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c > index 47be8bcf6bed..4a24ded670af 100644 > --- a/security/selinux/ss/sidtab.c > +++ b/security/selinux/ss/sidtab.c > @@ -463,6 +463,16 @@ int sidtab_convert(struct sidtab *s, struct sidtab_convert_params *params) > return 0; > } > > +void sidtab_cancel_convert(struct sidtab *s) > +{ > + unsigned long flags; > + > + /* cancelling policy load - disable live convert of sidtab */ > + spin_lock_irqsave(&s->lock, flags); > + s->convert = NULL; > + spin_unlock_irqrestore(&s->lock, flags); > +} > + > static void sidtab_destroy_entry(struct sidtab_entry *entry) > { > context_destroy(&entry->context); > diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h > index f2a84560b8b3..80c744d07ad6 100644 > --- a/security/selinux/ss/sidtab.h > +++ b/security/selinux/ss/sidtab.h > @@ -123,6 +123,8 @@ static inline struct context *sidtab_search_force(struct sidtab *s, u32 sid) > > int sidtab_convert(struct sidtab *s, struct sidtab_convert_params *params); > > +void sidtab_cancel_convert(struct sidtab *s); > + > int sidtab_context_to_sid(struct sidtab *s, struct context *context, u32 *sid); > > void sidtab_destroy(struct sidtab *s);
On 8/4/20 4:51 PM, Daniel Burgener wrote: > On 8/4/20 9:53 AM, Stephen Smalley wrote: >> With the refactoring of the policy load logic in the security >> server from the previous change, it is now possible to split out >> the committing of the new policy from security_load_policy() and >> perform it only after successful updating of selinuxfs. Change >> security_load_policy() to return the newly populated policy >> data structures to the caller, export selinux_policy_commit() >> for external callers, and introduce selinux_policy_cancel() to >> provide a way to cancel the policy load in the event of an error >> during updating of the selinuxfs directory tree. Further, rework >> the interfaces used by selinuxfs to get information from the policy >> when creating the new directory tree to take and act upon the >> new policy data structure rather than the current/active policy. >> Update selinuxfs to use these updated and new interfaces. While >> we are here, stop re-creating the policy_capabilities directory >> on each policy load since it does not depend on the policy, and >> stop trying to create the booleans and classes directories during >> the initial creation of selinuxfs since no information is available >> until first policy load. >> >> After this change, a failure while updating the booleans and class >> directories will cause the entire policy load to be canceled, leaving >> the original policy intact, and policy load notifications to userspace >> will only happen after a successful completion of updating those >> directories. This does not (yet) provide full atomicity with respect >> to the updating of the directory trees themselves. > > I have a patch series to perform the atomic updates very close to > done, using vfs_rename with RENAME_EXCHANGE to update the directories > out of tree and then swap them in as discussed earlier. I've just > been doing some final style cleanup before sending to the list. I'll > need to rebase on top of these changes of course. I didn't touch any > of the error recovery portions, so I hope my series will complement > this patch nicely. Great, I was trying to ensure that we wouldn't conflict/overlap significantly. >> This patch is relative to my previous one, >> https://patchwork.kernel.org/patch/11698505/. Although this does >> not ensure atomicity when updating the selinuxfs directoty tree, >> I suspect it will solve Daniel's original bug because systemd/dbusd >> won't get the policy load notifications until the kernel is done >> updating selinuxfs and therefore won't try to re-read selinuxfs >> in the middle of it (because libselinux caches the class/perm >> mappings and only flushes on a reload). > I agree with your suspicion that this will resolve the bug we've been > seeing (although only as a result of changing the timing, as you point > out). Thanks for your work on this! If you can easily test that my patches resolve that bug for you, you could add a Tested-by tag. One caveat is that it sounds like I'll be making one more change to the previous patch per Ondrej's request to avoid taking the read lock around sidtab_convert(). >> @@ -563,15 +560,19 @@ static ssize_t sel_write_load(struct file >> *file, const char __user *buf, >> if (copy_from_user(data, buf, count) != 0) >> goto out; >> - length = security_load_policy(fsi->state, data, count); >> + length = security_load_policy(fsi->state, data, count, &newpolicy); >> if (length) { >> pr_warn_ratelimited("SELinux: failed to load policy\n"); >> goto out; >> } >> - length = sel_make_policy_nodes(fsi); >> - if (length) >> + length = sel_make_policy_nodes(fsi, newpolicy); >> + if (length) { >> + selinux_policy_cancel(fsi->state, newpolicy); >> goto out1; > As things stand as of this patch, I think that this means that in the > event of a failure in recreating the directories, that directory will > be left unpopulated or partially populated. We could even get in a > state where the booleans directory has already been updated to the new > policy and the class directory has not. The full solution is of course > atomic swapover, which as I mentioned above I hope to submit a series > for soon, but I wonder if recreating the directories on the old policy > would be a better interim state? That probably depends on what sorts > of errors are possible. If we've failed because of something about > the new policy, recreating the old directories should get us back to a > good state. If we can't create new directories at all for whatever > reason, trying to recreate might leave us worse off than before we > started. I deliberately avoided any changes to the error handling during re-creation of the booleans and class directories because I viewed that as logically separate from my change and likely to conflict with your changes. So I expect to revisit that issue after both my patches and yours land. I think the only scenario where sel_make_bools/classes() can fail is an out-of-memory condition and if we are out of memory then we are unlikely to be able to re-create the old directories/files again. Hence, I don't think there is anything useful we can do without the atomic swapover. At most, we can delete everything under booleans and class on any failure while re-creating so that we aren't left with the partial set of booleans/classes. The other possibility I considered is explicitly checking whether there are any changes to booleans or classes between the old and new policies and if not, skipping that part of the selinuxfs update. That however would require a new security server function to iterate over all of the booleans and classes in two selinux_policy structures and compare them for equality. Didn't seem worth it if the atomic swapover support was coming anyway.
On 8/5/20 8:35 AM, Stephen Smalley wrote: > On 8/4/20 4:51 PM, Daniel Burgener wrote: > >> On 8/4/20 9:53 AM, Stephen Smalley wrote: >>> With the refactoring of the policy load logic in the security >>> server from the previous change, it is now possible to split out >>> the committing of the new policy from security_load_policy() and >>> perform it only after successful updating of selinuxfs. Change >>> security_load_policy() to return the newly populated policy >>> data structures to the caller, export selinux_policy_commit() >>> for external callers, and introduce selinux_policy_cancel() to >>> provide a way to cancel the policy load in the event of an error >>> during updating of the selinuxfs directory tree. Further, rework >>> the interfaces used by selinuxfs to get information from the policy >>> when creating the new directory tree to take and act upon the >>> new policy data structure rather than the current/active policy. >>> Update selinuxfs to use these updated and new interfaces. While >>> we are here, stop re-creating the policy_capabilities directory >>> on each policy load since it does not depend on the policy, and >>> stop trying to create the booleans and classes directories during >>> the initial creation of selinuxfs since no information is available >>> until first policy load. >>> >>> After this change, a failure while updating the booleans and class >>> directories will cause the entire policy load to be canceled, leaving >>> the original policy intact, and policy load notifications to userspace >>> will only happen after a successful completion of updating those >>> directories. This does not (yet) provide full atomicity with respect >>> to the updating of the directory trees themselves. >> >> I have a patch series to perform the atomic updates very close to >> done, using vfs_rename with RENAME_EXCHANGE to update the directories >> out of tree and then swap them in as discussed earlier. I've just >> been doing some final style cleanup before sending to the list. I'll >> need to rebase on top of these changes of course. I didn't touch any >> of the error recovery portions, so I hope my series will complement >> this patch nicely. > > Great, I was trying to ensure that we wouldn't conflict/overlap > significantly. > >>> This patch is relative to my previous one, >>> https://patchwork.kernel.org/patch/11698505/. Although this does >>> not ensure atomicity when updating the selinuxfs directoty tree, >>> I suspect it will solve Daniel's original bug because systemd/dbusd >>> won't get the policy load notifications until the kernel is done >>> updating selinuxfs and therefore won't try to re-read selinuxfs >>> in the middle of it (because libselinux caches the class/perm >>> mappings and only flushes on a reload). >> I agree with your suspicion that this will resolve the bug we've been >> seeing (although only as a result of changing the timing, as you >> point out). Thanks for your work on this! > > If you can easily test that my patches resolve that bug for you, you > could add a Tested-by tag. One caveat is that it sounds like I'll be > making one more change to the previous patch per Ondrej's request to > avoid taking the read lock around sidtab_convert(). Sounds good. I will wait for your update, and then test this series. > >>> @@ -563,15 +560,19 @@ static ssize_t sel_write_load(struct file >>> *file, const char __user *buf, >>> if (copy_from_user(data, buf, count) != 0) >>> goto out; >>> - length = security_load_policy(fsi->state, data, count); >>> + length = security_load_policy(fsi->state, data, count, >>> &newpolicy); >>> if (length) { >>> pr_warn_ratelimited("SELinux: failed to load policy\n"); >>> goto out; >>> } >>> - length = sel_make_policy_nodes(fsi); >>> - if (length) >>> + length = sel_make_policy_nodes(fsi, newpolicy); >>> + if (length) { >>> + selinux_policy_cancel(fsi->state, newpolicy); >>> goto out1; >> As things stand as of this patch, I think that this means that in the >> event of a failure in recreating the directories, that directory will >> be left unpopulated or partially populated. We could even get in a >> state where the booleans directory has already been updated to the >> new policy and the class directory has not. The full solution is of >> course atomic swapover, which as I mentioned above I hope to submit a >> series for soon, but I wonder if recreating the directories on the >> old policy would be a better interim state? That probably depends on >> what sorts of errors are possible. If we've failed because of >> something about the new policy, recreating the old directories should >> get us back to a good state. If we can't create new directories at >> all for whatever reason, trying to recreate might leave us worse off >> than before we started. > > I deliberately avoided any changes to the error handling during > re-creation of the booleans and class directories because I viewed > that as logically separate from my change and likely to conflict with > your changes. So I expect to revisit that issue after both my patches > and yours land. I think the only scenario where > sel_make_bools/classes() can fail is an out-of-memory condition and if > we are out of memory then we are unlikely to be able to re-create the > old directories/files again. Hence, I don't think there is anything > useful we can do without the atomic swapover. At most, we can delete > everything under booleans and class on any failure while re-creating > so that we aren't left with the partial set of booleans/classes. > > The other possibility I considered is explicitly checking whether > there are any changes to booleans or classes between the old and new > policies and if not, skipping that part of the selinuxfs update. That > however would require a new security server function to iterate over > all of the booleans and classes in two selinux_policy structures and > compare them for equality. Didn't seem worth it if the atomic > swapover support was coming anyway. > Sounds good. -Daniel
On 8/4/20 4:19 PM, Stephen Smalley wrote: > Recognizing that re-basing the selinux namespace patches on top of > these two patches might be painful, I went ahead and did so; the > result can be found here: > > https://github.com/stephensmalley/selinux-kernel/tree/working-selinuxns-rebase > > > The two patches that required manual fix-ups were the first one > ("selinux: rename selinux state to ns (namespace)") and the third one > ("selinux: dynamically allocate selinux namespace"). The rest re-based > without conflicts. The resulting tree built, booted, passed the > selinux-testsuite, and I could successfully follow the instructions to > create a new namespace and load a policy into it. As before, the child > namespace won't be usable if you switch it to enforcing mode since we > haven't yet revived the per-namespace support for inode and superblock > security blobs and it is still very unsafe to use in its current form. Re-based again with the revised version of both patches. This required one additional manual fix-up for "selinux: annotate lockdep for services locks" due to conflicting with the restored load_mutex.
diff --git a/security/selinux/include/conditional.h b/security/selinux/include/conditional.h index 539ab357707d..b09343346e3f 100644 --- a/security/selinux/include/conditional.h +++ b/security/selinux/include/conditional.h @@ -13,7 +13,7 @@ #include "security.h" -int security_get_bools(struct selinux_state *state, +int security_get_bools(struct selinux_policy *policy, u32 *len, char ***names, int **values); int security_set_bools(struct selinux_state *state, u32 len, int *values); diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index b0e02cfe3ce1..7fa67bfb2f9f 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -99,6 +99,7 @@ extern const char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX]; struct selinux_avc; struct selinux_ss; +struct selinux_policy; struct selinux_state { #ifdef CONFIG_SECURITY_SELINUX_DISABLE @@ -224,7 +225,12 @@ static inline bool selinux_policycap_genfs_seclabel_symlinks(void) int security_mls_enabled(struct selinux_state *state); int security_load_policy(struct selinux_state *state, - void *data, size_t len); + void *data, size_t len, + struct selinux_policy **newpolicyp); +void selinux_policy_commit(struct selinux_state *state, + struct selinux_policy *newpolicy); +void selinux_policy_cancel(struct selinux_state *state, + struct selinux_policy *policy); int security_read_policy(struct selinux_state *state, void **data, size_t *len); size_t security_policydb_len(struct selinux_state *state); @@ -358,9 +364,9 @@ int security_net_peersid_resolve(struct selinux_state *state, u32 xfrm_sid, u32 *peer_sid); -int security_get_classes(struct selinux_state *state, +int security_get_classes(struct selinux_policy *policy, char ***classes, int *nclasses); -int security_get_permissions(struct selinux_state *state, +int security_get_permissions(struct selinux_policy *policy, char *class, char ***perms, int *nperms); int security_get_reject_unknown(struct selinux_state *state); int security_get_allow_unknown(struct selinux_state *state); @@ -380,6 +386,10 @@ int security_genfs_sid(struct selinux_state *state, const char *fstype, char *name, u16 sclass, u32 *sid); +int selinux_policy_genfs_sid(struct selinux_policy *policy, + const char *fstype, char *name, u16 sclass, + u32 *sid); + #ifdef CONFIG_NETLABEL int security_netlbl_secattr_to_sid(struct selinux_state *state, struct netlbl_lsm_secattr *secattr, diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index 4781314c2510..131816878e50 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -346,9 +346,10 @@ static const struct file_operations sel_policyvers_ops = { }; /* declaration for sel_write_load */ -static int sel_make_bools(struct selinux_fs_info *fsi); -static int sel_make_classes(struct selinux_fs_info *fsi); -static int sel_make_policycap(struct selinux_fs_info *fsi); +static int sel_make_bools(struct selinux_fs_info *fsi, + struct selinux_policy *newpolicy); +static int sel_make_classes(struct selinux_fs_info *fsi, + struct selinux_policy *newpolicy); /* declaration for sel_make_class_dirs */ static struct dentry *sel_make_dir(struct dentry *dir, const char *name, @@ -508,28 +509,23 @@ static const struct file_operations sel_policy_ops = { .llseek = generic_file_llseek, }; -static int sel_make_policy_nodes(struct selinux_fs_info *fsi) +static int sel_make_policy_nodes(struct selinux_fs_info *fsi, + struct selinux_policy *newpolicy) { int ret; - ret = sel_make_bools(fsi); + ret = sel_make_bools(fsi, newpolicy); if (ret) { pr_err("SELinux: failed to load policy booleans\n"); return ret; } - ret = sel_make_classes(fsi); + ret = sel_make_classes(fsi, newpolicy); if (ret) { pr_err("SELinux: failed to load policy classes\n"); return ret; } - ret = sel_make_policycap(fsi); - if (ret) { - pr_err("SELinux: failed to load policy capabilities\n"); - return ret; - } - return 0; } @@ -538,6 +534,7 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf, { struct selinux_fs_info *fsi = file_inode(file)->i_sb->s_fs_info; + struct selinux_policy *newpolicy; ssize_t length; void *data = NULL; @@ -563,15 +560,19 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf, if (copy_from_user(data, buf, count) != 0) goto out; - length = security_load_policy(fsi->state, data, count); + length = security_load_policy(fsi->state, data, count, &newpolicy); if (length) { pr_warn_ratelimited("SELinux: failed to load policy\n"); goto out; } - length = sel_make_policy_nodes(fsi); - if (length) + length = sel_make_policy_nodes(fsi, newpolicy); + if (length) { + selinux_policy_cancel(fsi->state, newpolicy); goto out1; + } + + selinux_policy_commit(fsi->state, newpolicy); length = count; @@ -1333,7 +1334,8 @@ static void sel_remove_entries(struct dentry *de) #define BOOL_DIR_NAME "booleans" -static int sel_make_bools(struct selinux_fs_info *fsi) +static int sel_make_bools(struct selinux_fs_info *fsi, + struct selinux_policy *newpolicy) { int ret; ssize_t len; @@ -1362,7 +1364,7 @@ static int sel_make_bools(struct selinux_fs_info *fsi) if (!page) goto out; - ret = security_get_bools(fsi->state, &num, &names, &values); + ret = security_get_bools(newpolicy, &num, &names, &values); if (ret) goto out; @@ -1388,7 +1390,7 @@ static int sel_make_bools(struct selinux_fs_info *fsi) } isec = selinux_inode(inode); - ret = security_genfs_sid(fsi->state, "selinuxfs", page, + ret = selinux_policy_genfs_sid(newpolicy, "selinuxfs", page, SECCLASS_FILE, &sid); if (ret) { pr_warn_ratelimited("SELinux: no sid found, defaulting to security isid for %s\n", @@ -1791,14 +1793,14 @@ static const struct file_operations sel_policycap_ops = { .llseek = generic_file_llseek, }; -static int sel_make_perm_files(char *objclass, int classvalue, - struct dentry *dir) +static int sel_make_perm_files(struct selinux_policy *newpolicy, + char *objclass, int classvalue, + struct dentry *dir) { - struct selinux_fs_info *fsi = dir->d_sb->s_fs_info; int i, rc, nperms; char **perms; - rc = security_get_permissions(fsi->state, objclass, &perms, &nperms); + rc = security_get_permissions(newpolicy, objclass, &perms, &nperms); if (rc) return rc; @@ -1831,8 +1833,9 @@ static int sel_make_perm_files(char *objclass, int classvalue, return rc; } -static int sel_make_class_dir_entries(char *classname, int index, - struct dentry *dir) +static int sel_make_class_dir_entries(struct selinux_policy *newpolicy, + char *classname, int index, + struct dentry *dir) { struct super_block *sb = dir->d_sb; struct selinux_fs_info *fsi = sb->s_fs_info; @@ -1858,12 +1861,13 @@ static int sel_make_class_dir_entries(char *classname, int index, if (IS_ERR(dentry)) return PTR_ERR(dentry); - rc = sel_make_perm_files(classname, index, dentry); + rc = sel_make_perm_files(newpolicy, classname, index, dentry); return rc; } -static int sel_make_classes(struct selinux_fs_info *fsi) +static int sel_make_classes(struct selinux_fs_info *fsi, + struct selinux_policy *newpolicy) { int rc, nclasses, i; @@ -1872,7 +1876,7 @@ static int sel_make_classes(struct selinux_fs_info *fsi) /* delete any existing entries */ sel_remove_entries(fsi->class_dir); - rc = security_get_classes(fsi->state, &classes, &nclasses); + rc = security_get_classes(newpolicy, &classes, &nclasses); if (rc) return rc; @@ -1890,7 +1894,7 @@ static int sel_make_classes(struct selinux_fs_info *fsi) } /* i+1 since class values are 1-indexed */ - rc = sel_make_class_dir_entries(classes[i], i + 1, + rc = sel_make_class_dir_entries(newpolicy, classes[i], i + 1, class_name_dir); if (rc) goto out; @@ -1909,8 +1913,6 @@ static int sel_make_policycap(struct selinux_fs_info *fsi) struct dentry *dentry = NULL; struct inode *inode = NULL; - sel_remove_entries(fsi->policycap_dir); - for (iter = 0; iter <= POLICYDB_CAPABILITY_MAX; iter++) { if (iter < ARRAY_SIZE(selinux_policycap_names)) dentry = d_alloc_name(fsi->policycap_dir, @@ -2075,9 +2077,12 @@ static int sel_fill_super(struct super_block *sb, struct fs_context *fc) goto err; } - ret = sel_make_policy_nodes(fsi); - if (ret) + ret = sel_make_policycap(fsi); + if (ret) { + pr_err("SELinux: failed to load policy capabilities\n"); goto err; + } + return 0; err: pr_err("SELinux: %s: failed while creating inodes\n", diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index adc11d97b3ae..07a41e8db2d3 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -2143,8 +2143,18 @@ static void selinux_policy_free(struct selinux_policy *policy) kfree(policy); } -static void selinux_policy_commit(struct selinux_state *state, - struct selinux_policy *newpolicy) +void selinux_policy_cancel(struct selinux_state *state, + struct selinux_policy *policy) +{ + + read_lock(&state->ss->policy_rwlock); + sidtab_cancel_convert(&state->ss->policy->sidtab); + read_unlock(&state->ss->policy_rwlock); + selinux_policy_free(policy); +} + +void selinux_policy_commit(struct selinux_state *state, + struct selinux_policy *newpolicy) { struct selinux_policy *oldpolicy; u32 seqno; @@ -2200,7 +2210,8 @@ static void selinux_policy_commit(struct selinux_state *state, * This function will flush the access vector cache after * loading the new policy. */ -int security_load_policy(struct selinux_state *state, void *data, size_t len) +int security_load_policy(struct selinux_state *state, void *data, size_t len, + struct selinux_policy **newpolicyp) { struct selinux_policy *newpolicy; struct sidtab_convert_params convert_params; @@ -2230,7 +2241,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) if (!selinux_initialized(state)) { /* First policy load, so no need to preserve state from old policy */ - selinux_policy_commit(state, newpolicy); + *newpolicyp = newpolicy; return 0; } @@ -2264,7 +2275,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) goto err; } - selinux_policy_commit(state, newpolicy); + *newpolicyp = newpolicy; return 0; err: @@ -2691,17 +2702,15 @@ int security_get_user_sids(struct selinux_state *state, * Obtain a SID to use for a file in a filesystem that * cannot support xattr or use a fixed labeling behavior like * transition SIDs or task SIDs. - * - * The caller must acquire the policy_rwlock before calling this function. */ -static inline int __security_genfs_sid(struct selinux_state *state, +static inline int __security_genfs_sid(struct selinux_policy *policy, const char *fstype, char *path, u16 orig_sclass, u32 *sid) { - struct policydb *policydb = &state->ss->policy->policydb; - struct sidtab *sidtab = &state->ss->policy->sidtab; + struct policydb *policydb = &policy->policydb; + struct sidtab *sidtab = &policy->sidtab; int len; u16 sclass; struct genfs *genfs; @@ -2711,7 +2720,7 @@ static inline int __security_genfs_sid(struct selinux_state *state, while (path[0] == '/' && path[1] == '/') path++; - sclass = unmap_class(&state->ss->policy->map, orig_sclass); + sclass = unmap_class(&policy->map, orig_sclass); *sid = SECINITSID_UNLABELED; for (genfs = policydb->genfs; genfs; genfs = genfs->next) { @@ -2766,11 +2775,22 @@ int security_genfs_sid(struct selinux_state *state, int retval; read_lock(&state->ss->policy_rwlock); - retval = __security_genfs_sid(state, fstype, path, orig_sclass, sid); + retval = __security_genfs_sid(state->ss->policy, + fstype, path, orig_sclass, sid); read_unlock(&state->ss->policy_rwlock); return retval; } +int selinux_policy_genfs_sid(struct selinux_policy *policy, + const char *fstype, + char *path, + u16 orig_sclass, + u32 *sid) +{ + /* no lock required, policy is not yet accessible by other threads */ + return __security_genfs_sid(policy, fstype, path, orig_sclass, sid); +} + /** * security_fs_use - Determine how to handle labeling for a filesystem. * @sb: superblock in question @@ -2806,8 +2826,8 @@ int security_fs_use(struct selinux_state *state, struct super_block *sb) } sbsec->sid = c->sid[0]; } else { - rc = __security_genfs_sid(state, fstype, "/", SECCLASS_DIR, - &sbsec->sid); + rc = __security_genfs_sid(state->ss->policy, fstype, "/", + SECCLASS_DIR, &sbsec->sid); if (rc) { sbsec->behavior = SECURITY_FS_USE_NONE; rc = 0; @@ -2821,23 +2841,14 @@ int security_fs_use(struct selinux_state *state, struct super_block *sb) return rc; } -int security_get_bools(struct selinux_state *state, +int security_get_bools(struct selinux_policy *policy, u32 *len, char ***names, int **values) { struct policydb *policydb; u32 i; int rc; - if (!selinux_initialized(state)) { - *len = 0; - *names = NULL; - *values = NULL; - return 0; - } - - read_lock(&state->ss->policy_rwlock); - - policydb = &state->ss->policy->policydb; + policydb = &policy->policydb; *names = NULL; *values = NULL; @@ -2868,7 +2879,6 @@ int security_get_bools(struct selinux_state *state, } rc = 0; out: - read_unlock(&state->ss->policy_rwlock); return rc; err: if (*names) { @@ -2957,7 +2967,9 @@ static int security_preserve_bools(struct selinux_state *state, struct cond_bool_datum *booldatum; u32 i, nbools = 0; - rc = security_get_bools(state, &nbools, &bnames, &bvalues); + read_lock(&state->ss->policy_rwlock); + rc = security_get_bools(state->ss->policy, &nbools, &bnames, &bvalues); + read_unlock(&state->ss->policy_rwlock); if (rc) goto out; for (i = 0; i < nbools; i++) { @@ -3168,21 +3180,13 @@ static int get_classes_callback(void *k, void *d, void *args) return 0; } -int security_get_classes(struct selinux_state *state, +int security_get_classes(struct selinux_policy *policy, char ***classes, int *nclasses) { struct policydb *policydb; int rc; - if (!selinux_initialized(state)) { - *nclasses = 0; - *classes = NULL; - return 0; - } - - read_lock(&state->ss->policy_rwlock); - - policydb = &state->ss->policy->policydb; + policydb = &policy->policydb; rc = -ENOMEM; *nclasses = policydb->p_classes.nprim; @@ -3200,7 +3204,6 @@ int security_get_classes(struct selinux_state *state, } out: - read_unlock(&state->ss->policy_rwlock); return rc; } @@ -3217,16 +3220,14 @@ static int get_permissions_callback(void *k, void *d, void *args) return 0; } -int security_get_permissions(struct selinux_state *state, +int security_get_permissions(struct selinux_policy *policy, char *class, char ***perms, int *nperms) { struct policydb *policydb; int rc, i; struct class_datum *match; - read_lock(&state->ss->policy_rwlock); - - policydb = &state->ss->policy->policydb; + policydb = &policy->policydb; rc = -EINVAL; match = symtab_search(&policydb->p_classes, class); @@ -3255,11 +3256,9 @@ int security_get_permissions(struct selinux_state *state, goto err; out: - read_unlock(&state->ss->policy_rwlock); return rc; err: - read_unlock(&state->ss->policy_rwlock); for (i = 0; i < *nperms; i++) kfree((*perms)[i]); kfree(*perms); diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c index 47be8bcf6bed..4a24ded670af 100644 --- a/security/selinux/ss/sidtab.c +++ b/security/selinux/ss/sidtab.c @@ -463,6 +463,16 @@ int sidtab_convert(struct sidtab *s, struct sidtab_convert_params *params) return 0; } +void sidtab_cancel_convert(struct sidtab *s) +{ + unsigned long flags; + + /* cancelling policy load - disable live convert of sidtab */ + spin_lock_irqsave(&s->lock, flags); + s->convert = NULL; + spin_unlock_irqrestore(&s->lock, flags); +} + static void sidtab_destroy_entry(struct sidtab_entry *entry) { context_destroy(&entry->context); diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h index f2a84560b8b3..80c744d07ad6 100644 --- a/security/selinux/ss/sidtab.h +++ b/security/selinux/ss/sidtab.h @@ -123,6 +123,8 @@ static inline struct context *sidtab_search_force(struct sidtab *s, u32 sid) int sidtab_convert(struct sidtab *s, struct sidtab_convert_params *params); +void sidtab_cancel_convert(struct sidtab *s); + int sidtab_context_to_sid(struct sidtab *s, struct context *context, u32 *sid); void sidtab_destroy(struct sidtab *s);
With the refactoring of the policy load logic in the security server from the previous change, it is now possible to split out the committing of the new policy from security_load_policy() and perform it only after successful updating of selinuxfs. Change security_load_policy() to return the newly populated policy data structures to the caller, export selinux_policy_commit() for external callers, and introduce selinux_policy_cancel() to provide a way to cancel the policy load in the event of an error during updating of the selinuxfs directory tree. Further, rework the interfaces used by selinuxfs to get information from the policy when creating the new directory tree to take and act upon the new policy data structure rather than the current/active policy. Update selinuxfs to use these updated and new interfaces. While we are here, stop re-creating the policy_capabilities directory on each policy load since it does not depend on the policy, and stop trying to create the booleans and classes directories during the initial creation of selinuxfs since no information is available until first policy load. After this change, a failure while updating the booleans and class directories will cause the entire policy load to be canceled, leaving the original policy intact, and policy load notifications to userspace will only happen after a successful completion of updating those directories. This does not (yet) provide full atomicity with respect to the updating of the directory trees themselves. Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> --- This patch is relative to my previous one, https://patchwork.kernel.org/patch/11698505/. Although this does not ensure atomicity when updating the selinuxfs directoty tree, I suspect it will solve Daniel's original bug because systemd/dbusd won't get the policy load notifications until the kernel is done updating selinuxfs and therefore won't try to re-read selinuxfs in the middle of it (because libselinux caches the class/perm mappings and only flushes on a reload). security/selinux/include/conditional.h | 2 +- security/selinux/include/security.h | 16 ++++- security/selinux/selinuxfs.c | 69 ++++++++++---------- security/selinux/ss/services.c | 87 +++++++++++++------------- security/selinux/ss/sidtab.c | 10 +++ security/selinux/ss/sidtab.h | 2 + 6 files changed, 106 insertions(+), 80 deletions(-)