On Wed, Oct 25, 2023 at 6:46 PM Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> wrote:
> On 10/23/2023 8:52 PM, Paul Moore wrote:
> > On Oct 4, 2023 Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> wrote:
> >>
> >> IPE's interpretation of the what the user trusts is accomplished through
> >> its policy. IPE's design is to not provide support for a single trust
> >> provider, but to support multiple providers to enable the end-user to
> >> choose the best one to seek their needs.
> >>
> >> This requires the policy to be rather flexible and modular so that
> >> integrity providers, like fs-verity, dm-verity, dm-integrity, or
> >> some other system, can plug into the policy with minimal code changes.
> >>
> >> Signed-off-by: Deven Bowers <deven.desai@xxxxxxxxxxxxxxxxxxx>
> >> Signed-off-by: Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx>
> ...
> >> ---
> >> security/ipe/Makefile | 2 +
> >> security/ipe/policy.c | 101 ++++++++
> >> security/ipe/policy.h | 83 ++++++
> >> security/ipe/policy_parser.c | 487 +++++++++++++++++++++++++++++++++++
> >> security/ipe/policy_parser.h | 11 +
> >> 5 files changed, 684 insertions(+)
> >> create mode 100644 security/ipe/policy.c
> >> create mode 100644 security/ipe/policy.h
> >> create mode 100644 security/ipe/policy_parser.c
> >> create mode 100644 security/ipe/policy_parser.h
> >
> > ...
> >
> >> diff --git a/security/ipe/policy.c b/security/ipe/policy.c
> >> new file mode 100644
> >> index 000000000000..3a529c7950a1
> >> --- /dev/null
> >> +++ b/security/ipe/policy.c
> >> @@ -0,0 +1,101 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (C) Microsoft Corporation. All rights reserved.
> >> + */
> >
> > ...
> >
> >> +static int set_pkcs7_data(void *ctx, const void *data, size_t len,
> >> + size_t asn1hdrlen)
> >> +{
> >> + struct ipe_policy *p = ctx;
> >> +
> >> + p->text = (const char *)data;
> >> + p->textlen = len;
> >> +
> >> + return 0;
> >> +}
> >
> > The @asn1hdrlen parameter isn't used in this function, at least at this
> > point in the patchset, so you really should remove it. If it is needed
> > later in the patchset you can always update the function.
>
> Although the @asn1hdrlen is not used, it's a required parameter for the
> pkcs7 callback. I guess adding a __always_unused might be the right
> solution?
Ah gotcha, I'm sorry for the noise, I obviously didn't catch that. As
for the __always_unused marking, yes, that's probably a good idea.
> >> +/**
> >> + * parse_rule - parse a policy rule line.
> >> + * @line: Supplies rule line to be parsed.
> >> + * @p: Supplies the partial parsed policy.
> >> + *
> >> + * Return:
> >> + * * !IS_ERR - OK
> >> + * * -ENOMEM - Out of memory
> >> + * * -EBADMSG - Policy syntax error
> >> + */
> >> +static int parse_rule(char *line, struct ipe_parsed_policy *p)
> >> +{
> >> + int rc = 0;
> >> + bool first_token = true, is_default_rule = false;
> >> + bool op_parsed = false;
> >> + enum ipe_op_type op = IPE_OP_INVALID;
> >> + enum ipe_action_type action = IPE_ACTION_INVALID;
> >> + struct ipe_rule *r = NULL;
> >> + char *t;
> >> +
> >> + r = kzalloc(sizeof(*r), GFP_KERNEL);
> >> + if (!r)
> >> + return -ENOMEM;
> >> +
> >> + INIT_LIST_HEAD(&r->next);
> >> + INIT_LIST_HEAD(&r->props);
> >> +
> >> + while (t = strsep(&line, IPE_POLICY_DELIM), line) {
> >> + if (*t == '\0')
> >> + continue;
> >> + if (first_token && token_default(t)) {
> >> + is_default_rule = true;
> >> + } else {
> >> + if (!op_parsed) {
> >> + op = parse_operation(t);
> >> + if (op == IPE_OP_INVALID)
> >> + rc = -EBADMSG;
> >> + else
> >> + op_parsed = true;
> >> + } else {
> >> + rc = parse_property(t, r);
> >> + }
> >> + }
> >> +
> >> + if (rc)
> >> + goto err;
> >> + first_token = false;
> >> + }
> >> +
> >> + action = parse_action(t);
> >> + if (action == IPE_ACTION_INVALID) {
> >> + rc = -EBADMSG;
> >> + goto err;
> >> + }
> >> +
> >> + if (is_default_rule) {
> >> + if (!list_empty(&r->props)) {
> >> + rc = -EBADMSG;
> >> + } else if (op == IPE_OP_INVALID) {
> >> + if (p->global_default_action != IPE_ACTION_INVALID)
> >> + rc = -EBADMSG;
> >> + else
> >> + p->global_default_action = action;
> >> + } else {
> >> + if (p->rules[op].default_action != IPE_ACTION_INVALID)
> >> + rc = -EBADMSG;
> >> + else
> >> + p->rules[op].default_action = action;
> >> + }
> >> + } else if (op != IPE_OP_INVALID && action != IPE_ACTION_INVALID) {
> >> + r->op = op;
> >> + r->action = action;
> >> + } else {
> >> + rc = -EBADMSG;
> >> + }
> >
> > I might be missing something important in the policy syntac, but could
> > this function, and perhaps the ipe_parsed_policy struct, be simplified
> > if the default action was an explicit rule?
> >
> > "op=DEFAULT action=ALLOW"
>
> The complexity here arises from our need for two types of default rules:
> one for global settings and another for operation-specific settings.
>
> To illustrate the flexibility of operation-specific default rules, users
> can set their policy to have a default rule of 'DENY' for execution,
> meaning all execution actions are prohibited by default. This let users
> to create an allow-list for execution. At the same time, the default
> rule for read can be set to 'ALLOW'. This let users to create an
> deny-list for read.
>
> Adding explicit default rules can simplify ipe_parsed_policy struct, but
> that impose a burden on users that requires users always add the default
> rules the end of the policy. In contrast, our current design allows
> users to place the default rule anywhere in the policy. In practice, we
> often position the default rule at the beginning of the policy, which
> makes it more convenient for users to add new rules.
Okay, thanks for the explanation.
> >> +/**
> >> + * free_parsed_policy - free a parsed policy structure.
> >> + * @p: Supplies the parsed policy.
> >> + */
> >> +void free_parsed_policy(struct ipe_parsed_policy *p)
> >> +{
> >> + size_t i = 0;
> >> + struct ipe_rule *pp, *t;
> >> +
> >> + if (IS_ERR_OR_NULL(p))
> >> + return;
> >> +
> >> + for (i = 0; i < ARRAY_SIZE(p->rules); ++i)
> >> + list_for_each_entry_safe(pp, t, &p->rules[i].rules, next) {
> >> + list_del(&pp->next);
> >> + free_rule(pp);
> >> + }
> >> +
> >> + kfree(p->name);
> >> + kfree(p);
> >> +}
> >> +
> >> +/**
> >> + * validate_policy - validate a parsed policy.
> >> + * @p: Supplies the fully parsed policy.
> >> + *
> >> + * Given a policy structure that was just parsed, validate that all
> >> + * necessary fields are present, initialized correctly.
> >> + *
> >> + * A parsed policy can be in an invalid state for use (a default was
> >> + * undefined) by just parsing the policy.
> >> + *
> >> + * Return:
> >> + * * 0 - OK
> >> + * * -EBADMSG - Policy is invalid
> >> + */
> >> +static int validate_policy(const struct ipe_parsed_policy *p)
> >> +{
> >> + size_t i = 0;
> >> +
> >> + if (p->global_default_action != IPE_ACTION_INVALID)
> >> + return 0;
> >
> > Should the if conditional above be "==" and not "!="?
>
> >No, "!=" is the correct one.
>
> The purpose of validation is to ensure that we have enough default rules
> to cover all cases. If the global default action not invalid, it means
> we have a global default rule in the policy to cover all cases, thus we
> simply return 0.
>
> However, if there is no global default rule, then we need to ensure that
> for each operation, there is a operation specific default rule, this is
> validated in the for loop below.
Makes sense, thanks.
--
paul-moore.com
