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

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

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.

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