On 2/3/2024 2:25 PM, Paul Moore wrote:
I think we are safe here because root->i_rwsem is held. Other two operations set_active and delete are also depending on the inode lock.On Jan 30, 2024 Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> wrote:As is typical with LSMs, IPE uses securityfs as its interface with userspace. for a complete list of the interfaces and the respective inputs/outputs, please see the documentation under admin-guide/LSM/ipe.rst Signed-off-by: Deven Bowers <deven.desai@xxxxxxxxxxxxxxxxxxx> Signed-off-by: Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> --- v2: + Split evaluation loop, access control hooks, and evaluation loop from policy parser and userspace interface to pass mailing list character limit v3: + Move policy load and activation audit event to 03/12 + Fix a potential panic when a policy failed to load. + use pr_warn for a failure to parse instead of an audit record + Remove comments from headers + Add lockdep assertions to ipe_update_active_policy and ipe_activate_policy + Fix up warnings with checkpatch --strict + Use file_ns_capable for CAP_MAC_ADMIN for securityfs nodes. + Use memdup_user instead of kzalloc+simple_write_to_buffer. + Remove strict_parse command line parameter, as it is added by the sysctl command line. + Prefix extern variables with ipe_ v4: + Remove securityfs to reverse-dependency + Add SHA1 reverse dependency. + Add versioning scheme for IPE properties, and associated interface to query the versioning scheme. + Cause a parser to always return an error on unknown syntax. + Remove strict_parse option + Change active_policy interface from sysctl, to securityfs, and change scheme. v5: + Cause an error if a default action is not defined for each operation. + Minor function renames v6: + No changes v7: + Propagating changes to support the new ipe_context structure in the evaluation loop. + Further split the parser and userspace interface changes into separate commits. + "raw" was renamed to "pkcs7" and made read only + "raw"'s write functionality (update a policy) moved to "update" + introduced "version", "policy_name" nodes. + "content" renamed to "policy" + changes to allow the compiled-in policy to be treated identical to deployed-after-the-fact policies. v8: + Prevent securityfs initialization if the LSM is disabled v9: + Switch to securityfs_recursive_remove for policy folder deletion v10: + Simplify and correct concurrency + Fix typos v11: + Correct code comments v12: + Correct locking and remove redundant code --- security/ipe/Makefile | 2 + security/ipe/fs.c | 101 +++++++++ security/ipe/fs.h | 16 ++ security/ipe/ipe.c | 3 + security/ipe/ipe.h | 2 + security/ipe/policy.c | 123 ++++++++++ security/ipe/policy.h | 9 + security/ipe/policy_fs.c | 469 +++++++++++++++++++++++++++++++++++++++ 8 files changed, 725 insertions(+) create mode 100644 security/ipe/fs.c create mode 100644 security/ipe/fs.h create mode 100644 security/ipe/policy_fs.c...diff --git a/security/ipe/policy.c b/security/ipe/policy.c index f22a576a6d68..61fea3e38e11 100644 --- a/security/ipe/policy.c +++ b/security/ipe/policy.c @@ -43,6 +71,68 @@ static int set_pkcs7_data(void *ctx, const void *data, size_t len, return 0; }+/**+ * ipe_update_policy - parse a new policy and replace old with it. + * @root: Supplies a pointer to the securityfs inode saved the policy. + * @text: Supplies a pointer to the plain text policy. + * @textlen: Supplies the length of @text. + * @pkcs7: Supplies a pointer to a buffer containing a pkcs7 message. + * @pkcs7len: Supplies the length of @pkcs7len. + * + * @text/@textlen is mutually exclusive with @pkcs7/@pkcs7len - see + * ipe_new_policy. + * + * Context: Requires root->i_rwsem to be held. + * Return: + * * !IS_ERR - The existing policy saved in the inode before update + * * -ENOENT - Policy doesn't exist + * * -EINVAL - New policy is invalid + */ +struct ipe_policy *ipe_update_policy(struct inode *root, + const char *text, size_t textlen, + const char *pkcs7, size_t pkcs7len) +{ + int rc = 0; + struct ipe_policy *old, *ap, *new = NULL; + + old = (struct ipe_policy *)root->i_private; + if (!old) + return ERR_PTR(-ENOENT); + + new = ipe_new_policy(text, textlen, pkcs7, pkcs7len); + if (IS_ERR(new)) + return new; + + if (strcmp(new->parsed->name, old->parsed->name)) { + rc = -EINVAL; + goto err; + } + + if (ver_to_u64(old) > ver_to_u64(new)) { + rc = -EINVAL; + goto err; + } + + root->i_private = new; + swap(new->policyfs, old->policyfs);Should the swap() take place with @ipe_policy_lock held?
The call of synchronize_rcu() is because we are updating the current active policy so we need to set the new policy as active.+ mutex_lock(&ipe_policy_lock); + ap = rcu_dereference_protected(ipe_active_policy, + lockdep_is_held(&ipe_policy_lock)); + if (old == ap) { + rcu_assign_pointer(ipe_active_policy, new); + mutex_unlock(&ipe_policy_lock); + synchronize_rcu();I'm guessing you are forcing a synchronize_rcu() here because you are free()'ing @old in the caller, yes? Looking at the code, I only see one caller, update_policy(). With only one caller, why not free @old directly in ipe_update_policy()? Do you see others callers that would do something different?
I do agree we can free the old inside this function.
+ } else { + mutex_unlock(&ipe_policy_lock); + } + + return old; +err: + ipe_free_policy(new); + return ERR_PTR(rc); +} + /** * ipe_new_policy - Allocate and parse an ipe_policy structure. * @@ -99,3 +189,36 @@ struct ipe_policy *ipe_new_policy(const char *text, size_t textlen, ipe_free_policy(new); return ERR_PTR(rc); } + +/** + * ipe_set_active_pol - Make @p the active policy. + * @p: Supplies a pointer to the policy to make active. + * + * Context: Requires root->i_rwsem, which i_private has the policy, to be held. + * Return: + * * !IS_ERR - Success + * * -EINVAL - New active policy version is invalid + */ +int ipe_set_active_pol(const struct ipe_policy *p) +{ + struct ipe_policy *ap = NULL; + + mutex_lock(&ipe_policy_lock); + + ap = rcu_dereference_protected(ipe_active_policy, + lockdep_is_held(&ipe_policy_lock)); + if (ap == p) { + mutex_unlock(&ipe_policy_lock); + return 0; + } + if (ap && ver_to_u64(ap) > ver_to_u64(p)) { + mutex_unlock(&ipe_policy_lock); + return -EINVAL; + } + + rcu_assign_pointer(ipe_active_policy, p); + mutex_unlock(&ipe_policy_lock); + synchronize_rcu();Why do you need the synchronize_rcu() call here?+ return 0; +}-- paul-moore.com
|