On Jun 28, 2023 Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> 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 <deven.desai@xxxxxxxxxxxxxxxxxxx> > Signed-off-by: Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> > --- > 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
![]() |