Re: [PATCH RFC v12 8/20] ipe: add userspace interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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.

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




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux