On 2/5/2024 3:10 PM, Paul Moore wrote:
On Mon, Feb 5, 2024 at 6:01 PM Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> wrote:
On 2/3/2024 2:25 PM, Paul Moore wrote:
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?
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.
+ 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?
The call of synchronize_rcu() is because we are updating the current
active policy so we need to set the new policy as active.
Unless I'm mistaken, a syncronize_rcu() call only ensures that the
current task will see the updated value by waiting until all current
RCU critical sections have finished. Given the mutex involved here I
don't believe this is necessary, but please correct me if I'm wrong.
Sorry for the confusion. I think your previous comment was right, the
call of synchronize_rcu() is to free the old one. And I should put the
free of old just after the synchronize_rcu() call.
Thanks,
Fan
