[RFC,v10,03/17] ipe: add evaluation loop

Commit Message

Fan Wu June 28, 2023, 9:09 p.m. UTC

From: Deven Bowers <[e-mail]>

IPE must have a centralized function to evaluate incoming callers
against IPE's policy. This iteration of the policy for against the rules
for that specific caller is known as the evaluation loop.

Signed-off-by: Deven Bowers <[e-mail]>
Signed-off-by: Fan Wu <[e-mail]>

---
v2:
+ Split evaluation loop, access control hooks, and evaluation loop from policy parser and userspace interface to pass mailing list character limit

v3:
+ Move ipe_load_properties to patch 04.
+ Remove useless 0-initializations Prefix extern variables with ipe_
+ Remove kernel module parameters, as these are exposed through sysctls.
+ Add more prose to the IPE base config option help text.
+ Use GFP_KERNEL for audit_log_start.
+ Remove unnecessary caching system.
+ Remove comments from headers
+ Use rcu_access_pointer for rcu-pointer null check
+ Remove usage of reqprot; use prot only.
+Move policy load and activation audit event to 03/12

v4:
+ Remove sysctls in favor of securityfs nodes
+ Re-add kernel module parameters, as these are now exposed through securityfs.
+ Refactor property audit loop to a separate function.

v5:
+ fix minor grammatical errors
+ do not group rule by curly-brace in audit record,
+ reconstruct the exact rule.

v6:
+ No changes

v7:
+ Further split lsm creation into a separate commit from the evaluation loop and audit system, for easier review.
+ Propagating changes to support the new ipe_context structure in the evaluation loop.

v8:
+ Remove ipe_hook enumeration; hooks can be correlated via syscall record.

v9:
+ Remove ipe_context related code and simplify the evaluation loop.

v10:
+ Split eval part and boot_verified part
---
 security/ipe/Makefile |  1 +
 security/ipe/eval.c   | 94 +++++++++++++++++++++++++++++++++++++++++++
 security/ipe/eval.h   | 25 ++++++++++++
 3 files changed, 120 insertions(+)
 create mode 100644 security/ipe/eval.c
 create mode 100644 security/ipe/eval.h

Comments

Paul Moore July 8, 2023, 5:36 a.m. UTC | #1

On Jun 28, 2023 Fan Wu <[e-mail]> wrote:
> 
> IPE must have a centralized function to evaluate incoming callers
> against IPE's policy. This iteration of the policy for against the rules
> for that specific caller is known as the evaluation loop.

Can you rewrite that second sentence, it reads a bit awkward and I'm
unclear as to the meaning.

> Signed-off-by: Deven Bowers <[e-mail]>
> Signed-off-by: Fan Wu <[e-mail]>
> ---
>  security/ipe/Makefile |  1 +
>  security/ipe/eval.c   | 94 +++++++++++++++++++++++++++++++++++++++++++
>  security/ipe/eval.h   | 25 ++++++++++++
>  3 files changed, 120 insertions(+)
>  create mode 100644 security/ipe/eval.c
>  create mode 100644 security/ipe/eval.h

...

> diff --git a/security/ipe/eval.c b/security/ipe/eval.c
> new file mode 100644
> index 000000000000..59144b2ecdda
> --- /dev/null
> +++ b/security/ipe/eval.c
> @@ -0,0 +1,94 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) Microsoft Corporation. All rights reserved.
> + */
> +
> +#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 "ipe.h"
> +#include "eval.h"
> +#include "hooks.h"

There is no "hooks.h" at this point in the patchset.

In order for 'git bisect' to remain useful (and it can be a very handy
tool), we need to ensure that each point in the patchset compiles
cleanly.

> +#include "policy.h"
> +
> +struct ipe_policy __rcu *ipe_active_policy;
> +
> +/**
> + * 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)
> +{
> +	return false;
> +}
> +
> +/**
> + * 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;
> +
> +	rcu_read_lock();
> +
> +	pol = rcu_dereference(ipe_active_policy);
> +	if (!pol) {
> +		rcu_read_unlock();
> +		return 0;
> +	}
> +
> +	if (ctx->op == __IPE_OP_INVALID) {
> +		action = pol->parsed->global_default_action;
> +		goto eval;

It looks like you are missing a rcu_read_unlock() in this case.

Also, given how simplistic the evaluation is in this case, why not
just do it here, saving the assignment, jump, etc.?

  if (ctx->op == INVALID) {
    rcu_read_unlock()
    if (global_action == DENY)
      return -EACCES;
    return 0;
  }

> +	}
> +
> +	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);

Why not break from this loop once evaluate_property() returns false?

> +
> +		if (match)
> +			break;
> +	}
> +
> +	if (match)
> +		action = rule->action;
> +	else if (rules->default_action != __IPE_ACTION_INVALID)
> +		action = rules->default_action;
> +	else
> +		action = pol->parsed->global_default_action;
> +
> +	rcu_read_unlock();
> +eval:
> +	if (action == __IPE_ACTION_DENY)
> +		rc = -EACCES;
> +
> +	return rc;

This can just be 'return 0;' right?

> +}

--
paul-moore.com

Fan Wu July 14, 2023, 8:28 p.m. UTC | #2

On Sat, Jul 08, 2023 at 12:23:01AM -0400, Paul Moore wrote:
> On Jun 28, 2023 Fan Wu <[e-mail]> wrote:
> > 
> > IPE must have a centralized function to evaluate incoming callers
> > against IPE's policy. This iteration of the policy for against the rules
> > for that specific caller is known as the evaluation loop.
> 
> Can you rewrite that second sentence, it reads a bit awkward and I'm
> unclear as to the meaning.
> 
Sure, it is indeed not clear, I might rewrite the whole message in the next version.

> > Signed-off-by: Deven Bowers <[e-mail]>
> > Signed-off-by: Fan Wu <[e-mail]>
> > ---
> >  security/ipe/Makefile |  1 +
> >  security/ipe/eval.c   | 94 +++++++++++++++++++++++++++++++++++++++++++
> >  security/ipe/eval.h   | 25 ++++++++++++
> >  3 files changed, 120 insertions(+)
> >  create mode 100644 security/ipe/eval.c
> >  create mode 100644 security/ipe/eval.h
> 
> ...
> 
> > diff --git a/security/ipe/eval.c b/security/ipe/eval.c
> > new file mode 100644
> > index 000000000000..59144b2ecdda
> > --- /dev/null
> > +++ b/security/ipe/eval.c
> > @@ -0,0 +1,94 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) Microsoft Corporation. All rights reserved.
> > + */
> > +
> > +#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 "ipe.h"
> > +#include "eval.h"
> > +#include "hooks.h"
> 
> There is no "hooks.h" at this point in the patchset.
> 
> In order for 'git bisect' to remain useful (and it can be a very handy
> tool), we need to ensure that each point in the patchset compiles
> cleanly.
> 
Sorry for the mistake here, I will avoid this kind of problems in the future.

> > +#include "policy.h"
> > +
> > +struct ipe_policy __rcu *ipe_active_policy;
> > +
> > +/**
> > + * 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)
> > +{
> > +	return false;
> > +}
> > +
> > +/**
> > + * 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;
> > +
> > +	rcu_read_lock();
> > +
> > +	pol = rcu_dereference(ipe_active_policy);
> > +	if (!pol) {
> > +		rcu_read_unlock();
> > +		return 0;
> > +	}
> > +
> > +	if (ctx->op == __IPE_OP_INVALID) {
> > +		action = pol->parsed->global_default_action;
> > +		goto eval;
> 
> It looks like you are missing a rcu_read_unlock() in this case.
> 
Thanks for pointing that out. I will add the unlock. 
> Also, given how simplistic the evaluation is in this case, why not
> just do it here, saving the assignment, jump, etc.?
> 
>   if (ctx->op == INVALID) {
>     rcu_read_unlock()
>     if (global_action == DENY)
>       return -EACCES;
>     return 0;
>   }
> 
The jump is actually for auditing the decision in the next patch, while
it does make more sense to not jump when the auditing is not introduced. 
I will make the return here then switch to jump in the auditing patch.

> > +	}
> > +
> > +	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);
> 
> Why not break from this loop once evaluate_property() returns false?
> 
Yes we can break when one property evals to false, thanks for the advice.

> > +
> > +		if (match)
> > +			break;
> > +	}
> > +
> > +	if (match)
> > +		action = rule->action;
> > +	else if (rules->default_action != __IPE_ACTION_INVALID)
> > +		action = rules->default_action;
> > +	else
> > +		action = pol->parsed->global_default_action;
> > +
> > +	rcu_read_unlock();
> > +eval:
> > +	if (action == __IPE_ACTION_DENY)
> > +		rc = -EACCES;
> > +
> > +	return rc;
> 
> This can just be 'return 0;' right?
> 
For this patch, if we just return on error, then yes the end of the function
could just return 0. But when auditing(audit rc) and permissive switch(overwrite rc)
are introduced then return on error might not be the clean way. So I kept
the rc variable in this patch. I can change the style in this patch then
switch to use rc when auditing and permissive switch are introduced.

-Fan

> > +}
> 
> --
> paul-moore.com

Patch

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..59144b2ecdda
--- /dev/null
+++ b/security/ipe/eval.c
@@ -0,0 +1,94 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) Microsoft Corporation. All rights reserved.
+ */
+
+#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 "ipe.h"
+#include "eval.h"
+#include "hooks.h"
+#include "policy.h"
+
+struct ipe_policy __rcu *ipe_active_policy;
+
+/**
+ * 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)
+{
+	return false;
+}
+
+/**
+ * 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;
+
+	rcu_read_lock();
+
+	pol = rcu_dereference(ipe_active_policy);
+	if (!pol) {
+		rcu_read_unlock();
+		return 0;
+	}
+
+	if (ctx->op == __IPE_OP_INVALID) {
+		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_INVALID)
+		action = rules->default_action;
+	else
+		action = pol->parsed->global_default_action;
+
+	rcu_read_unlock();
+eval:
+	if (action == __IPE_ACTION_DENY)
+		rc = -EACCES;
+
+	return rc;
+}
diff --git a/security/ipe/eval.h b/security/ipe/eval.h
new file mode 100644
index 000000000000..972580dfec15
--- /dev/null
+++ b/security/ipe/eval.h
@@ -0,0 +1,25 @@ 
+/* 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;
+};
+
+int ipe_evaluate_event(const struct ipe_eval_ctx *const ctx);
+
+#endif /* _IPE_EVAL_H */