Re: [PATCH RFC v11 3/19] ipe: add evaluation loop

Fan Wu
2023-10-26T00:15:28Z

On 10/23/2023 8:52 PM, Paul Moore wrote:
> On Oct 4, 2023 Fan Wu <wufan@linux.microsoft.com> 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.
>>
>> 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 | 96 +++++++++++++++++++++++++++++++++++++++++++
>> security/ipe/eval.h | 24 +++++++++++
>> 3 files changed, 121 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..5533c359bbeb
>> --- /dev/null
>> +++ b/security/ipe/eval.c
>> @@ -0,0 +1,96 @@
>> +// 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 "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)
>> +{
>> + 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) {
>> + rcu_read_unlock();
>> + if (pol->parsed->global_default_action == IPE_ACTION_DENY)
>> + return -EACCES;
>
> Assuming that the RCU lock protects @pol, shouldn't it be held until
> after the global_default_action comparison?
>
Yes for this part the unlock should be moved after the comparison.
Thanks for spotting this.

>> + 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);
>
> The @match variable will always be true on the right side above, or am
> I missing something?
>
Yes the "match &&" are completely unnecessary. I will remove them.

-Fan
>> + if (!match)
>> + break;
>> + }
>> +
>> + 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();
>> + if (action == IPE_ACTION_DENY)
>> + return -EACCES;
>> +
>> + return 0;
>> +}
>
> --
> paul-moore.com