Re: [RFC PATCH v9 02/16] ipe: add policy parser

On Thu, Mar 02, 2023 at 02:02:32PM -0500, Paul Moore wrote: > On Mon, Jan 30, 2023 at 5:58???PM Fan Wu <wufan(a)linux.microsoft.com&gt; wrote: > > > > From: Deven Bowers <deven.desai(a)linux.microsoft.com&gt; > > > > 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(a)linux.microsoft.com&gt; > > Signed-off-by: Fan Wu <wufan(a)linux.microsoft.com&gt; > > ... > > > --- > > security/ipe/Makefile | 2 + > > security/ipe/policy.c | 99 +++++++ > > security/ipe/policy.h | 77 ++++++ > > security/ipe/policy_parser.c | 515 +++++++++++++++++++++++++++++++++++ > > security/ipe/policy_parser.h | 11 + > > 5 files changed, 704 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_parser.c b/security/ipe/policy_parser.c > > new file mode 100644 > > index 000000000000..c7ba0e865366 > > --- /dev/null > > +++ b/security/ipe/policy_parser.c > > @@ -0,0 +1,515 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (C) Microsoft Corporation. All rights reserved. > > + */ > > + > > +#include "policy.h" > > +#include "policy_parser.h" > > +#include "digest.h" > > + > > +#include <linux/parser.h> > > + > > +#define START_COMMENT '#' > > + > > +/** > > + * new_parsed_policy - Allocate and initialize a parsed policy. > > + * > > + * Return: > > + * * !IS_ERR - OK > > + * * -ENOMEM - Out of memory > > + */ > > +static struct ipe_parsed_policy *new_parsed_policy(void) > > +{ > > + size_t i = 0; > > + struct ipe_parsed_policy *p = NULL; > > + struct ipe_op_table *t = NULL; > > + > > + p = kzalloc(sizeof(*p), GFP_KERNEL); > > + if (!p) > > + return ERR_PTR(-ENOMEM); > > + > > + p->global_default_action = ipe_action_max; > > I'm assuming you're using the "ipe_action_max" as an intentional bogus > placeholder value here, yes? If that is the case, have you considered > creating an "invalid" enum with an explicit zero value to save you > this additional assignment (you are already using kzalloc())? For > example: > > enum ipe_op_type { > IPE_OP_INVALID = 0, > IPE_OP_EXEC, > ... > IPE_OP_MAX, > }; > > enum ipe_action_type { > IPE_ACTION_INVALID = 0, > IPE_ACTION_ALLOW, > ... > IPE_ACTION_MAX, > }; > Yes, IPE_ACTION_MAX is kind of the INVALID value we are using here. But I think we might be adding unnecessary complexity by using the IPE_OP_INVLIAD enum here. Currently, we are using IPE_OP_MAX to represent the number of operations we have, and we have allocated an IPE_OP_MAX-sized array to store linked lists that link all rules for each operation. If we were to add IPE_OP_INVLIAD to the enum definition, then IPE_OP_MAX-1 would become the number of operations, and we would need to change the index used to access the linked list array.