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.
--
paul-moore.com
