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

[Fan Wu <wufan@linux.microsoft.com>]

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