On Jun 28, 2023 Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> wrote: > > As is typical with LSMs, IPE uses securityfs as its interface with > userspace. for a complete list of the interfaces and the respective > inputs/outputs, please see the documentation under > admin-guide/LSM/ipe.rst > > Signed-off-by: Deven Bowers <deven.desai@xxxxxxxxxxxxxxxxxxx> > Signed-off-by: Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> > --- > 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 | 111 +++++++++ > security/ipe/policy.h | 9 + > security/ipe/policy_fs.c | 481 +++++++++++++++++++++++++++++++++++++++ > 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 4069ff075093..3e8e4a06a044 100644 > --- a/security/ipe/policy.c > +++ b/security/ipe/policy.c > @@ -7,9 +7,36 @@ > #include <linux/verification.h> > > #include "ipe.h" > +#include "eval.h" > +#include "fs.h" > #include "policy.h" > #include "policy_parser.h" > > +/* lock for synchronizing writers across ipe policy */ > +DEFINE_MUTEX(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; > + > + 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,65 @@ 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. What does "@old" refer to? I'm guessing you want to drop the "@". > + * @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. > + * > + * Return: > + * * !IS_ERR - The old policy "The old policy" is what? > + * * -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; > + > + lockdep_assert_held(&ipe_policy_lock); > + > + 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; > + > + ap = rcu_dereference_protected(ipe_active_policy, > + lockdep_is_held(&ipe_policy_lock)); > + if (old == ap) > + rcu_assign_pointer(ipe_active_policy, new); > + > + swap(new->policyfs, old->policyfs); We don't have to worry about @new, but is there a guarantee that this function is the only one attempting to modify @old? *EDIT*: I found that @root is locked by the caller, that's good. I would suggest adding this assumption/requirement to the function's description. In general whenever a function requires something from a caller it should be documented in the function's description. > +out: > + return (rc < 0) ? ERR_PTR(rc) : old; > +err: > + ipe_free_policy(new); > + goto out; > +} > + ... > diff --git a/security/ipe/policy_fs.c b/security/ipe/policy_fs.c > new file mode 100644 > index 000000000000..52a120118cda > --- /dev/null > +++ b/security/ipe/policy_fs.c > @@ -0,0 +1,481 @@ ... > +/** > + * 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 inode *root = NULL; > + const struct ipe_policy *p = NULL; > + > + root = d_inode(f->f_path.dentry->d_parent); > + > + inode_lock_shared(root); > + p = (struct ipe_policy *)root->i_private; > + if (!p) { > + inode_unlock_shared(root); > + return -ENOENT; > + } > + inode_unlock_shared(root); > + > + str = (p == rcu_access_pointer(ipe_active_policy)) ? "1" : "0"; The line above should be wrapped with a RCU lock. > + rc = simple_read_from_buffer(data, len, offset, str, 1); > + > + return rc; > +} -- paul-moore.com