diff mbox series

[RFC,v9,05/16] ipe: add userspace interface

Message ID 1675119451-23180-6-git-send-email-wufan@linux.microsoft.com (mailing list archive)
State Changes Requested
Delegated to: Paul Moore
Headers show
Series Integrity Policy Enforcement LSM (IPE) | expand

Commit Message

Fan Wu Jan. 30, 2023, 10:57 p.m. UTC
From: Deven Bowers <deven.desai@linux.microsoft.com>

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
    operaiton.
  + Minor function renames

v6:
  + No changes

v7:
  + Propogating 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
---
 security/ipe/Makefile    |   2 +
 security/ipe/fs.c        | 101 +++++++++
 security/ipe/fs.h        |  17 ++
 security/ipe/ipe.c       |   3 +
 security/ipe/ipe.h       |   2 +
 security/ipe/policy.c    | 135 ++++++++++++
 security/ipe/policy.h    |   7 +
 security/ipe/policy_fs.c | 459 +++++++++++++++++++++++++++++++++++++++
 8 files changed, 726 insertions(+)
 create mode 100644 security/ipe/fs.c
 create mode 100644 security/ipe/fs.h
 create mode 100644 security/ipe/policy_fs.c

Comments

Roberto Sassu Jan. 31, 2023, 10:49 a.m. UTC | #1
On Mon, 2023-01-30 at 14:57 -0800, Fan Wu wrote:
> From: Deven Bowers <deven.desai@linux.microsoft.com>
> 
> 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
>     operaiton.
>   + Minor function renames
> 
> v6:
>   + No changes
> 
> v7:
>   + Propogating 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
> ---
>  security/ipe/Makefile    |   2 +
>  security/ipe/fs.c        | 101 +++++++++
>  security/ipe/fs.h        |  17 ++
>  security/ipe/ipe.c       |   3 +
>  security/ipe/ipe.h       |   2 +
>  security/ipe/policy.c    | 135 ++++++++++++
>  security/ipe/policy.h    |   7 +
>  security/ipe/policy_fs.c | 459 +++++++++++++++++++++++++++++++++++++++
>  8 files changed, 726 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/Makefile b/security/ipe/Makefile
> index d7f2870d7c09..8602d71250b4 100644
> --- a/security/ipe/Makefile
> +++ b/security/ipe/Makefile
> @@ -7,7 +7,9 @@
>  
>  obj-$(CONFIG_SECURITY_IPE) += \
>  	eval.o \
> +	fs.o \
>  	hooks.o \
>  	ipe.o \
>  	policy.o \
> +	policy_fs.o \
>  	policy_parser.o \
> diff --git a/security/ipe/fs.c b/security/ipe/fs.c
> new file mode 100644
> index 000000000000..9f6a4867bec2
> --- /dev/null
> +++ b/security/ipe/fs.c
> @@ -0,0 +1,101 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) Microsoft Corporation. All rights reserved.
> + */
> +#include "ipe.h"
> +#include "fs.h"
> +#include "policy.h"
> +
> +#include <linux/dcache.h>
> +#include <linux/security.h>
> +
> +static struct dentry *np __ro_after_init;
> +static struct dentry *root __ro_after_init;
> +struct dentry *policy_root __ro_after_init;
> +
> +/**
> + * new_policy - Write handler for the securityfs node, "ipe/new_policy".
> + * @f: Supplies a file structure representing the securityfs node.
> + * @data: Suppleis a buffer passed to the write syscall.

Typo: Suppleis.

> + * @len: Supplies the length of @data.
> + * @offset: unused.
> + *
> + * Return:
> + * * >0	- Success, Length of buffer written
> + * * <0	- Error
> + */
> +static ssize_t new_policy(struct file *f, const char __user *data,
> +			  size_t len, loff_t *offset)
> +{
> +	int rc = 0;
> +	char *copy = NULL;
> +	struct ipe_policy *p = NULL;
> +
> +	if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN))
> +		return -EPERM;
> +
> +	copy = memdup_user_nul(data, len);
> +	if (IS_ERR(copy)) {
> +		rc = PTR_ERR(copy);
> +		goto err;
> +	}
> +
> +	p = ipe_new_policy(NULL, 0, copy, len);
> +	if (IS_ERR(p)) {
> +		rc = PTR_ERR(p);
> +		goto err;
> +	}
> +
> +	rc = ipe_new_policyfs_node(p);
> +	if (rc)
> +		goto err;

Uhm, don't you need to do cleanup of allocated memory or revert the
actions of ipe_new_policy()?

> +
> +err:
> +	return (rc < 0) ? rc : len;
> +}
> +
> +static const struct file_operations np_fops = {
> +	.write = new_policy,
> +};
> +
> +/**
> + * ipe_init_securityfs - Initialize IPE's securityfs tree at fsinit.
> + *
> + * Return:
> + * * !0	- Error
> + * * 0	- OK
> + */
> +static int __init ipe_init_securityfs(void)
> +{
> +	int rc = 0;
> +
> +	if (!ipe_enabled)
> +		return -EOPNOTSUPP;
> +
> +	root = securityfs_create_dir("ipe", NULL);
> +	if (IS_ERR(root)) {
> +		rc = PTR_ERR(root);
> +		goto err;
> +	}
> +
> +	np = securityfs_create_file("new_policy", 0200, root, NULL, &np_fops);
> +	if (IS_ERR(np)) {
> +		rc = PTR_ERR(np);
> +		goto err;
> +	}
> +
> +	policy_root = securityfs_create_dir("policies", root);
> +	if (IS_ERR(policy_root)) {
> +		rc = PTR_ERR(policy_root);
> +		goto err;
> +	}
> +
> +	return 0;
> +err:
> +	securityfs_remove(np);
> +	securityfs_remove(root);
> +	securityfs_remove(policy_root);
> +	return rc;
> +}
> +
> +fs_initcall(ipe_init_securityfs);
> diff --git a/security/ipe/fs.h b/security/ipe/fs.h
> new file mode 100644
> index 000000000000..fa105d9d6fc5
> --- /dev/null
> +++ b/security/ipe/fs.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) Microsoft Corporation. All rights reserved.
> + */
> +
> +#ifndef IPE_FS_H
> +#define IPE_FS_H
> +
> +#include "policy.h"
> +
> +extern struct dentry *policy_root __ro_after_init;
> +
> +void ipe_soft_del_policyfs(struct ipe_policy *p);
> +int ipe_new_policyfs_node(struct ipe_policy *p);
> +void ipe_del_policyfs_node(struct ipe_policy *p);
> +
> +#endif /* IPE_FS_H */
> diff --git a/security/ipe/ipe.c b/security/ipe/ipe.c
> index 551c6d90ac11..bef923026b50 100644
> --- a/security/ipe/ipe.c
> +++ b/security/ipe/ipe.c
> @@ -5,6 +5,8 @@
>  
>  #include "ipe.h"
>  
> +bool ipe_enabled;
> +
>  static struct lsm_blob_sizes ipe_blobs __lsm_ro_after_init = {
>  };
>  
> @@ -30,6 +32,7 @@ static int __init ipe_init(void)
>  	int rc = 0;
>  
>  	security_add_hooks(ipe_hooks, ARRAY_SIZE(ipe_hooks), "ipe");
> +	ipe_enabled = true;
>  
>  	return rc;
>  }
> diff --git a/security/ipe/ipe.h b/security/ipe/ipe.h
> index ee7ec3f3b55d..43cc132ed048 100644
> --- a/security/ipe/ipe.h
> +++ b/security/ipe/ipe.h
> @@ -10,4 +10,6 @@
>  
>  #include <linux/lsm_hooks.h>
>  
> +extern bool ipe_enabled;
> +
>  #endif /* IPE_H */
> diff --git a/security/ipe/policy.c b/security/ipe/policy.c
> index 772d876b1087..a5e9c6e5691b 100644
> --- a/security/ipe/policy.c
> +++ b/security/ipe/policy.c
> @@ -4,12 +4,39 @@
>   */
>  
>  #include "ipe.h"
> +#include "eval.h"
> +#include "fs.h"
>  #include "policy.h"
>  #include "policy_parser.h"
>  #include "digest.h"
>  
>  #include <linux/verification.h>
>  
> +/* lock for synchronizing writers across ipe policy */
> +DEFINE_SPINLOCK(ipe_policy_lock);
> +
> +/**
> + * ver_to_u64 - Convert an internal ipe_policy_version to a u64.
> + * @p: Policy to extract the version from.
> + *
> + * Bits (LSB is index 0):
> + *	[48,32] -> Major
> + *	[32,16] -> Minor
> + *	[16, 0] -> Revision
> + *
> + * Return: u64 version of the embedded version structure.
> + */
> +static inline u64 ver_to_u64(const struct ipe_policy *const p)
> +{
> +	u64 r = 0;
> +
> +	r = (((u64)p->parsed->version.major) << 32)
> +	  | (((u64)p->parsed->version.minor) << 16)
> +	  | ((u64)(p->parsed->version.rev));
> +
> +	return r;
> +}
> +
>  /**
>   * ipe_free_policy - Deallocate a given IPE policy.
>   * @p: Supplies the policy to free.
> @@ -21,6 +48,7 @@ void ipe_free_policy(struct ipe_policy *p)
>  	if (IS_ERR_OR_NULL(p))
>  		return;
>  
> +	ipe_del_policyfs_node(p);
>  	free_parsed_policy(p->parsed);
>  	if (!p->pkcs7)
>  		kfree(p->text);
> @@ -39,6 +67,70 @@ 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.
> + * @addr: Supplies a pointer to the i_private for saving 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.
> + *
> + * Return:
> + * * !IS_ERR	- OK
> + * * -ENOENT	- Policy doesn't exist
> + * * -EINVAL	- New policy is invalid
> + */
> +struct ipe_policy *ipe_update_policy(struct ipe_policy __rcu **addr,
> +				     const char *text, size_t textlen,
> +				     const char *pkcs7, size_t pkcs7len)
> +{
> +	int rc = 0;
> +	struct ipe_policy *old, *new;
> +
> +	old = ipe_get_policy_rcu(*addr);
> +	if (!old) {
> +		rc = -ENOENT;
> +		goto err;
> +	}
> +
> +	new = ipe_new_policy(text, textlen, pkcs7, pkcs7len);
> +	if (IS_ERR(new)) {
> +		rc = PTR_ERR(new);
> +		goto err;
> +	}
> +
> +	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;
> +	}
> +
> +	if (ipe_is_policy_active(old)) {
> +		spin_lock(&ipe_policy_lock);
> +		rcu_assign_pointer(ipe_active_policy, new);
> +		spin_unlock(&ipe_policy_lock);
> +		synchronize_rcu();
> +	}
> +
> +	rcu_assign_pointer(*addr, new);
> +
> +	swap(new->policyfs, old->policyfs);
> +	ipe_free_policy(old);
> +
> +	goto out;
> +err:
> +	ipe_free_policy(new);
> +out:
> +	return (rc < 0) ? ERR_PTR(rc) : new;
> +}

I would like more to see all the functions managing the policy
together. If the patch is too long, you could further split by adding
the helpers (that don't directly deal with the policy) in a separate
patch.

Here you would simply instantiate dirs/files in securityfs and call the
existing functions previously introduced.

Roberto

> +
>  /**
>   * ipe_new_policy - Allocate and parse an ipe_policy structure.
>   *
> @@ -117,3 +209,46 @@ struct ipe_policy *ipe_get_policy_rcu(struct ipe_policy __rcu *p)
>  
>  	return rv;
>  }
> +
> +/**
> + * ipe_set_active_pol - Make @p the active policy.
> + * @p: Supplies a pointer to the policy to make active.
> + */
> +int ipe_set_active_pol(const struct ipe_policy *p)
> +{
> +	int rc = 0;
> +	struct ipe_policy *ap = NULL;
> +
> +	ap = ipe_get_policy_rcu(ipe_active_policy);
> +	if (ap && ver_to_u64(ap) > ver_to_u64(p)) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +
> +	spin_lock(&ipe_policy_lock);
> +	rcu_assign_pointer(ipe_active_policy, p);
> +	spin_unlock(&ipe_policy_lock);
> +	synchronize_rcu();
> +
> +out:
> +	return rc;
> +}
> +
> +/**
> + * ipe_is_policy_active - Determine wehther @p is the active policy.
> + * @p: Supplies a pointer to the policy to check.
> + *
> + * Return:
> + * * true	- @p is the active policy
> + * * false	- @p is not the active policy
> + */
> +bool ipe_is_policy_active(const struct ipe_policy *p)
> +{
> +	bool rv;
> +
> +	rcu_read_lock();
> +	rv = rcu_access_pointer(ipe_active_policy) == p;
> +	rcu_read_unlock();
> +
> +	return rv;
> +}
> diff --git a/security/ipe/policy.h b/security/ipe/policy.h
> index 967d816cd5cd..0cb42b6f246e 100644
> --- a/security/ipe/policy.h
> +++ b/security/ipe/policy.h
> @@ -70,11 +70,18 @@ struct ipe_policy {
>  	size_t		textlen;
>  
>  	struct ipe_parsed_policy *parsed;
> +
> +	struct dentry *policyfs;
>  };
>  
>  struct ipe_policy *ipe_new_policy(const char *text, size_t textlen,
>  				  const char *pkcs7, size_t pkcs7len);
>  void ipe_free_policy(struct ipe_policy *pol);
>  struct ipe_policy *ipe_get_policy_rcu(struct ipe_policy __rcu *p);
> +struct ipe_policy *ipe_update_policy(struct ipe_policy **addr, const char *text,
> +				     size_t textlen, const char *pkcs7,
> +				     size_t pkcs7len);
> +int ipe_set_active_pol(const struct ipe_policy *p);
> +bool ipe_is_policy_active(const struct ipe_policy *p);
>  
>  #endif /* IPE_POLICY_H */
> diff --git a/security/ipe/policy_fs.c b/security/ipe/policy_fs.c
> new file mode 100644
> index 000000000000..72759cc8938d
> --- /dev/null
> +++ b/security/ipe/policy_fs.c
> @@ -0,0 +1,459 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) Microsoft Corporation. All rights reserved.
> + */
> +#include "ipe.h"
> +#include "policy.h"
> +#include "fs.h"
> +
> +#include <linux/fs.h>
> +#include <linux/namei.h>
> +#include <linux/types.h>
> +#include <linux/dcache.h>
> +#include <linux/security.h>
> +
> +#define MAX_VERSION_SIZE ARRAY_SIZE("65535.65535.65535")
> +
> +/**
> + * find_policy - return a policy pointer saved in i_private of a dentry.
> + * @f: Securityfs object that contains a link to the dentry containing the
> + *     policy structure.
> + *
> + * Return: Always-Valid Address Pointer
> + */
> +static inline struct ipe_policy __rcu **find_policy(struct file *f)
> +{
> +	struct dentry *link;
> +
> +	link = d_inode(f->f_path.dentry)->i_private;
> +
> +	return (struct ipe_policy __rcu **)&(d_inode(link)->i_private);
> +}
> +
> +/**
> + * ipefs_file - defines a file in securityfs.
> + */
> +struct ipefs_file {
> +	const char	*name;
> +	umode_t		access;
> +	const struct	file_operations *fops;
> +};
> +
> +/**
> + * read_pkcs7 - Read handler for "ipe/policies/$name/pkcs7".
> + * @f: Supplies a file structure representing the securityfs node.
> + * @data: Suppleis a buffer passed to the write syscall.
> + * @len: Supplies the length of @data.
> + * @offset: unused.
> + *
> + * @data will be populated with the pkcs7 blob representing the policy
> + * on success. If the policy is unsigned (like the boot policy), this
> + * will return -ENOENT.
> + *
> + * Return:
> + * * >0	- Success, Length of buffer written
> + * * <0	- Error
> + */
> +static ssize_t read_pkcs7(struct file *f, char __user *data,
> +			  size_t len, loff_t *offset)
> +{
> +	int rc = 0;
> +	struct ipe_policy *p = NULL;
> +
> +	p = ipe_get_policy_rcu(*find_policy(f));
> +	if (!p)
> +		return -ENOENT;
> +
> +	if (!p->pkcs7) {
> +		rc = -ENOENT;
> +		goto out;
> +	}
> +
> +	rc = simple_read_from_buffer(data, len, offset, p->pkcs7, p->pkcs7len);
> +
> +out:
> +	return rc;
> +}
> +
> +/**
> + * read_policy - Read handler for "ipe/policies/$name/policy".
> + * @f: Supplies a file structure representing the securityfs node.
> + * @data: Suppleis a buffer passed to the write syscall.
> + * @len: Supplies the length of @data.
> + * @offset: unused.
> + *
> + * @data will be populated with the plain-text version of the policy
> + * on success.
> + *
> + * Return:
> + * * >0	- Success, Length of buffer written
> + * * <0	- Error
> + */
> +static ssize_t read_policy(struct file *f, char __user *data,
> +			   size_t len, loff_t *offset)
> +{
> +	int rc = 0;
> +	struct ipe_policy *p = NULL;
> +
> +	p = ipe_get_policy_rcu(*find_policy(f));
> +	if (!p)
> +		return -ENOENT;
> +
> +	rc = simple_read_from_buffer(data, len, offset, p->text, p->textlen);
> +
> +	return rc;
> +}
> +
> +/**
> + * read_name: Read handler for "ipe/policies/$name/name".
> + * @f: Supplies a file structure representing the securityfs node.
> + * @data: Suppleis a buffer passed to the write syscall.
> + * @len: Supplies the length of @data.
> + * @offset: unused.
> + *
> + * @data will be populated with the policy_name attribute on success.
> + *
> + * Return:
> + * * >0	- Success, Length of buffer written
> + * * <0	- Error
> + */
> +static ssize_t read_name(struct file *f, char __user *data,
> +			 size_t len, loff_t *offset)
> +{
> +	int rc = 0;
> +	struct ipe_policy *p = NULL;
> +
> +	p = ipe_get_policy_rcu(*find_policy(f));
> +	if (!p)
> +		return -ENOENT;
> +
> +	rc = simple_read_from_buffer(data, len, offset, p->parsed->name,
> +				     strlen(p->parsed->name));
> +
> +	return rc;
> +}
> +
> +/**
> + * read_version - Read handler for "ipe/policies/$name/version".
> + * @f: Supplies a file structure representing the securityfs node.
> + * @data: Suppleis a buffer passed to the write syscall.
> + * @len: Supplies the length of @data.
> + * @offset: unused.
> + *
> + * @data will be populated with the version string on success.
> + *
> + * Return:
> + * * >0	- Success, Length of buffer written
> + * * <0	- Error
> + */
> +static ssize_t read_version(struct file *f, char __user *data,
> +			    size_t len, loff_t *offset)
> +{
> +	ssize_t rc = 0;
> +	size_t bufsize = 0;
> +	struct ipe_policy *p = NULL;
> +	char buffer[MAX_VERSION_SIZE] = { 0 };
> +
> +	p = ipe_get_policy_rcu(*find_policy(f));
> +	if (!p)
> +		return -ENOENT;
> +
> +	bufsize = scnprintf(buffer, ARRAY_SIZE(buffer), "%hu.%hu.%hu",
> +			    p->parsed->version.major, p->parsed->version.minor,
> +			    p->parsed->version.rev);
> +
> +	rc = simple_read_from_buffer(data, len, offset, buffer, bufsize);
> +
> +	return rc;
> +}
> +
> +/**
> + * setactive - Write handler for "ipe/policies/$name/active".
> + * @f: Supplies a file structure representing the securityfs node.
> + * @data: Supplies a buffer passed to the write syscall.
> + * @len: Supplies the length of @data.
> + * @offset: unused.
> + *
> + * Return:
> + * * >0	- Success, Length of buffer written
> + * * <0	- Error
> + */
> +static ssize_t setactive(struct file *f, const char __user *data,
> +			 size_t len, loff_t *offset)
> +{
> +	int rc = 0;
> +	bool value = false;
> +	struct ipe_policy *p = NULL;
> +
> +	if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN))
> +		return -EPERM;
> +
> +	rc = kstrtobool_from_user(data, len, &value);
> +	if (rc)
> +		goto out;
> +
> +	if (!value) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +
> +	p = ipe_get_policy_rcu(*find_policy(f));
> +	if (!p) {
> +		rc = -ENOENT;
> +		goto out;
> +	}
> +
> +	rc = ipe_set_active_pol(p);
> +
> +out:
> +	return (rc < 0) ? rc : len;
> +}
> +
> +/**
> + * getactive - Read handler for "ipe/policies/$name/active".
> + * @f: Supplies a file structure representing the securityfs node.
> + * @data: Suppleis a buffer passed to the write syscall.
> + * @len: Supplies the length of @data.
> + * @offset: unused.
> + *
> + * @data will be populated with the 1 or 0 depending on if the
> + * corresponding policy is active.
> + *
> + * Return:
> + * * >0	- Success, Length of buffer written
> + * * <0	- Error
> + */
> +static ssize_t getactive(struct file *f, char __user *data,
> +			 size_t len, loff_t *offset)
> +{
> +	int rc = 0;
> +	const char *str;
> +	struct ipe_policy *p = NULL;
> +
> +	p = ipe_get_policy_rcu(*find_policy(f));
> +	if (!p) {
> +		rc = -ENOENT;
> +		goto out;
> +	}
> +
> +	str = ipe_is_policy_active(p) ? "1" : "0";
> +	rc = simple_read_from_buffer(data, len, offset, str, 1);
> +
> +out:
> +	return rc;
> +}
> +
> +/**
> + * update_policy - Write handler for "ipe/policies/$name/update".
> + * @f: Supplies a file structure representing the securityfs node.
> + * @data: Supplies a buffer passed to the write syscall.
> + * @len: Supplies the length of @data.
> + * @offset: unused.
> + *
> + * On success this updates the policy represented by $name,
> + * in-place.
> + *
> + * Return:
> + * * >0	- Success, Length of buffer written
> + * * <0	- Error
> + */
> +static ssize_t update_policy(struct file *f, const char __user *data,
> +			     size_t len, loff_t *offset)
> +{
> +	int rc = 0;
> +	char *copy = NULL;
> +	struct inode *ino = NULL;
> +	struct ipe_policy *new = NULL;
> +	struct ipe_policy __rcu **addr = NULL;
> +
> +	if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN))
> +		return -EPERM;
> +
> +	copy = memdup_user(data, len);
> +	if (IS_ERR(copy)) {
> +		rc = PTR_ERR(copy);
> +		goto err;
> +	}
> +
> +	ino = d_inode(f->f_path.dentry->d_parent);
> +	inode_lock(ino);
> +	addr = find_policy(f);
> +	new = ipe_update_policy(addr, NULL, 0, copy, len);
> +	inode_unlock(ino);
> +	synchronize_rcu();
> +	if (IS_ERR(new)) {
> +		rc = PTR_ERR(new);
> +		goto err;
> +	}
> +
> +	kfree(copy);
> +	return len;
> +err:
> +	kfree(copy);
> +	return rc;
> +}
> +
> +/**
> + * delete_policy - write handler for  "ipe/policies/$name/delete".
> + * @f: Supplies a file structure representing the securityfs node.
> + * @data: Supplies a buffer passed to the write syscall.
> + * @len: Supplies the length of @data.
> + * @offset: unused.
> + *
> + * On success this deletes the policy represented by $name.
> + *
> + * Return:
> + * * >0	- Success, Length of buffer written
> + * * <0	- Error
> + */
> +static ssize_t delete_policy(struct file *f, const char __user *data,
> +			     size_t len, loff_t *offset)
> +{
> +	int rc = 0;
> +	bool value = false;
> +	struct ipe_policy *p = NULL;
> +
> +	if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN))
> +		return -EPERM;
> +
> +	rc = kstrtobool_from_user(data, len, &value);
> +	if (rc)
> +		goto out;
> +
> +	if (!value) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +
> +	p = ipe_get_policy_rcu(*find_policy(f));
> +	if (!p) {
> +		rc = -ENOENT;
> +		goto out;
> +	}
> +
> +	if (ipe_is_policy_active(p)) {
> +		rc = -EPERM;
> +		goto out;
> +	}
> +
> +	ipe_free_policy(p);
> +out:
> +	return (rc < 0) ? rc : len;
> +}
> +
> +static const struct file_operations content_fops = {
> +	.read = read_policy,
> +};
> +
> +static const struct file_operations pkcs7_fops = {
> +	.read = read_pkcs7,
> +};
> +
> +static const struct file_operations name_fops = {
> +	.read = read_name,
> +};
> +
> +static const struct file_operations ver_fops = {
> +	.read = read_version,
> +};
> +
> +static const struct file_operations active_fops = {
> +	.write = setactive,
> +	.read = getactive,
> +};
> +
> +static const struct file_operations update_fops = {
> +	.write = update_policy,
> +};
> +
> +static const struct file_operations delete_fops = {
> +	.write = delete_policy,
> +};
> +
> +/**
> + * policy_subdir - files under a policy subdirectory
> + */
> +static const struct ipefs_file policy_subdir[] = {
> +	{ "pkcs7", 0444, &pkcs7_fops },
> +	{ "policy", 0444, &content_fops },
> +	{ "name", 0444, &name_fops },
> +	{ "version", 0444, &ver_fops },
> +	{ "active", 0600, &active_fops },
> +	{ "update", 0200, &update_fops },
> +	{ "delete", 0200, &delete_fops },
> +};
> +
> +/**
> + * soft_del_policyfs - soft delete a policyfs node.
> + * @p: Supplies a ipe_policy associated with the node to delete.
> + *
> + * This deletes the i_private field of a policyfs node.
> + */
> +static void soft_del_policyfs(struct ipe_policy *p)
> +{
> +	struct inode *ino = NULL;
> +	struct ipe_policy __rcu **addr = NULL;
> +
> +	ino = d_inode(p->policyfs);
> +	addr = (struct ipe_policy __rcu **)&ino->i_private;
> +
> +	inode_lock(ino);
> +	rcu_assign_pointer(*addr, NULL);
> +	inode_unlock(ino);
> +	synchronize_rcu();
> +}
> +
> +/**
> + * ipe_del_policyfs_node - Delete a securityfs entry for @p.
> + * @p: Supplies a pointer to the policy to delete a securityfs entry for.
> + */
> +void ipe_del_policyfs_node(struct ipe_policy *p)
> +{
> +	if (IS_ERR_OR_NULL(p->policyfs))
> +		return;
> +
> +	soft_del_policyfs(p);
> +	securityfs_recursive_remove(p->policyfs);
> +}
> +
> +/**
> + * ipe_new_policyfs_node - Create a securityfs entry for @p.
> + * @p: Supplies a pointer to the policy to create a securityfs entry for.
> + *
> + * Return:
> + * * 0	- OK
> + * * !0	- Error
> + */
> +int ipe_new_policyfs_node(struct ipe_policy *p)
> +{
> +	int rc = 0;
> +	size_t i = 0;
> +	struct dentry *d = NULL;
> +	struct ipe_policy **addr = NULL;
> +	const struct ipefs_file *f = NULL;
> +
> +	p->policyfs = securityfs_create_dir(p->parsed->name, policy_root);
> +	if (IS_ERR(p->policyfs)) {
> +		rc = PTR_ERR(p->policyfs);
> +		goto err;
> +	}
> +
> +	addr = (struct ipe_policy **)&(d_inode(p->policyfs)->i_private);
> +	*addr = p;
> +
> +	for (i = 0; i < ARRAY_SIZE(policy_subdir); ++i) {
> +		f = &policy_subdir[i];
> +
> +		d = securityfs_create_file(f->name, f->access, p->policyfs, p->policyfs,
> +					   f->fops);
> +		if (IS_ERR(d)) {
> +			rc = PTR_ERR(d);
> +			goto err;
> +		}
> +	}
> +
> +	return 0;
> +err:
> +	ipe_del_policyfs_node(p);
> +	return rc;
> +}
Fan Wu Feb. 1, 2023, 7:46 p.m. UTC | #2
On Tue, Jan 31, 2023 at 11:49:44AM +0100, Roberto Sassu wrote:
> On Mon, 2023-01-30 at 14:57 -0800, Fan Wu wrote:
> > From: Deven Bowers <deven.desai@linux.microsoft.com>
> > +
> > +/**
> > + * new_policy - Write handler for the securityfs node, "ipe/new_policy".
> > + * @f: Supplies a file structure representing the securityfs node.
> > + * @data: Suppleis a buffer passed to the write syscall.
> 
> Typo: Suppleis.
> 
Thanks for spotting the typos!

> > + * @len: Supplies the length of @data.
> > + * @offset: unused.
> > + *
> > + * Return:
> > + * * >0	- Success, Length of buffer written
> > + * * <0	- Error
> > + */
> > +static ssize_t new_policy(struct file *f, const char __user *data,
> > +			  size_t len, loff_t *offset)
> > +{
> > +	int rc = 0;
> > +	char *copy = NULL;
> > +	struct ipe_policy *p = NULL;
> > +
> > +	if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN))
> > +		return -EPERM;
> > +
> > +	copy = memdup_user_nul(data, len);
> > +	if (IS_ERR(copy)) {
> > +		rc = PTR_ERR(copy);
> > +		goto err;
> > +	}
> > +
> > +	p = ipe_new_policy(NULL, 0, copy, len);
> > +	if (IS_ERR(p)) {
> > +		rc = PTR_ERR(p);
> > +		goto err;
> > +	}
> > +
> > +	rc = ipe_new_policyfs_node(p);
> > +	if (rc)
> > +		goto err;
> 
> Uhm, don't you need to do cleanup of allocated memory or revert the
> actions of ipe_new_policy()?
> 
Yes that should be cleaned up but should be done in ipe_new_policy instead,
will add a ipe_free_policy call at the end. Thanks for pointing that out.

> 
> I would like more to see all the functions managing the policy
> together. If the patch is too long, you could further split by adding
> the helpers (that don't directly deal with the policy) in a separate
> patch.
> 
> Here you would simply instantiate dirs/files in securityfs and call the
> existing functions previously introduced.
> 
> Roberto
> 

I will try to split them in the next version. Thanks for the suggestion.
-Fan
Paul Moore March 2, 2023, 7:04 p.m. UTC | #3
On Mon, Jan 30, 2023 at 5:58 PM Fan Wu <wufan@linux.microsoft.com> wrote:
>
> From: Deven Bowers <deven.desai@linux.microsoft.com>
>
> 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>

...

> ---
>  security/ipe/Makefile    |   2 +
>  security/ipe/fs.c        | 101 +++++++++
>  security/ipe/fs.h        |  17 ++
>  security/ipe/ipe.c       |   3 +
>  security/ipe/ipe.h       |   2 +
>  security/ipe/policy.c    | 135 ++++++++++++
>  security/ipe/policy.h    |   7 +
>  security/ipe/policy_fs.c | 459 +++++++++++++++++++++++++++++++++++++++
>  8 files changed, 726 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 772d876b1087..a5e9c6e5691b 100644
> --- a/security/ipe/policy.c
> +++ b/security/ipe/policy.c
> @@ -4,12 +4,39 @@
>   */
>
>  #include "ipe.h"
> +#include "eval.h"
> +#include "fs.h"
>  #include "policy.h"
>  #include "policy_parser.h"
>  #include "digest.h"
>
>  #include <linux/verification.h>
>
> +/* lock for synchronizing writers across ipe policy */
> +DEFINE_SPINLOCK(ipe_policy_lock);
> +
> +/**
> + * ver_to_u64 - Convert an internal ipe_policy_version to a u64.
> + * @p: Policy to extract the version from.
> + *
> + * Bits (LSB is index 0):
> + *     [48,32] -> Major
> + *     [32,16] -> Minor
> + *     [16, 0] -> Revision
> + *
> + * Return: u64 version of the embedded version structure.
> + */
> +static inline u64 ver_to_u64(const struct ipe_policy *const p)
> +{
> +       u64 r = 0;

No need to set @r to 0 since you set it to the version immediately below.

> +       r = (((u64)p->parsed->version.major) << 32)
> +         | (((u64)p->parsed->version.minor) << 16)
> +         | ((u64)(p->parsed->version.rev));
> +
> +       return r;
> +}
> +
>  /**
>   * ipe_free_policy - Deallocate a given IPE policy.
>   * @p: Supplies the policy to free.
> @@ -21,6 +48,7 @@ void ipe_free_policy(struct ipe_policy *p)
>         if (IS_ERR_OR_NULL(p))
>                 return;
>
> +       ipe_del_policyfs_node(p);
>         free_parsed_policy(p->parsed);
>         if (!p->pkcs7)
>                 kfree(p->text);
> @@ -39,6 +67,70 @@ 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.
> + * @addr: Supplies a pointer to the i_private for saving 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.
> + *
> + * Return:
> + * * !IS_ERR   - OK
> + * * -ENOENT   - Policy doesn't exist
> + * * -EINVAL   - New policy is invalid
> + */
> +struct ipe_policy *ipe_update_policy(struct ipe_policy __rcu **addr,
> +                                    const char *text, size_t textlen,
> +                                    const char *pkcs7, size_t pkcs7len)
> +{
> +       int rc = 0;
> +       struct ipe_policy *old, *new;
> +
> +       old = ipe_get_policy_rcu(*addr);
> +       if (!old) {
> +               rc = -ENOENT;
> +               goto err;
> +       }
> +
> +       new = ipe_new_policy(text, textlen, pkcs7, pkcs7len);
> +       if (IS_ERR(new)) {
> +               rc = PTR_ERR(new);
> +               goto err;
> +       }
> +
> +       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;
> +       }
> +
> +       if (ipe_is_policy_active(old)) {

I don't understand the is-active check, you want to make @new the new
active policy regardless, right?  Could this is-active check ever be
false?

> +               spin_lock(&ipe_policy_lock);
> +               rcu_assign_pointer(ipe_active_policy, new);
> +               spin_unlock(&ipe_policy_lock);
> +               synchronize_rcu();
> +       }
> +
> +       rcu_assign_pointer(*addr, new);
> +
> +       swap(new->policyfs, old->policyfs);
> +       ipe_free_policy(old);
> +
> +       goto out;
> +err:
> +       ipe_free_policy(new);
> +out:
> +       return (rc < 0) ? ERR_PTR(rc) : new;
> +}
> +
>  /**
>   * ipe_new_policy - Allocate and parse an ipe_policy structure.
>   *
> @@ -117,3 +209,46 @@ struct ipe_policy *ipe_get_policy_rcu(struct ipe_policy __rcu *p)
>
>         return rv;
>  }
> +
> +/**
> + * ipe_set_active_pol - Make @p the active policy.
> + * @p: Supplies a pointer to the policy to make active.
> + */
> +int ipe_set_active_pol(const struct ipe_policy *p)
> +{
> +       int rc = 0;
> +       struct ipe_policy *ap = NULL;
> +
> +       ap = ipe_get_policy_rcu(ipe_active_policy);
> +       if (ap && ver_to_u64(ap) > ver_to_u64(p)) {
> +               rc = -EINVAL;
> +               goto out;
> +       }
> +
> +       spin_lock(&ipe_policy_lock);
> +       rcu_assign_pointer(ipe_active_policy, p);
> +       spin_unlock(&ipe_policy_lock);
> +       synchronize_rcu();
> +
> +out:
> +       return rc;
> +}
> +
> +/**
> + * ipe_is_policy_active - Determine wehther @p is the active policy.
> + * @p: Supplies a pointer to the policy to check.
> + *
> + * Return:
> + * * true      - @p is the active policy
> + * * false     - @p is not the active policy
> + */
> +bool ipe_is_policy_active(const struct ipe_policy *p)
> +{
> +       bool rv;
> +
> +       rcu_read_lock();
> +       rv = rcu_access_pointer(ipe_active_policy) == p;
> +       rcu_read_unlock();
> +
> +       return rv;
> +}

--
paul-moore.com
Fan Wu April 10, 2023, 7:10 p.m. UTC | #4
On Thu, Mar 02, 2023 at 02:04:42PM -0500, Paul Moore wrote:
> On Mon, Jan 30, 2023 at 5:58???PM Fan Wu <wufan@linux.microsoft.com> wrote:
> >
> > From: Deven Bowers <deven.desai@linux.microsoft.com>
> >
> > 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>
> 
> ...
> 
> > ---
> >  security/ipe/Makefile    |   2 +
> >  security/ipe/fs.c        | 101 +++++++++
> >  security/ipe/fs.h        |  17 ++
> >  security/ipe/ipe.c       |   3 +
> >  security/ipe/ipe.h       |   2 +
> >  security/ipe/policy.c    | 135 ++++++++++++
> >  security/ipe/policy.h    |   7 +
> >  security/ipe/policy_fs.c | 459 +++++++++++++++++++++++++++++++++++++++
> >  8 files changed, 726 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 772d876b1087..a5e9c6e5691b 100644
> > --- a/security/ipe/policy.c
> > +++ b/security/ipe/policy.c
> > @@ -4,12 +4,39 @@
> >   */
> >
> >  #include "ipe.h"
> > +#include "eval.h"
> > +#include "fs.h"
> >  #include "policy.h"
> >  #include "policy_parser.h"
> >  #include "digest.h"
> >
> >  #include <linux/verification.h>
> >
> > +/* lock for synchronizing writers across ipe policy */
> > +DEFINE_SPINLOCK(ipe_policy_lock);
> > +
> > +/**
> > + * ver_to_u64 - Convert an internal ipe_policy_version to a u64.
> > + * @p: Policy to extract the version from.
> > + *
> > + * Bits (LSB is index 0):
> > + *     [48,32] -> Major
> > + *     [32,16] -> Minor
> > + *     [16, 0] -> Revision
> > + *
> > + * Return: u64 version of the embedded version structure.
> > + */
> > +static inline u64 ver_to_u64(const struct ipe_policy *const p)
> > +{
> > +       u64 r = 0;
> 
> No need to set @r to 0 since you set it to the version immediately below.
> 

Yes this is redundant, I will remove it.

> > +       r = (((u64)p->parsed->version.major) << 32)
> > +         | (((u64)p->parsed->version.minor) << 16)
> > +         | ((u64)(p->parsed->version.rev));
> > +
> > +       return r;
> > +}
> > +
> >  /**
> >   * ipe_free_policy - Deallocate a given IPE policy.
> >   * @p: Supplies the policy to free.
> > @@ -21,6 +48,7 @@ void ipe_free_policy(struct ipe_policy *p)
> >         if (IS_ERR_OR_NULL(p))
> >                 return;
> >
> > +       ipe_del_policyfs_node(p);
> >         free_parsed_policy(p->parsed);
> >         if (!p->pkcs7)
> >                 kfree(p->text);
> > @@ -39,6 +67,70 @@ 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.
> > + * @addr: Supplies a pointer to the i_private for saving 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.
> > + *
> > + * Return:
> > + * * !IS_ERR   - OK
> > + * * -ENOENT   - Policy doesn't exist
> > + * * -EINVAL   - New policy is invalid
> > + */
> > +struct ipe_policy *ipe_update_policy(struct ipe_policy __rcu **addr,
> > +                                    const char *text, size_t textlen,
> > +                                    const char *pkcs7, size_t pkcs7len)
> > +{
> > +       int rc = 0;
> > +       struct ipe_policy *old, *new;
> > +
> > +       old = ipe_get_policy_rcu(*addr);
> > +       if (!old) {
> > +               rc = -ENOENT;
> > +               goto err;
> > +       }
> > +
> > +       new = ipe_new_policy(text, textlen, pkcs7, pkcs7len);
> > +       if (IS_ERR(new)) {
> > +               rc = PTR_ERR(new);
> > +               goto err;
> > +       }
> > +
> > +       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;
> > +       }
> > +
> > +       if (ipe_is_policy_active(old)) {
> 
> I don't understand the is-active check, you want to make @new the new
> active policy regardless, right?  Could this is-active check ever be
> false?
> 

Actually this is needed. Policy updates can be applied to any deployed
policy, which may be saved in two places: the securityfs file node
and the ipe_active_policy pointer. To update a policy, this function first
checks if the policy saved in the securityfs file node is currently active.
If so, it updates the ipe_active_policy pointer to point to the new policy,
and finally updates the policy pointer in the securityfs to the new policy.

-Fan

> > +               spin_lock(&ipe_policy_lock);
> > +               rcu_assign_pointer(ipe_active_policy, new);
> > +               spin_unlock(&ipe_policy_lock);
> > +               synchronize_rcu();
> > +       }
> > +
> > +       rcu_assign_pointer(*addr, new);
> > +
> > +       swap(new->policyfs, old->policyfs);
> > +       ipe_free_policy(old);
> > +
> > +       goto out;
> > +err:
> > +       ipe_free_policy(new);
> > +out:
> > +       return (rc < 0) ? ERR_PTR(rc) : new;
> > +}
> > +
> >  /**
> >   * ipe_new_policy - Allocate and parse an ipe_policy structure.
> >   *
> > @@ -117,3 +209,46 @@ struct ipe_policy *ipe_get_policy_rcu(struct ipe_policy __rcu *p)
> >
> >         return rv;
> >  }
> > +
> > +/**
> > + * ipe_set_active_pol - Make @p the active policy.
> > + * @p: Supplies a pointer to the policy to make active.
> > + */
> > +int ipe_set_active_pol(const struct ipe_policy *p)
> > +{
> > +       int rc = 0;
> > +       struct ipe_policy *ap = NULL;
> > +
> > +       ap = ipe_get_policy_rcu(ipe_active_policy);
> > +       if (ap && ver_to_u64(ap) > ver_to_u64(p)) {
> > +               rc = -EINVAL;
> > +               goto out;
> > +       }
> > +
> > +       spin_lock(&ipe_policy_lock);
> > +       rcu_assign_pointer(ipe_active_policy, p);
> > +       spin_unlock(&ipe_policy_lock);
> > +       synchronize_rcu();
> > +
> > +out:
> > +       return rc;
> > +}
> > +
> > +/**
> > + * ipe_is_policy_active - Determine wehther @p is the active policy.
> > + * @p: Supplies a pointer to the policy to check.
> > + *
> > + * Return:
> > + * * true      - @p is the active policy
> > + * * false     - @p is not the active policy
> > + */
> > +bool ipe_is_policy_active(const struct ipe_policy *p)
> > +{
> > +       bool rv;
> > +
> > +       rcu_read_lock();
> > +       rv = rcu_access_pointer(ipe_active_policy) == p;
> > +       rcu_read_unlock();
> > +
> > +       return rv;
> > +}
> 
> --
> paul-moore.com
Paul Moore April 11, 2023, 9:45 p.m. UTC | #5
On Mon, Apr 10, 2023 at 3:10 PM Fan Wu <wufan@linux.microsoft.com> wrote:
> On Thu, Mar 02, 2023 at 02:04:42PM -0500, Paul Moore wrote:
> > On Mon, Jan 30, 2023 at 5:58???PM Fan Wu <wufan@linux.microsoft.com> wrote:
> > >
> > > From: Deven Bowers <deven.desai@linux.microsoft.com>
> > >
> > > 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>
> >
> > ...
> >
> > > ---
> > >  security/ipe/Makefile    |   2 +
> > >  security/ipe/fs.c        | 101 +++++++++
> > >  security/ipe/fs.h        |  17 ++
> > >  security/ipe/ipe.c       |   3 +
> > >  security/ipe/ipe.h       |   2 +
> > >  security/ipe/policy.c    | 135 ++++++++++++
> > >  security/ipe/policy.h    |   7 +
> > >  security/ipe/policy_fs.c | 459 +++++++++++++++++++++++++++++++++++++++
> > >  8 files changed, 726 insertions(+)
> > >  create mode 100644 security/ipe/fs.c
> > >  create mode 100644 security/ipe/fs.h
> > >  create mode 100644 security/ipe/policy_fs.c

...

> > > +/**
> > > + * ipe_update_policy - parse a new policy and replace @old with it.
> > > + * @addr: Supplies a pointer to the i_private for saving 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.
> > > + *
> > > + * Return:
> > > + * * !IS_ERR   - OK
> > > + * * -ENOENT   - Policy doesn't exist
> > > + * * -EINVAL   - New policy is invalid
> > > + */
> > > +struct ipe_policy *ipe_update_policy(struct ipe_policy __rcu **addr,
> > > +                                    const char *text, size_t textlen,
> > > +                                    const char *pkcs7, size_t pkcs7len)
> > > +{
> > > +       int rc = 0;
> > > +       struct ipe_policy *old, *new;
> > > +
> > > +       old = ipe_get_policy_rcu(*addr);
> > > +       if (!old) {
> > > +               rc = -ENOENT;
> > > +               goto err;
> > > +       }
> > > +
> > > +       new = ipe_new_policy(text, textlen, pkcs7, pkcs7len);
> > > +       if (IS_ERR(new)) {
> > > +               rc = PTR_ERR(new);
> > > +               goto err;
> > > +       }
> > > +
> > > +       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;
> > > +       }
> > > +
> > > +       if (ipe_is_policy_active(old)) {
> >
> > I don't understand the is-active check, you want to make @new the new
> > active policy regardless, right?  Could this is-active check ever be
> > false?
>
> Actually this is needed. Policy updates can be applied to any deployed
> policy, which may be saved in two places: the securityfs file node
> and the ipe_active_policy pointer. To update a policy, this function first
> checks if the policy saved in the securityfs file node is currently active.
> If so, it updates the ipe_active_policy pointer to point to the new policy,
> and finally updates the policy pointer in the securityfs to the new policy.

Ah, okay.  I must have forgotten, or not realized, that multiple
policies could be loaded and not active.

I guess this does make me wonder about keeping a non-active policy
loaded in the kernel, what purpose does that serve?
Fan Wu April 12, 2023, 11:36 p.m. UTC | #6
On Tue, Apr 11, 2023 at 05:45:41PM -0400, Paul Moore wrote:
> On Mon, Apr 10, 2023 at 3:10???PM Fan Wu <wufan@linux.microsoft.com> wrote:
> > On Thu, Mar 02, 2023 at 02:04:42PM -0500, Paul Moore wrote:
> > > On Mon, Jan 30, 2023 at 5:58???PM Fan Wu <wufan@linux.microsoft.com> wrote:
> > > >
> > > > From: Deven Bowers <deven.desai@linux.microsoft.com>
> > > >
> > > > 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>
> > >
> > > ...
> > >
> > > > ---
> > > >  security/ipe/Makefile    |   2 +
> > > >  security/ipe/fs.c        | 101 +++++++++
> > > >  security/ipe/fs.h        |  17 ++
> > > >  security/ipe/ipe.c       |   3 +
> > > >  security/ipe/ipe.h       |   2 +
> > > >  security/ipe/policy.c    | 135 ++++++++++++
> > > >  security/ipe/policy.h    |   7 +
> > > >  security/ipe/policy_fs.c | 459 +++++++++++++++++++++++++++++++++++++++
> > > >  8 files changed, 726 insertions(+)
> > > >  create mode 100644 security/ipe/fs.c
> > > >  create mode 100644 security/ipe/fs.h
> > > >  create mode 100644 security/ipe/policy_fs.c
> 
> ...
> 
> > > > +/**
> > > > + * ipe_update_policy - parse a new policy and replace @old with it.
> > > > + * @addr: Supplies a pointer to the i_private for saving 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.
> > > > + *
> > > > + * Return:
> > > > + * * !IS_ERR   - OK
> > > > + * * -ENOENT   - Policy doesn't exist
> > > > + * * -EINVAL   - New policy is invalid
> > > > + */
> > > > +struct ipe_policy *ipe_update_policy(struct ipe_policy __rcu **addr,
> > > > +                                    const char *text, size_t textlen,
> > > > +                                    const char *pkcs7, size_t pkcs7len)
> > > > +{
> > > > +       int rc = 0;
> > > > +       struct ipe_policy *old, *new;
> > > > +
> > > > +       old = ipe_get_policy_rcu(*addr);
> > > > +       if (!old) {
> > > > +               rc = -ENOENT;
> > > > +               goto err;
> > > > +       }
> > > > +
> > > > +       new = ipe_new_policy(text, textlen, pkcs7, pkcs7len);
> > > > +       if (IS_ERR(new)) {
> > > > +               rc = PTR_ERR(new);
> > > > +               goto err;
> > > > +       }
> > > > +
> > > > +       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;
> > > > +       }
> > > > +
> > > > +       if (ipe_is_policy_active(old)) {
> > >
> > > I don't understand the is-active check, you want to make @new the new
> > > active policy regardless, right?  Could this is-active check ever be
> > > false?
> >
> > Actually this is needed. Policy updates can be applied to any deployed
> > policy, which may be saved in two places: the securityfs file node
> > and the ipe_active_policy pointer. To update a policy, this function first
> > checks if the policy saved in the securityfs file node is currently active.
> > If so, it updates the ipe_active_policy pointer to point to the new policy,
> > and finally updates the policy pointer in the securityfs to the new policy.
> 
> Ah, okay.  I must have forgotten, or not realized, that multiple
> policies could be loaded and not active.
> 
> I guess this does make me wonder about keeping a non-active policy
> loaded in the kernel, what purpose does that serve?
> 

The non-active policy doesn't serve anything unless it is activated. User can
even delete a policy if that is no longer needed. Non-active is just the default
state when a new policy is loaded.

If IPE supports namespace, there is another use case where different containers
can select different policies as the active policy from among multiple loaded
policies. Deven has presented a demo of this during LSS 2021. But this goes
beyond the scope of this version.

-Fan

> -- 
> paul-moore.com
Paul Moore April 13, 2023, 6:45 p.m. UTC | #7
On Wed, Apr 12, 2023 at 7:36 PM Fan Wu <wufan@linux.microsoft.com> wrote:
> On Tue, Apr 11, 2023 at 05:45:41PM -0400, Paul Moore wrote:
> > On Mon, Apr 10, 2023 at 3:10???PM Fan Wu <wufan@linux.microsoft.com> wrote:
> > > On Thu, Mar 02, 2023 at 02:04:42PM -0500, Paul Moore wrote:
> > > > On Mon, Jan 30, 2023 at 5:58???PM Fan Wu <wufan@linux.microsoft.com> wrote:

...

> > I guess this does make me wonder about keeping a non-active policy
> > loaded in the kernel, what purpose does that serve?
> >
>
> The non-active policy doesn't serve anything unless it is activated. User can
> even delete a policy if that is no longer needed. Non-active is just the default
> state when a new policy is loaded.
>
> If IPE supports namespace, there is another use case where different containers
> can select different policies as the active policy from among multiple loaded
> policies. Deven has presented a demo of this during LSS 2021. But this goes
> beyond the scope of this version.

Do you plan to add namespace support at some point in the
not-too-distant future?  If so, I'm okay with keeping support for
multiple policies, but if you think you're only going to support one
active policy at a time, it might be better to remove support for
multiple (inactive) policies.
Fan Wu April 17, 2023, 6:06 p.m. UTC | #8
On Thu, Apr 13, 2023 at 02:45:07PM -0400, Paul Moore wrote:
> On Wed, Apr 12, 2023 at 7:36???PM Fan Wu <wufan@linux.microsoft.com> wrote:
> > On Tue, Apr 11, 2023 at 05:45:41PM -0400, Paul Moore wrote:
> > > On Mon, Apr 10, 2023 at 3:10???PM Fan Wu <wufan@linux.microsoft.com> wrote:
> > > > On Thu, Mar 02, 2023 at 02:04:42PM -0500, Paul Moore wrote:
> > > > > On Mon, Jan 30, 2023 at 5:58???PM Fan Wu <wufan@linux.microsoft.com> wrote:
> 
> ...
> 
> > > I guess this does make me wonder about keeping a non-active policy
> > > loaded in the kernel, what purpose does that serve?
> > >
> >
> > The non-active policy doesn't serve anything unless it is activated. User can
> > even delete a policy if that is no longer needed. Non-active is just the default
> > state when a new policy is loaded.
> >
> > If IPE supports namespace, there is another use case where different containers
> > can select different policies as the active policy from among multiple loaded
> > policies. Deven has presented a demo of this during LSS 2021. But this goes
> > beyond the scope of this version.
> 
> Do you plan to add namespace support at some point in the
> not-too-distant future?  If so, I'm okay with keeping support for
> multiple policies, but if you think you're only going to support one
> active policy at a time, it might be better to remove support for
> multiple (inactive) policies.
> 
> -- 
> paul-moore.com

Another benefit of having multiple policies is that it provides isolation
between different policies. For instance, if we have two policies named
"policy_a" and "policy_b," we can ensure that only team a can update "policy_a,"
and only team b can update "policy_b." This way, both teams can update
their policy without affecting others. However, if there is only one policy
in the system, both teams will have to operate on the same policy, making it
less manageable.

Besides, removing multiple (inactive) policies support will
render the policy_name field meaningless, and we should only audit the policy
hash. I am fine if we decide to go for the single policy option.

-Fan
Paul Moore April 17, 2023, 8:16 p.m. UTC | #9
On Mon, Apr 17, 2023 at 2:06 PM Fan Wu <wufan@linux.microsoft.com> wrote:
> On Thu, Apr 13, 2023 at 02:45:07PM -0400, Paul Moore wrote:
> > On Wed, Apr 12, 2023 at 7:36???PM Fan Wu <wufan@linux.microsoft.com> wrote:
> > > On Tue, Apr 11, 2023 at 05:45:41PM -0400, Paul Moore wrote:
> > > > On Mon, Apr 10, 2023 at 3:10???PM Fan Wu <wufan@linux.microsoft.com> wrote:
> > > > > On Thu, Mar 02, 2023 at 02:04:42PM -0500, Paul Moore wrote:
> > > > > > On Mon, Jan 30, 2023 at 5:58???PM Fan Wu <wufan@linux.microsoft.com> wrote:
> >
> > ...
> >
> > > > I guess this does make me wonder about keeping a non-active policy
> > > > loaded in the kernel, what purpose does that serve?
> > > >
> > >
> > > The non-active policy doesn't serve anything unless it is activated. User can
> > > even delete a policy if that is no longer needed. Non-active is just the default
> > > state when a new policy is loaded.
> > >
> > > If IPE supports namespace, there is another use case where different containers
> > > can select different policies as the active policy from among multiple loaded
> > > policies. Deven has presented a demo of this during LSS 2021. But this goes
> > > beyond the scope of this version.
> >
> > Do you plan to add namespace support at some point in the
> > not-too-distant future?  If so, I'm okay with keeping support for
> > multiple policies, but if you think you're only going to support one
> > active policy at a time, it might be better to remove support for
> > multiple (inactive) policies.
> >
> > --
> > paul-moore.com
>
> Another benefit of having multiple policies is that it provides isolation
> between different policies. For instance, if we have two policies named
> "policy_a" and "policy_b," we can ensure that only team a can update "policy_a,"
> and only team b can update "policy_b." This way, both teams can update
> their policy without affecting others. However, if there is only one policy
> in the system, both teams will have to operate on the same policy, making it
> less manageable.

That only really matters if both policies are active at the same time;
if only one policy can be active at one point in time the only
permission that matters is the one who can load/activate a policy.

Allowing for multiple policies complicates the code.  If there is
another feature that requires multiple policies, e.g. IPE namespaces,
then that is okay.  However, if there is no feature which requires
multiple active policies, supporting multiple loaded policies only
increases the risk of an exploitable bug in the IPE code.

> Besides, removing multiple (inactive) policies support will
> render the policy_name field meaningless, and we should only audit the policy
> hash. I am fine if we decide to go for the single policy option.

Once again, I think it comes back to: do you still want to support IPE
namespaces at some point in the future, and if so, when do you expect
to work on that?
Fan Wu April 17, 2023, 9:18 p.m. UTC | #10
On Mon, Apr 17, 2023 at 04:16:29PM -0400, Paul Moore wrote:
> On Mon, Apr 17, 2023 at 2:06???PM Fan Wu <wufan@linux.microsoft.com> wrote:
> > On Thu, Apr 13, 2023 at 02:45:07PM -0400, Paul Moore wrote:
> > > On Wed, Apr 12, 2023 at 7:36???PM Fan Wu <wufan@linux.microsoft.com> wrote:
> > > > On Tue, Apr 11, 2023 at 05:45:41PM -0400, Paul Moore wrote:
> > > > > On Mon, Apr 10, 2023 at 3:10???PM Fan Wu <wufan@linux.microsoft.com> wrote:
> > > > > > On Thu, Mar 02, 2023 at 02:04:42PM -0500, Paul Moore wrote:
> > > > > > > On Mon, Jan 30, 2023 at 5:58???PM Fan Wu <wufan@linux.microsoft.com> wrote:
> > >
> > > ...
> > >
> > > > > I guess this does make me wonder about keeping a non-active policy
> > > > > loaded in the kernel, what purpose does that serve?
> > > > >
> > > >
> > > > The non-active policy doesn't serve anything unless it is activated. User can
> > > > even delete a policy if that is no longer needed. Non-active is just the default
> > > > state when a new policy is loaded.
> > > >
> > > > If IPE supports namespace, there is another use case where different containers
> > > > can select different policies as the active policy from among multiple loaded
> > > > policies. Deven has presented a demo of this during LSS 2021. But this goes
> > > > beyond the scope of this version.
> > >
> > > Do you plan to add namespace support at some point in the
> > > not-too-distant future?  If so, I'm okay with keeping support for
> > > multiple policies, but if you think you're only going to support one
> > > active policy at a time, it might be better to remove support for
> > > multiple (inactive) policies.
> > >
> > > --
> > > paul-moore.com
> >
> > Another benefit of having multiple policies is that it provides isolation
> > between different policies. For instance, if we have two policies named
> > "policy_a" and "policy_b," we can ensure that only team a can update "policy_a,"
> > and only team b can update "policy_b." This way, both teams can update
> > their policy without affecting others. However, if there is only one policy
> > in the system, both teams will have to operate on the same policy, making it
> > less manageable.
> 
> That only really matters if both policies are active at the same time;
> if only one policy can be active at one point in time the only
> permission that matters is the one who can load/activate a policy.
> 
> Allowing for multiple policies complicates the code.  If there is
> another feature that requires multiple policies, e.g. IPE namespaces,
> then that is okay.  However, if there is no feature which requires
> multiple active policies, supporting multiple loaded policies only
> increases the risk of an exploitable bug in the IPE code.
> 
> > Besides, removing multiple (inactive) policies support will
> > render the policy_name field meaningless, and we should only audit the policy
> > hash. I am fine if we decide to go for the single policy option.
> 
> Once again, I think it comes back to: do you still want to support IPE
> namespaces at some point in the future, and if so, when do you expect
> to work on that?
> 

Yes, absolutely! We definitely have plans to support namespaces in the future.
However, it's worth mentioning that there are other tasks that we may need
to prioritize due to their relatively lower complexity. For example, before
we can fully implement namespaces, we need to address some other important
aspects of the system, such as adding a policy language for integrity
enforcement on configuration files and defining trusted certificates
that can sign the root hash. Therefore, the timeline for implementing
namespaces will depend on the completion time of these tasks.

I understand your concerns, and we can proceed with a single policy design
for the initial version.

-Fan
Paul Moore April 17, 2023, 9:31 p.m. UTC | #11
On Mon, Apr 17, 2023 at 5:18 PM Fan Wu <wufan@linux.microsoft.com> wrote:
> On Mon, Apr 17, 2023 at 04:16:29PM -0400, Paul Moore wrote:
> > On Mon, Apr 17, 2023 at 2:06???PM Fan Wu <wufan@linux.microsoft.com> wrote:
> > > On Thu, Apr 13, 2023 at 02:45:07PM -0400, Paul Moore wrote:
> > > > On Wed, Apr 12, 2023 at 7:36???PM Fan Wu <wufan@linux.microsoft.com> wrote:
> > > > > On Tue, Apr 11, 2023 at 05:45:41PM -0400, Paul Moore wrote:
> > > > > > On Mon, Apr 10, 2023 at 3:10???PM Fan Wu <wufan@linux.microsoft.com> wrote:
> > > > > > > On Thu, Mar 02, 2023 at 02:04:42PM -0500, Paul Moore wrote:
> > > > > > > > On Mon, Jan 30, 2023 at 5:58???PM Fan Wu <wufan@linux.microsoft.com> wrote:
> > > >
> > > > ...
> > > >
> > > > > > I guess this does make me wonder about keeping a non-active policy
> > > > > > loaded in the kernel, what purpose does that serve?
> > > > > >
> > > > >
> > > > > The non-active policy doesn't serve anything unless it is activated. User can
> > > > > even delete a policy if that is no longer needed. Non-active is just the default
> > > > > state when a new policy is loaded.
> > > > >
> > > > > If IPE supports namespace, there is another use case where different containers
> > > > > can select different policies as the active policy from among multiple loaded
> > > > > policies. Deven has presented a demo of this during LSS 2021. But this goes
> > > > > beyond the scope of this version.
> > > >
> > > > Do you plan to add namespace support at some point in the
> > > > not-too-distant future?  If so, I'm okay with keeping support for
> > > > multiple policies, but if you think you're only going to support one
> > > > active policy at a time, it might be better to remove support for
> > > > multiple (inactive) policies.
> > > >
> > > > --
> > > > paul-moore.com
> > >
> > > Another benefit of having multiple policies is that it provides isolation
> > > between different policies. For instance, if we have two policies named
> > > "policy_a" and "policy_b," we can ensure that only team a can update "policy_a,"
> > > and only team b can update "policy_b." This way, both teams can update
> > > their policy without affecting others. However, if there is only one policy
> > > in the system, both teams will have to operate on the same policy, making it
> > > less manageable.
> >
> > That only really matters if both policies are active at the same time;
> > if only one policy can be active at one point in time the only
> > permission that matters is the one who can load/activate a policy.
> >
> > Allowing for multiple policies complicates the code.  If there is
> > another feature that requires multiple policies, e.g. IPE namespaces,
> > then that is okay.  However, if there is no feature which requires
> > multiple active policies, supporting multiple loaded policies only
> > increases the risk of an exploitable bug in the IPE code.
> >
> > > Besides, removing multiple (inactive) policies support will
> > > render the policy_name field meaningless, and we should only audit the policy
> > > hash. I am fine if we decide to go for the single policy option.
> >
> > Once again, I think it comes back to: do you still want to support IPE
> > namespaces at some point in the future, and if so, when do you expect
> > to work on that?
>
> Yes, absolutely! We definitely have plans to support namespaces in the future.
> However, it's worth mentioning that there are other tasks that we may need
> to prioritize due to their relatively lower complexity. For example, before
> we can fully implement namespaces, we need to address some other important
> aspects of the system, such as adding a policy language for integrity
> enforcement on configuration files and defining trusted certificates
> that can sign the root hash. Therefore, the timeline for implementing
> namespaces will depend on the completion time of these tasks.
>
> I understand your concerns, and we can proceed with a single policy design
> for the initial version.

I think it's okay to stick with the multi-policy code for the initial
submission, you've got the code now, and it's tested.  I just wanted
to make sure there were plans to make use of it at some point, if not
we might as well drop it now.  However, it sounds like you've got a
plan to utilize the multi-policy support so that's fine with me.
diff mbox series

Patch

diff --git a/security/ipe/Makefile b/security/ipe/Makefile
index d7f2870d7c09..8602d71250b4 100644
--- a/security/ipe/Makefile
+++ b/security/ipe/Makefile
@@ -7,7 +7,9 @@ 
 
 obj-$(CONFIG_SECURITY_IPE) += \
 	eval.o \
+	fs.o \
 	hooks.o \
 	ipe.o \
 	policy.o \
+	policy_fs.o \
 	policy_parser.o \
diff --git a/security/ipe/fs.c b/security/ipe/fs.c
new file mode 100644
index 000000000000..9f6a4867bec2
--- /dev/null
+++ b/security/ipe/fs.c
@@ -0,0 +1,101 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) Microsoft Corporation. All rights reserved.
+ */
+#include "ipe.h"
+#include "fs.h"
+#include "policy.h"
+
+#include <linux/dcache.h>
+#include <linux/security.h>
+
+static struct dentry *np __ro_after_init;
+static struct dentry *root __ro_after_init;
+struct dentry *policy_root __ro_after_init;
+
+/**
+ * new_policy - Write handler for the securityfs node, "ipe/new_policy".
+ * @f: Supplies a file structure representing the securityfs node.
+ * @data: Suppleis a buffer passed to the write syscall.
+ * @len: Supplies the length of @data.
+ * @offset: unused.
+ *
+ * Return:
+ * * >0	- Success, Length of buffer written
+ * * <0	- Error
+ */
+static ssize_t new_policy(struct file *f, const char __user *data,
+			  size_t len, loff_t *offset)
+{
+	int rc = 0;
+	char *copy = NULL;
+	struct ipe_policy *p = NULL;
+
+	if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN))
+		return -EPERM;
+
+	copy = memdup_user_nul(data, len);
+	if (IS_ERR(copy)) {
+		rc = PTR_ERR(copy);
+		goto err;
+	}
+
+	p = ipe_new_policy(NULL, 0, copy, len);
+	if (IS_ERR(p)) {
+		rc = PTR_ERR(p);
+		goto err;
+	}
+
+	rc = ipe_new_policyfs_node(p);
+	if (rc)
+		goto err;
+
+err:
+	return (rc < 0) ? rc : len;
+}
+
+static const struct file_operations np_fops = {
+	.write = new_policy,
+};
+
+/**
+ * ipe_init_securityfs - Initialize IPE's securityfs tree at fsinit.
+ *
+ * Return:
+ * * !0	- Error
+ * * 0	- OK
+ */
+static int __init ipe_init_securityfs(void)
+{
+	int rc = 0;
+
+	if (!ipe_enabled)
+		return -EOPNOTSUPP;
+
+	root = securityfs_create_dir("ipe", NULL);
+	if (IS_ERR(root)) {
+		rc = PTR_ERR(root);
+		goto err;
+	}
+
+	np = securityfs_create_file("new_policy", 0200, root, NULL, &np_fops);
+	if (IS_ERR(np)) {
+		rc = PTR_ERR(np);
+		goto err;
+	}
+
+	policy_root = securityfs_create_dir("policies", root);
+	if (IS_ERR(policy_root)) {
+		rc = PTR_ERR(policy_root);
+		goto err;
+	}
+
+	return 0;
+err:
+	securityfs_remove(np);
+	securityfs_remove(root);
+	securityfs_remove(policy_root);
+	return rc;
+}
+
+fs_initcall(ipe_init_securityfs);
diff --git a/security/ipe/fs.h b/security/ipe/fs.h
new file mode 100644
index 000000000000..fa105d9d6fc5
--- /dev/null
+++ b/security/ipe/fs.h
@@ -0,0 +1,17 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) Microsoft Corporation. All rights reserved.
+ */
+
+#ifndef IPE_FS_H
+#define IPE_FS_H
+
+#include "policy.h"
+
+extern struct dentry *policy_root __ro_after_init;
+
+void ipe_soft_del_policyfs(struct ipe_policy *p);
+int ipe_new_policyfs_node(struct ipe_policy *p);
+void ipe_del_policyfs_node(struct ipe_policy *p);
+
+#endif /* IPE_FS_H */
diff --git a/security/ipe/ipe.c b/security/ipe/ipe.c
index 551c6d90ac11..bef923026b50 100644
--- a/security/ipe/ipe.c
+++ b/security/ipe/ipe.c
@@ -5,6 +5,8 @@ 
 
 #include "ipe.h"
 
+bool ipe_enabled;
+
 static struct lsm_blob_sizes ipe_blobs __lsm_ro_after_init = {
 };
 
@@ -30,6 +32,7 @@  static int __init ipe_init(void)
 	int rc = 0;
 
 	security_add_hooks(ipe_hooks, ARRAY_SIZE(ipe_hooks), "ipe");
+	ipe_enabled = true;
 
 	return rc;
 }
diff --git a/security/ipe/ipe.h b/security/ipe/ipe.h
index ee7ec3f3b55d..43cc132ed048 100644
--- a/security/ipe/ipe.h
+++ b/security/ipe/ipe.h
@@ -10,4 +10,6 @@ 
 
 #include <linux/lsm_hooks.h>
 
+extern bool ipe_enabled;
+
 #endif /* IPE_H */
diff --git a/security/ipe/policy.c b/security/ipe/policy.c
index 772d876b1087..a5e9c6e5691b 100644
--- a/security/ipe/policy.c
+++ b/security/ipe/policy.c
@@ -4,12 +4,39 @@ 
  */
 
 #include "ipe.h"
+#include "eval.h"
+#include "fs.h"
 #include "policy.h"
 #include "policy_parser.h"
 #include "digest.h"
 
 #include <linux/verification.h>
 
+/* lock for synchronizing writers across ipe policy */
+DEFINE_SPINLOCK(ipe_policy_lock);
+
+/**
+ * ver_to_u64 - Convert an internal ipe_policy_version to a u64.
+ * @p: Policy to extract the version from.
+ *
+ * Bits (LSB is index 0):
+ *	[48,32] -> Major
+ *	[32,16] -> Minor
+ *	[16, 0] -> Revision
+ *
+ * Return: u64 version of the embedded version structure.
+ */
+static inline u64 ver_to_u64(const struct ipe_policy *const p)
+{
+	u64 r = 0;
+
+	r = (((u64)p->parsed->version.major) << 32)
+	  | (((u64)p->parsed->version.minor) << 16)
+	  | ((u64)(p->parsed->version.rev));
+
+	return r;
+}
+
 /**
  * ipe_free_policy - Deallocate a given IPE policy.
  * @p: Supplies the policy to free.
@@ -21,6 +48,7 @@  void ipe_free_policy(struct ipe_policy *p)
 	if (IS_ERR_OR_NULL(p))
 		return;
 
+	ipe_del_policyfs_node(p);
 	free_parsed_policy(p->parsed);
 	if (!p->pkcs7)
 		kfree(p->text);
@@ -39,6 +67,70 @@  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.
+ * @addr: Supplies a pointer to the i_private for saving 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.
+ *
+ * Return:
+ * * !IS_ERR	- OK
+ * * -ENOENT	- Policy doesn't exist
+ * * -EINVAL	- New policy is invalid
+ */
+struct ipe_policy *ipe_update_policy(struct ipe_policy __rcu **addr,
+				     const char *text, size_t textlen,
+				     const char *pkcs7, size_t pkcs7len)
+{
+	int rc = 0;
+	struct ipe_policy *old, *new;
+
+	old = ipe_get_policy_rcu(*addr);
+	if (!old) {
+		rc = -ENOENT;
+		goto err;
+	}
+
+	new = ipe_new_policy(text, textlen, pkcs7, pkcs7len);
+	if (IS_ERR(new)) {
+		rc = PTR_ERR(new);
+		goto err;
+	}
+
+	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;
+	}
+
+	if (ipe_is_policy_active(old)) {
+		spin_lock(&ipe_policy_lock);
+		rcu_assign_pointer(ipe_active_policy, new);
+		spin_unlock(&ipe_policy_lock);
+		synchronize_rcu();
+	}
+
+	rcu_assign_pointer(*addr, new);
+
+	swap(new->policyfs, old->policyfs);
+	ipe_free_policy(old);
+
+	goto out;
+err:
+	ipe_free_policy(new);
+out:
+	return (rc < 0) ? ERR_PTR(rc) : new;
+}
+
 /**
  * ipe_new_policy - Allocate and parse an ipe_policy structure.
  *
@@ -117,3 +209,46 @@  struct ipe_policy *ipe_get_policy_rcu(struct ipe_policy __rcu *p)
 
 	return rv;
 }
+
+/**
+ * ipe_set_active_pol - Make @p the active policy.
+ * @p: Supplies a pointer to the policy to make active.
+ */
+int ipe_set_active_pol(const struct ipe_policy *p)
+{
+	int rc = 0;
+	struct ipe_policy *ap = NULL;
+
+	ap = ipe_get_policy_rcu(ipe_active_policy);
+	if (ap && ver_to_u64(ap) > ver_to_u64(p)) {
+		rc = -EINVAL;
+		goto out;
+	}
+
+	spin_lock(&ipe_policy_lock);
+	rcu_assign_pointer(ipe_active_policy, p);
+	spin_unlock(&ipe_policy_lock);
+	synchronize_rcu();
+
+out:
+	return rc;
+}
+
+/**
+ * ipe_is_policy_active - Determine wehther @p is the active policy.
+ * @p: Supplies a pointer to the policy to check.
+ *
+ * Return:
+ * * true	- @p is the active policy
+ * * false	- @p is not the active policy
+ */
+bool ipe_is_policy_active(const struct ipe_policy *p)
+{
+	bool rv;
+
+	rcu_read_lock();
+	rv = rcu_access_pointer(ipe_active_policy) == p;
+	rcu_read_unlock();
+
+	return rv;
+}
diff --git a/security/ipe/policy.h b/security/ipe/policy.h
index 967d816cd5cd..0cb42b6f246e 100644
--- a/security/ipe/policy.h
+++ b/security/ipe/policy.h
@@ -70,11 +70,18 @@  struct ipe_policy {
 	size_t		textlen;
 
 	struct ipe_parsed_policy *parsed;
+
+	struct dentry *policyfs;
 };
 
 struct ipe_policy *ipe_new_policy(const char *text, size_t textlen,
 				  const char *pkcs7, size_t pkcs7len);
 void ipe_free_policy(struct ipe_policy *pol);
 struct ipe_policy *ipe_get_policy_rcu(struct ipe_policy __rcu *p);
+struct ipe_policy *ipe_update_policy(struct ipe_policy **addr, const char *text,
+				     size_t textlen, const char *pkcs7,
+				     size_t pkcs7len);
+int ipe_set_active_pol(const struct ipe_policy *p);
+bool ipe_is_policy_active(const struct ipe_policy *p);
 
 #endif /* IPE_POLICY_H */
diff --git a/security/ipe/policy_fs.c b/security/ipe/policy_fs.c
new file mode 100644
index 000000000000..72759cc8938d
--- /dev/null
+++ b/security/ipe/policy_fs.c
@@ -0,0 +1,459 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) Microsoft Corporation. All rights reserved.
+ */
+#include "ipe.h"
+#include "policy.h"
+#include "fs.h"
+
+#include <linux/fs.h>
+#include <linux/namei.h>
+#include <linux/types.h>
+#include <linux/dcache.h>
+#include <linux/security.h>
+
+#define MAX_VERSION_SIZE ARRAY_SIZE("65535.65535.65535")
+
+/**
+ * find_policy - return a policy pointer saved in i_private of a dentry.
+ * @f: Securityfs object that contains a link to the dentry containing the
+ *     policy structure.
+ *
+ * Return: Always-Valid Address Pointer
+ */
+static inline struct ipe_policy __rcu **find_policy(struct file *f)
+{
+	struct dentry *link;
+
+	link = d_inode(f->f_path.dentry)->i_private;
+
+	return (struct ipe_policy __rcu **)&(d_inode(link)->i_private);
+}
+
+/**
+ * ipefs_file - defines a file in securityfs.
+ */
+struct ipefs_file {
+	const char	*name;
+	umode_t		access;
+	const struct	file_operations *fops;
+};
+
+/**
+ * read_pkcs7 - Read handler for "ipe/policies/$name/pkcs7".
+ * @f: Supplies a file structure representing the securityfs node.
+ * @data: Suppleis a buffer passed to the write syscall.
+ * @len: Supplies the length of @data.
+ * @offset: unused.
+ *
+ * @data will be populated with the pkcs7 blob representing the policy
+ * on success. If the policy is unsigned (like the boot policy), this
+ * will return -ENOENT.
+ *
+ * Return:
+ * * >0	- Success, Length of buffer written
+ * * <0	- Error
+ */
+static ssize_t read_pkcs7(struct file *f, char __user *data,
+			  size_t len, loff_t *offset)
+{
+	int rc = 0;
+	struct ipe_policy *p = NULL;
+
+	p = ipe_get_policy_rcu(*find_policy(f));
+	if (!p)
+		return -ENOENT;
+
+	if (!p->pkcs7) {
+		rc = -ENOENT;
+		goto out;
+	}
+
+	rc = simple_read_from_buffer(data, len, offset, p->pkcs7, p->pkcs7len);
+
+out:
+	return rc;
+}
+
+/**
+ * read_policy - Read handler for "ipe/policies/$name/policy".
+ * @f: Supplies a file structure representing the securityfs node.
+ * @data: Suppleis a buffer passed to the write syscall.
+ * @len: Supplies the length of @data.
+ * @offset: unused.
+ *
+ * @data will be populated with the plain-text version of the policy
+ * on success.
+ *
+ * Return:
+ * * >0	- Success, Length of buffer written
+ * * <0	- Error
+ */
+static ssize_t read_policy(struct file *f, char __user *data,
+			   size_t len, loff_t *offset)
+{
+	int rc = 0;
+	struct ipe_policy *p = NULL;
+
+	p = ipe_get_policy_rcu(*find_policy(f));
+	if (!p)
+		return -ENOENT;
+
+	rc = simple_read_from_buffer(data, len, offset, p->text, p->textlen);
+
+	return rc;
+}
+
+/**
+ * read_name: Read handler for "ipe/policies/$name/name".
+ * @f: Supplies a file structure representing the securityfs node.
+ * @data: Suppleis a buffer passed to the write syscall.
+ * @len: Supplies the length of @data.
+ * @offset: unused.
+ *
+ * @data will be populated with the policy_name attribute on success.
+ *
+ * Return:
+ * * >0	- Success, Length of buffer written
+ * * <0	- Error
+ */
+static ssize_t read_name(struct file *f, char __user *data,
+			 size_t len, loff_t *offset)
+{
+	int rc = 0;
+	struct ipe_policy *p = NULL;
+
+	p = ipe_get_policy_rcu(*find_policy(f));
+	if (!p)
+		return -ENOENT;
+
+	rc = simple_read_from_buffer(data, len, offset, p->parsed->name,
+				     strlen(p->parsed->name));
+
+	return rc;
+}
+
+/**
+ * read_version - Read handler for "ipe/policies/$name/version".
+ * @f: Supplies a file structure representing the securityfs node.
+ * @data: Suppleis a buffer passed to the write syscall.
+ * @len: Supplies the length of @data.
+ * @offset: unused.
+ *
+ * @data will be populated with the version string on success.
+ *
+ * Return:
+ * * >0	- Success, Length of buffer written
+ * * <0	- Error
+ */
+static ssize_t read_version(struct file *f, char __user *data,
+			    size_t len, loff_t *offset)
+{
+	ssize_t rc = 0;
+	size_t bufsize = 0;
+	struct ipe_policy *p = NULL;
+	char buffer[MAX_VERSION_SIZE] = { 0 };
+
+	p = ipe_get_policy_rcu(*find_policy(f));
+	if (!p)
+		return -ENOENT;
+
+	bufsize = scnprintf(buffer, ARRAY_SIZE(buffer), "%hu.%hu.%hu",
+			    p->parsed->version.major, p->parsed->version.minor,
+			    p->parsed->version.rev);
+
+	rc = simple_read_from_buffer(data, len, offset, buffer, bufsize);
+
+	return rc;
+}
+
+/**
+ * setactive - Write handler for "ipe/policies/$name/active".
+ * @f: Supplies a file structure representing the securityfs node.
+ * @data: Supplies a buffer passed to the write syscall.
+ * @len: Supplies the length of @data.
+ * @offset: unused.
+ *
+ * Return:
+ * * >0	- Success, Length of buffer written
+ * * <0	- Error
+ */
+static ssize_t setactive(struct file *f, const char __user *data,
+			 size_t len, loff_t *offset)
+{
+	int rc = 0;
+	bool value = false;
+	struct ipe_policy *p = NULL;
+
+	if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN))
+		return -EPERM;
+
+	rc = kstrtobool_from_user(data, len, &value);
+	if (rc)
+		goto out;
+
+	if (!value) {
+		rc = -EINVAL;
+		goto out;
+	}
+
+	p = ipe_get_policy_rcu(*find_policy(f));
+	if (!p) {
+		rc = -ENOENT;
+		goto out;
+	}
+
+	rc = ipe_set_active_pol(p);
+
+out:
+	return (rc < 0) ? rc : len;
+}
+
+/**
+ * getactive - Read handler for "ipe/policies/$name/active".
+ * @f: Supplies a file structure representing the securityfs node.
+ * @data: Suppleis a buffer passed to the write syscall.
+ * @len: Supplies the length of @data.
+ * @offset: unused.
+ *
+ * @data will be populated with the 1 or 0 depending on if the
+ * corresponding policy is active.
+ *
+ * Return:
+ * * >0	- Success, Length of buffer written
+ * * <0	- Error
+ */
+static ssize_t getactive(struct file *f, char __user *data,
+			 size_t len, loff_t *offset)
+{
+	int rc = 0;
+	const char *str;
+	struct ipe_policy *p = NULL;
+
+	p = ipe_get_policy_rcu(*find_policy(f));
+	if (!p) {
+		rc = -ENOENT;
+		goto out;
+	}
+
+	str = ipe_is_policy_active(p) ? "1" : "0";
+	rc = simple_read_from_buffer(data, len, offset, str, 1);
+
+out:
+	return rc;
+}
+
+/**
+ * update_policy - Write handler for "ipe/policies/$name/update".
+ * @f: Supplies a file structure representing the securityfs node.
+ * @data: Supplies a buffer passed to the write syscall.
+ * @len: Supplies the length of @data.
+ * @offset: unused.
+ *
+ * On success this updates the policy represented by $name,
+ * in-place.
+ *
+ * Return:
+ * * >0	- Success, Length of buffer written
+ * * <0	- Error
+ */
+static ssize_t update_policy(struct file *f, const char __user *data,
+			     size_t len, loff_t *offset)
+{
+	int rc = 0;
+	char *copy = NULL;
+	struct inode *ino = NULL;
+	struct ipe_policy *new = NULL;
+	struct ipe_policy __rcu **addr = NULL;
+
+	if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN))
+		return -EPERM;
+
+	copy = memdup_user(data, len);
+	if (IS_ERR(copy)) {
+		rc = PTR_ERR(copy);
+		goto err;
+	}
+
+	ino = d_inode(f->f_path.dentry->d_parent);
+	inode_lock(ino);
+	addr = find_policy(f);
+	new = ipe_update_policy(addr, NULL, 0, copy, len);
+	inode_unlock(ino);
+	synchronize_rcu();
+	if (IS_ERR(new)) {
+		rc = PTR_ERR(new);
+		goto err;
+	}
+
+	kfree(copy);
+	return len;
+err:
+	kfree(copy);
+	return rc;
+}
+
+/**
+ * delete_policy - write handler for  "ipe/policies/$name/delete".
+ * @f: Supplies a file structure representing the securityfs node.
+ * @data: Supplies a buffer passed to the write syscall.
+ * @len: Supplies the length of @data.
+ * @offset: unused.
+ *
+ * On success this deletes the policy represented by $name.
+ *
+ * Return:
+ * * >0	- Success, Length of buffer written
+ * * <0	- Error
+ */
+static ssize_t delete_policy(struct file *f, const char __user *data,
+			     size_t len, loff_t *offset)
+{
+	int rc = 0;
+	bool value = false;
+	struct ipe_policy *p = NULL;
+
+	if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN))
+		return -EPERM;
+
+	rc = kstrtobool_from_user(data, len, &value);
+	if (rc)
+		goto out;
+
+	if (!value) {
+		rc = -EINVAL;
+		goto out;
+	}
+
+	p = ipe_get_policy_rcu(*find_policy(f));
+	if (!p) {
+		rc = -ENOENT;
+		goto out;
+	}
+
+	if (ipe_is_policy_active(p)) {
+		rc = -EPERM;
+		goto out;
+	}
+
+	ipe_free_policy(p);
+out:
+	return (rc < 0) ? rc : len;
+}
+
+static const struct file_operations content_fops = {
+	.read = read_policy,
+};
+
+static const struct file_operations pkcs7_fops = {
+	.read = read_pkcs7,
+};
+
+static const struct file_operations name_fops = {
+	.read = read_name,
+};
+
+static const struct file_operations ver_fops = {
+	.read = read_version,
+};
+
+static const struct file_operations active_fops = {
+	.write = setactive,
+	.read = getactive,
+};
+
+static const struct file_operations update_fops = {
+	.write = update_policy,
+};
+
+static const struct file_operations delete_fops = {
+	.write = delete_policy,
+};
+
+/**
+ * policy_subdir - files under a policy subdirectory
+ */
+static const struct ipefs_file policy_subdir[] = {
+	{ "pkcs7", 0444, &pkcs7_fops },
+	{ "policy", 0444, &content_fops },
+	{ "name", 0444, &name_fops },
+	{ "version", 0444, &ver_fops },
+	{ "active", 0600, &active_fops },
+	{ "update", 0200, &update_fops },
+	{ "delete", 0200, &delete_fops },
+};
+
+/**
+ * soft_del_policyfs - soft delete a policyfs node.
+ * @p: Supplies a ipe_policy associated with the node to delete.
+ *
+ * This deletes the i_private field of a policyfs node.
+ */
+static void soft_del_policyfs(struct ipe_policy *p)
+{
+	struct inode *ino = NULL;
+	struct ipe_policy __rcu **addr = NULL;
+
+	ino = d_inode(p->policyfs);
+	addr = (struct ipe_policy __rcu **)&ino->i_private;
+
+	inode_lock(ino);
+	rcu_assign_pointer(*addr, NULL);
+	inode_unlock(ino);
+	synchronize_rcu();
+}
+
+/**
+ * ipe_del_policyfs_node - Delete a securityfs entry for @p.
+ * @p: Supplies a pointer to the policy to delete a securityfs entry for.
+ */
+void ipe_del_policyfs_node(struct ipe_policy *p)
+{
+	if (IS_ERR_OR_NULL(p->policyfs))
+		return;
+
+	soft_del_policyfs(p);
+	securityfs_recursive_remove(p->policyfs);
+}
+
+/**
+ * ipe_new_policyfs_node - Create a securityfs entry for @p.
+ * @p: Supplies a pointer to the policy to create a securityfs entry for.
+ *
+ * Return:
+ * * 0	- OK
+ * * !0	- Error
+ */
+int ipe_new_policyfs_node(struct ipe_policy *p)
+{
+	int rc = 0;
+	size_t i = 0;
+	struct dentry *d = NULL;
+	struct ipe_policy **addr = NULL;
+	const struct ipefs_file *f = NULL;
+
+	p->policyfs = securityfs_create_dir(p->parsed->name, policy_root);
+	if (IS_ERR(p->policyfs)) {
+		rc = PTR_ERR(p->policyfs);
+		goto err;
+	}
+
+	addr = (struct ipe_policy **)&(d_inode(p->policyfs)->i_private);
+	*addr = p;
+
+	for (i = 0; i < ARRAY_SIZE(policy_subdir); ++i) {
+		f = &policy_subdir[i];
+
+		d = securityfs_create_file(f->name, f->access, p->policyfs, p->policyfs,
+					   f->fops);
+		if (IS_ERR(d)) {
+			rc = PTR_ERR(d);
+			goto err;
+		}
+	}
+
+	return 0;
+err:
+	ipe_del_policyfs_node(p);
+	return rc;
+}