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

[Fan Wu <wufan@linux.microsoft.com>]

On 2/5/2024 3:10 PM, Paul Moore wrote:
> On Mon, Feb 5, 2024 at 6:01 PM Fan Wu <wufan@linux.microsoft.com> wrote:
>> On 2/3/2024 2:25 PM, Paul Moore wrote:
>>> On Jan 30, 2024 Fan Wu <wufan@linux.microsoft.com> 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@linux.microsoft.com>
>>>> Signed-off-by: Fan Wu <wufan@linux.microsoft.com>
>>>> ---
>>>> 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