Re: [RFC PATCH v9 03/16] ipe: add evaluation loop and introduce 'boot_verified' as a trust provider
On Mon, Apr 10, 2023 at 2:53 PM Fan Wu wrote:
> On Thu, Mar 02, 2023 at 02:03:11PM -0500, Paul Moore wrote:
> > On Mon, Jan 30, 2023 at 5:58???PM Fan Wu wrote:
> > >
> > > From: Deven Bowers
> > >
> > > IPE must have a centralized function to evaluate incoming callers
> > > against IPE's policy. This iteration of the policy against the rules
> > > for that specific caller is known as the evaluation loop.
> > >
> > > In addition, IPE is designed to provide system level trust guarantees,
> > > this usually implies that trust starts from bootup with a hardware root
> > > of trust, which validates the bootloader. After this, the bootloader
> > > verifies the kernel and the initramfs.
> > >
> > > As there's no currently supported integrity method for initramfs, and
> > > it's typically already verified by the bootloader, introduce a property
> > > that causes the first superblock to have an execution to be "pinned",
> > > which is typically initramfs.
> > >
> > > Signed-off-by: Deven Bowers
> > > Signed-off-by: Fan Wu
> >
> > ...
> >
> > > ---
> > > security/ipe/Makefile| 1 +
> > > security/ipe/eval.c | 180 +++
> > > security/ipe/eval.h | 28 ++
> > > security/ipe/hooks.c | 25 +
> > > security/ipe/hooks.h | 14 +++
> > > security/ipe/ipe.c | 1 +
> > > security/ipe/policy.c| 20
> > > security/ipe/policy.h| 3 +
> > > security/ipe/policy_parser.c | 8 +-
> > > 9 files changed, 279 insertions(+), 1 deletion(-)
> > > create mode 100644 security/ipe/eval.c
> > > create mode 100644 security/ipe/eval.h
> > > create mode 100644 security/ipe/hooks.c
> > > create mode 100644 security/ipe/hooks.h
...
> > > diff --git a/security/ipe/eval.c b/security/ipe/eval.c
> > > new file mode 100644
> > > index ..48b5104a3463
> > > --- /dev/null
> > > +++ b/security/ipe/eval.c
> > > @@ -0,0 +1,180 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) Microsoft Corporation. All rights reserved.
> > > + */
> > > +
> > > +#include "ipe.h"
> > > +#include "eval.h"
> > > +#include "hooks.h"
> > > +#include "policy.h"
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +struct ipe_policy __rcu *ipe_active_policy;
> > > +
> > > +static struct super_block *pinned_sb;
> > > +static DEFINE_SPINLOCK(pin_lock);
> > > +#define FILE_SUPERBLOCK(f) ((f)->f_path.mnt->mnt_sb)
> > > +
> > > +/**
> > > + * pin_sb - Pin the underlying superblock of @f, marking it as trusted.
> > > + * @f: Supplies a file structure to source the super_block from.
> > > + */
> > > +static void pin_sb(const struct file *f)
> > > +{
> > > + if (!f)
> > > + return;
> > > + spin_lock(&pin_lock);
> > > + if (pinned_sb)
> > > + goto out;
> > > + pinned_sb = FILE_SUPERBLOCK(f);
> > > +out:
> > > + spin_unlock(&pin_lock);
> > > +}
> >
> > Since you don't actually use @f, just the super_block, you might
> > consider passing the super_block as the parameter and not the
> > associated file.
> >
> > I'd probably also flip the if-then to avoid the 'goto', for example:
> >
> > static void pin_sb(const struct super_block *sb)
> > {
> > if (!sb)
> > return;
> > spin_lock(&pin_lock);
> > if (!pinned_sb)
> > pinned_sb = sb;
> > spin_unlock(&pin_lock);
> > }
> >
>
> Sure, I can change the code accordingly.
>
> > Also, do we need to worry about the initramfs' being unmounted and the
> > super_block going away?
>
> If initramfs is being unmounted, the boot_verified property will never be
> TRUE,
> which is an expected behavior. In an actual use case, we can leverage this
> property to only enable files in initramfs during the booting stage, and
> later switch
> to another policy without the boot_verified property after unmounting the
> initramfs.
> This approach helps keep the allowed set of files minimum at each stage.
I think I was worried about not catching when the fs was unmounted and
the superblock disappeared, but you've got a hook defined for that so
it should be okay. I'm not sure what I was thinking here, sorry for
the noise ...
Regardless of the source of my confusion, your policy/boot_verified
description all sounds good to me.
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v9 02/16] ipe: add policy parser
On Thu, Apr 6, 2023 at 4:00 PM Fan Wu wrote:
> On Thu, Mar 02, 2023 at 02:02:32PM -0500, Paul Moore wrote:
> > On Mon, Jan 30, 2023 at 5:58???PM Fan Wu wrote:
> > >
> > > From: Deven Bowers
> > >
> > > IPE's interpretation of the what the user trusts is accomplished through
> > > its policy. IPE's design is to not provide support for a single trust
> > > provider, but to support multiple providers to enable the end-user to
> > > choose the best one to seek their needs.
> > >
> > > This requires the policy to be rather flexible and modular so that
> > > integrity providers, like fs-verity, dm-verity, dm-integrity, or
> > > some other system, can plug into the policy with minimal code changes.
> > >
> > > Signed-off-by: Deven Bowers
> > > Signed-off-by: Fan Wu
> >
> > ...
> >
> > > ---
> > > security/ipe/Makefile| 2 +
> > > security/ipe/policy.c| 99 +++
> > > security/ipe/policy.h| 77 ++
> > > security/ipe/policy_parser.c | 515 +++
> > > security/ipe/policy_parser.h | 11 +
> > > 5 files changed, 704 insertions(+)
> > > create mode 100644 security/ipe/policy.c
> > > create mode 100644 security/ipe/policy.h
> > > create mode 100644 security/ipe/policy_parser.c
> > > create mode 100644 security/ipe/policy_parser.h
...
> > > diff --git a/security/ipe/policy_parser.c b/security/ipe/policy_parser.c
> > > new file mode 100644
> > > index ..c7ba0e865366
> > > --- /dev/null
> > > +++ b/security/ipe/policy_parser.c
> > > @@ -0,0 +1,515 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) Microsoft Corporation. All rights reserved.
> > > + */
> > > +
> > > +#include "policy.h"
> > > +#include "policy_parser.h"
> > > +#include "digest.h"
> > > +
> > > +#include
> > > +
> > > +#define START_COMMENT '#'
> > > +
> > > +/**
> > > + * new_parsed_policy - Allocate and initialize a parsed policy.
> > > + *
> > > + * Return:
> > > + * * !IS_ERR - OK
> > > + * * -ENOMEM - Out of memory
> > > + */
> > > +static struct ipe_parsed_policy *new_parsed_policy(void)
> > > +{
> > > + size_t i = 0;
> > > + struct ipe_parsed_policy *p = NULL;
> > > + struct ipe_op_table *t = NULL;
> > > +
> > > + p = kzalloc(sizeof(*p), GFP_KERNEL);
> > > + if (!p)
> > > + return ERR_PTR(-ENOMEM);
> > > +
> > > + p->global_default_action = ipe_action_max;
> >
> > I'm assuming you're using the "ipe_action_max" as an intentional bogus
> > placeholder value here, yes? If that is the case, have you considered
> > creating an "invalid" enum with an explicit zero value to save you
> > this additional assignment (you are already using kzalloc())? For
> > example:
> >
> > enum ipe_op_type {
> > IPE_OP_INVALID = 0,
> > IPE_OP_EXEC,
> > ...
> > IPE_OP_MAX,
> > };
> >
> > enum ipe_action_type {
> > IPE_ACTION_INVALID = 0,
> > IPE_ACTION_ALLOW,
> > ...
> > IPE_ACTION_MAX,
> > };
> >
>
> Yes, IPE_ACTION_MAX is kind of the INVALID value we are using here.
>
> But I think we might be adding unnecessary complexity by using the
> IPE_OP_INVLIAD enum here. Currently, we are using IPE_OP_MAX to
> represent the number of operations we have, and we have allocated
> an IPE_OP_MAX-sized array to store linked lists that link all rules
> for each operation. If we were to add IPE_OP_INVLIAD to the enum
> definition, then IPE_OP_MAX-1 would become the number of operations,
> and we would need to change the index used to access the linked list
> array.
Gotcha. Thanks for the explanation, that hadn't occurred to me while
I was reviewing the code.
Another option would be to create a macro to help reinforce that the
"max" value is being used as an "invalid" value, for example:
#define IPE_OP_INVALID IPE_OP_MAX
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
