lkml.org 
[lkml]   [2023]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [RFC PATCH v9 03/16] ipe: add evaluation loop and introduce 'boot_verified' as a trust provider
    On Mon, Jan 30, 2023 at 5:58 PM Fan Wu <wufan@linux.microsoft.com> wrote:
    >
    > From: Deven Bowers <deven.desai@linux.microsoft.com>
    >
    > 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 <deven.desai@linux.microsoft.com>
    > Signed-off-by: Fan Wu <wufan@linux.microsoft.com>

    ...

    > ---
    > 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/Makefile b/security/ipe/Makefile
    > index 16bbe80991f1..d7f2870d7c09 100644
    > --- a/security/ipe/Makefile
    > +++ b/security/ipe/Makefile
    > @@ -6,6 +6,7 @@
    > #
    >
    > obj-$(CONFIG_SECURITY_IPE) += \
    > + eval.o \
    > hooks.o \
    > ipe.o \
    > policy.o \
    > diff --git a/security/ipe/eval.c b/security/ipe/eval.c
    > new file mode 100644
    > index 000000000000..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 <linux/fs.h>
    > +#include <linux/types.h>
    > +#include <linux/slab.h>
    > +#include <linux/file.h>
    > +#include <linux/sched.h>
    > +#include <linux/rcupdate.h>
    > +#include <linux/spinlock.h>
    > +
    > +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);
    }

    Also, do we need to worry about the initramfs' being unmounted and the
    super_block going away?

    > +/**
    > + * from_pinned - Determine whether @f is source from the pinned super_block.
    > + * @f: Supplies a file structure to check against the pinned super_block.
    > + *
    > + * Return:
    > + * * true - @f is sourced from the pinned super_block
    > + * * false - @f is not sourced from the pinned super_block
    > + */
    > +static bool from_pinned(const struct file *f)
    > +{
    > + bool rv;
    > +
    > + if (!f)
    > + return false;
    > + spin_lock(&pin_lock);
    > + rv = !IS_ERR_OR_NULL(pinned_sb) && pinned_sb == FILE_SUPERBLOCK(f);
    > + spin_unlock(&pin_lock);
    > + return rv;
    > +}
    > +
    > +/**
    > + * build_eval_ctx - Build an evaluation context.
    > + * @ctx: Supplies a pointer to the context to be populdated.
    > + * @file: Supplies a pointer to the file to associated with the evaluation.
    > + * @op: Supplies the IPE policy operation associated with the evaluation.
    > + */
    > +void build_eval_ctx(struct ipe_eval_ctx *ctx,
    > + const struct file *file,
    > + enum ipe_op_type op)
    > +{
    > + ctx->file = file;
    > + ctx->op = op;
    > + ctx->from_init_sb = from_pinned(file);
    > +}

    I was a little concerned about the spinlock around the pinned
    superblock being a potential issue so I was checking the callers of
    `build_eval_ctx()` and realized there are no callers in this patch ...
    ? Maybe it makes sense for `build_eval_ctx()` to be in this patch but
    it seems a little odd.

    > +/**
    > + * evaluate_property - Analyze @ctx against a property.
    > + * @ctx: Supplies a pointer to the context to be evaluated.
    > + * @p: Supplies a pointer to the property to be evaluated.
    > + *
    > + * Return:
    > + * * true - The current @ctx match the @p
    > + * * false - The current @ctx doesn't match the @p
    > + */
    > +static bool evaluate_property(const struct ipe_eval_ctx *const ctx,
    > + struct ipe_prop *p)
    > +{
    > + bool eval = false;
    > +
    > + switch (p->type) {
    > + case ipe_prop_boot_verified_false:
    > + eval = !ctx->from_init_sb;
    > + break;
    > + case ipe_prop_boot_verified_true:
    > + eval = ctx->from_init_sb;
    > + break;
    > + default:
    > + eval = false;

    You don't need to set @eval to false both when it is declared or in
    the 'default' case.

    Honestly, you don't need @eval at all, you can simply replace all of
    the @eval assignment statements with return statements.

    > + }
    > +
    > + return eval;
    > +}
    > +
    > +/**
    > + * ipe_evaluate_event - Analyze @ctx against the current active policy.
    > + * @ctx: Supplies a pointer to the context to be evaluated.
    > + *
    > + * This is the loop where all policy evaluation happens against IPE policy.
    > + *
    > + * Return:
    > + * * 0 - OK
    > + * * -EACCES - @ctx did not pass evaluation.
    > + * * !0 - Error
    > + */
    > +int ipe_evaluate_event(const struct ipe_eval_ctx *const ctx)
    > +{
    > + int rc = 0;
    > + bool match = false;
    > + enum ipe_action_type action;
    > + struct ipe_policy *pol = NULL;
    > + const struct ipe_rule *rule = NULL;
    > + const struct ipe_op_table *rules = NULL;
    > + struct ipe_prop *prop = NULL;
    > +
    > + if (ctx->op == ipe_op_exec)
    > + pin_sb(ctx->file);

    If I understand things correctly, the initramfs is determined by the
    first process to be executed? I think that's reasonable, but I'm
    beginning to wonder if that pinned super_block spinlock is going to be
    a problem, especially for something that is written once (twice if you
    consider the ERR_PTR(-EIO) on umount), yet read for each IPE policy
    evaluation.

    I'm okay if you want to keep this as a spinlock for now, but this
    seems like a good candidate for RCU, and the change would be trivial
    since it is a single pointer.

    > + pol = ipe_get_policy_rcu(ipe_active_policy);

    I don't think you can safely drop the RCU lock and leave the RCU
    critical section while you are still using @ipe_active_policy. I
    think the right thing to do is to get rid of `ipe_get_policy_rcu()`
    and simply place from here on down in `ipe_evaluate_event()` in a RCU
    critical section. Doing so would ensure that @ipe_active_policy could
    not be free'd/replaced from underneath you while evaluating an event.

    > + if (!pol)
    > + goto out;
    > +
    > + if (ctx->op == ipe_op_max) {
    > + action = pol->parsed->global_default_action;
    > + goto eval;
    > + }
    > +
    > + rules = &pol->parsed->rules[ctx->op];
    > +
    > + list_for_each_entry(rule, &rules->rules, next) {
    > + match = true;
    > +
    > + list_for_each_entry(prop, &rule->props, next)
    > + match = match && evaluate_property(ctx, prop);
    > +
    > + if (match)
    > + break;
    > + }
    > +
    > + if (match)
    > + action = rule->action;
    > + else if (rules->default_action != ipe_action_max)
    > + action = rules->default_action;
    > + else
    > + action = pol->parsed->global_default_action;
    > +
    > +eval:
    > + if (action == ipe_action_deny)
    > + rc = -EACCES;
    > +
    > +out:
    > + return rc;
    > +}
    > +
    > +/**
    > + * ipe_invalidate_pinned_sb - invalidte the ipe pinned super_block.
    > + * @mnt_sb: super_block to check against the pinned super_block.
    > + *
    > + * This function is called a super_block like the initramfs's is freed,
    > + * if the super_block is currently pinned by ipe it will be invalided,
    > + * so ipe won't consider the block device is boot verified afterward.
    > + */
    > +void ipe_invalidate_pinned_sb(const struct super_block *mnt_sb)
    > +{
    > + spin_lock(&pin_lock);
    > +
    > + if (!IS_ERR_OR_NULL(pinned_sb) && mnt_sb == pinned_sb)
    > + pinned_sb = ERR_PTR(-EIO);

    I think you only need to check if @pinned_sb is equal to @mnt_sb,
    that's all that really matters here.

    > + spin_unlock(&pin_lock);
    > +}
    > diff --git a/security/ipe/eval.h b/security/ipe/eval.h
    > new file mode 100644
    > index 000000000000..887797438b9b
    > --- /dev/null
    > +++ b/security/ipe/eval.h
    > @@ -0,0 +1,28 @@
    > +/* SPDX-License-Identifier: GPL-2.0 */
    > +/*
    > + * Copyright (C) Microsoft Corporation. All rights reserved.
    > + */
    > +
    > +#ifndef IPE_EVAL_H
    > +#define IPE_EVAL_H
    > +
    > +#include <linux/file.h>
    > +#include <linux/types.h>
    > +
    > +#include "hooks.h"
    > +#include "policy.h"
    > +
    > +extern struct ipe_policy __rcu *ipe_active_policy;
    > +
    > +struct ipe_eval_ctx {
    > + enum ipe_op_type op;
    > +
    > + const struct file *file;
    > + bool from_init_sb;
    > +};
    > +
    > +void build_eval_ctx(struct ipe_eval_ctx *ctx, const struct file *file, enum ipe_op_type op);
    > +int ipe_evaluate_event(const struct ipe_eval_ctx *const ctx);
    > +void ipe_invalidate_pinned_sb(const struct super_block *mnt_sb);
    > +
    > +#endif /* IPE_EVAL_H */
    > diff --git a/security/ipe/hooks.c b/security/ipe/hooks.c
    > new file mode 100644
    > index 000000000000..335b773c7ae1
    > --- /dev/null
    > +++ b/security/ipe/hooks.c
    > @@ -0,0 +1,25 @@
    > +// SPDX-License-Identifier: GPL-2.0
    > +/*
    > + * Copyright (C) Microsoft Corporation. All rights reserved.
    > + */
    > +
    > +#include "ipe.h"
    > +#include "hooks.h"
    > +#include "eval.h"
    > +
    > +#include <linux/fs.h>
    > +#include <linux/types.h>
    > +#include <linux/binfmts.h>
    > +#include <linux/mman.h>
    > +
    > +/**
    > + * ipe_sb_free_security - ipe security hook function for super_block.
    > + * @mnt_sb: Supplies a pointer to a super_block is about to be freed.
    > + *
    > + * IPE does not have any structures with mnt_sb, but uses this hook to
    > + * invalidate a pinned super_block.
    > + */
    > +void ipe_sb_free_security(struct super_block *mnt_sb)
    > +{
    > + ipe_invalidate_pinned_sb(mnt_sb);
    > +}
    > diff --git a/security/ipe/hooks.h b/security/ipe/hooks.h
    > new file mode 100644
    > index 000000000000..30fe455389bf
    > --- /dev/null
    > +++ b/security/ipe/hooks.h
    > @@ -0,0 +1,14 @@
    > +/* SPDX-License-Identifier: GPL-2.0 */
    > +/*
    > + * Copyright (C) Microsoft Corporation. All rights reserved.
    > + */
    > +#ifndef IPE_HOOKS_H
    > +#define IPE_HOOKS_H
    > +
    > +#include <linux/fs.h>
    > +#include <linux/binfmts.h>
    > +#include <linux/security.h>
    > +
    > +void ipe_sb_free_security(struct super_block *mnt_sb);
    > +
    > +#endif /* IPE_HOOKS_H */
    > diff --git a/security/ipe/ipe.c b/security/ipe/ipe.c
    > index 9ed3bf4dcc04..551c6d90ac11 100644
    > --- a/security/ipe/ipe.c
    > +++ b/security/ipe/ipe.c
    > @@ -9,6 +9,7 @@ static struct lsm_blob_sizes ipe_blobs __lsm_ro_after_init = {
    > };
    >
    > static struct security_hook_list ipe_hooks[] __lsm_ro_after_init = {
    > + LSM_HOOK_INIT(sb_free_security, ipe_sb_free_security),
    > };
    >
    > /**
    > diff --git a/security/ipe/policy.c b/security/ipe/policy.c
    > index e446f4b84152..772d876b1087 100644
    > --- a/security/ipe/policy.c
    > +++ b/security/ipe/policy.c
    > @@ -97,3 +97,23 @@ struct ipe_policy *ipe_new_policy(const char *text, size_t textlen,
    > err:
    > return ERR_PTR(rc);
    > }
    > +
    > +/**
    > + * ipe_get_policy_rcu - Dereference a rcu-protected policy pointer.
    > + *
    > + * @p: rcu-protected pointer to a policy.
    > + *
    > + * Not safe to call on IS_ERR.
    > + *
    > + * Return: the value of @p
    > + */
    > +struct ipe_policy *ipe_get_policy_rcu(struct ipe_policy __rcu *p)
    > +{
    > + struct ipe_policy *rv = NULL;
    > +
    > + rcu_read_lock();
    > + rv = rcu_dereference(p);
    > + rcu_read_unlock();
    > +
    > + return rv;
    > +}
    > diff --git a/security/ipe/policy.h b/security/ipe/policy.h
    > index 6af2d9a811ec..967d816cd5cd 100644
    > --- a/security/ipe/policy.h
    > +++ b/security/ipe/policy.h
    > @@ -26,6 +26,8 @@ enum ipe_action_type {
    > };
    >
    > enum ipe_prop_type {
    > + ipe_prop_boot_verified_false,
    > + ipe_prop_boot_verified_true,
    > ipe_prop_max
    > };
    >
    > @@ -73,5 +75,6 @@ struct ipe_policy {
    > 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);
    >
    > #endif /* IPE_POLICY_H */
    > diff --git a/security/ipe/policy_parser.c b/security/ipe/policy_parser.c
    > index c7ba0e865366..7efafc482e46 100644
    > --- a/security/ipe/policy_parser.c
    > +++ b/security/ipe/policy_parser.c
    > @@ -265,7 +265,9 @@ static enum ipe_action_type parse_action(char *t)
    > }
    >
    > static const match_table_t property_tokens = {
    > - {ipe_prop_max, NULL}
    > + {ipe_prop_boot_verified_false, "boot_verified=FALSE"},
    > + {ipe_prop_boot_verified_true, "boot_verified=TRUE"},
    > + {ipe_prop_max, NULL}
    > };
    >
    > /**
    > @@ -295,6 +297,10 @@ int parse_property(char *t, struct ipe_rule *r)
    > token = match_token(t, property_tokens, args);
    >
    > switch (token) {
    > + case ipe_prop_boot_verified_false:
    > + case ipe_prop_boot_verified_true:
    > + p->type = token;
    > + break;
    > case ipe_prop_max:
    > default:
    > rc = -EBADMSG;
    > --
    > 2.39.0

    --
    paul-moore.com

    \
     
     \ /
      Last update: 2023-03-27 00:42    [W:4.075 / U:0.568 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site