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?
> + 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?
> + } 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
