On Sat, Jul 08, 2023 at 12:23:04AM -0400, Paul Moore wrote:
> 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 "@".
>
Yes it shouldn't be here, sorry confusion.
> > + * @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?
>
Let me try to pharse it in another way, how about 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;
> > +
> > + 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.
>
Sorry I missed that, I will add the locking assumption in the description.
Also, just found the err part below might better be replaced by return.
> > +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.
>
This call only checks the value inside the pointer but doesn't dereference it.
Also from https://lwn.net/Articles/652156/ I found it says "The call to
rcu_access_pointer() need not be protected. In contrast, rcu_dereference() must
either be within an RCU read-side critical section", so I didn't add the lock
here, is this article outdated?
Thanks,
-Fan
> > + rc = simple_read_from_buffer(data, len, offset, str, 1);
> > +
> > + return rc;
> > +}
>
> --
> paul-moore.com
