Re: [PATCH RFC v10 2/17] ipe: add policy parser

[Fan Wu]

On Sat, Jul 08, 2023 at 12:23:00AM -0400, Paul Moore wrote:
> On Jun 28, 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        |  97 +++++++
> >  security/ipe/policy.h        |  83 ++++++
> >  security/ipe/policy_parser.c | 488 +++++++++++++++++++++++++++++++++++
> >  security/ipe/policy_parser.h |  11 +
> >  5 files changed, 681 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..4069ff075093
> > --- /dev/null
> > +++ b/security/ipe/policy.c
> > @@ -0,0 +1,97 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) Microsoft Corporation. All rights reserved.
> > + */
> > +
> > +#include <linux/errno.h>
> > +#include <linux/verification.h>
> > +
> > +#include "ipe.h"
> > +#include "policy.h"
> > +#include "policy_parser.h"
> > +
> > +/**
> > + * ipe_free_policy - Deallocate a given IPE policy.
> > + * @p: Supplies the policy to free.
> > + *
> > + * Safe to call on IS_ERR/NULL.
> > + */
> > +void ipe_free_policy(struct ipe_policy *p)
> > +{
> > +	if (IS_ERR_OR_NULL(p))
> > +		return;
> > +
> > +	free_parsed_policy(p->parsed);
> > +	if (!p->pkcs7)
> > +		kfree(p->text);
> 
> Since it's safe to kfree(NULL), you could kfree(p->text) without
> having to check if p->pkcs7 was non-NULL, correct?
> 
when p->pkcs7 is not NULL, p->text points to the plain text policy area inside
the data of p->pkcs7, for such cases p->text is not really an allocated memory chunk
so it cannot be passed to kfree.

I might better add a comment here to avoid confusion in the future.

> > +	kfree(p->pkcs7);
> > +	kfree(p);
> > +}
> 
> ...
> 
> > diff --git a/security/ipe/policy.h b/security/ipe/policy.h
> > new file mode 100644
> > index 000000000000..113a037f0d71
> > --- /dev/null
> > +++ b/security/ipe/policy.h
> > @@ -0,0 +1,83 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) Microsoft Corporation. All rights reserved.
> > + */
> > +#ifndef _IPE_POLICY_H
> > +#define _IPE_POLICY_H
> > +
> > +#include <linux/list.h>
> > +#include <linux/types.h>
> > +
> > +enum ipe_op_type {
> > +	__IPE_OP_EXEC = 0,
> > +	__IPE_OP_FIRMWARE,
> > +	__IPE_OP_KERNEL_MODULE,
> > +	__IPE_OP_KEXEC_IMAGE,
> > +	__IPE_OP_KEXEC_INITRAMFS,
> > +	__IPE_OP_IMA_POLICY,
> > +	__IPE_OP_IMA_X509,
> > +	__IPE_OP_MAX
> > +};
> 
> Thanks for capitalizing the enums, that helps make IPE consistent with
> the majority of the kernel.  However, when I talked about using
> underscores for "__IPE_OP_MAX", I was talking about *only*
> "__IPE_OP_MAX" to help indicate it is a sentinel value and not an enum
> value that would normally be used by itself.
> 
> Here is what I was intending:
> 
> enum ipe_op_type {
>   IPE_OP_EXEC = 0,
>   IPE_OP_FIRMWARE,
>   ...
>   IPE_OP_IMA_X509,
>   __IPE_OP_MAX
> };
> 
> > +#define __IPE_OP_INVALID __IPE_OP_MAX
> 
> Similarly, I would remove the underscores from "__IPE_OP_INVALID":
> 
> #define IPE_OP_INVALID __IPE_OP_MAX
> 
> Both of these comments would apply to the other IPE enums as well.
> 
Sorry for the mistake, I will update them.

> > diff --git a/security/ipe/policy_parser.c b/security/ipe/policy_parser.c
> > new file mode 100644
> > index 000000000000..27e5767480b0
> > --- /dev/null
> > +++ b/security/ipe/policy_parser.c
> > @@ -0,0 +1,488 @@
> 
> ...
> 
> > +/**
> > + * parse_header - Parse policy header information.
> > + * @line: Supplies header line to be parsed.
> > + * @p: Supplies the partial parsed policy.
> > + *
> > + * Return:
> > + * * 0	- OK
> > + * * !0	- Standard errno
> > + */
> > +static int parse_header(char *line, struct ipe_parsed_policy *p)
> > +{
> > +	int rc = 0;
> > +	char *t, *ver = NULL;
> > +	substring_t args[MAX_OPT_ARGS];
> > +	size_t idx = 0;
> > +
> > +	while ((t = strsep(&line, " \t")) != NULL) {
> 
> It might be nice to define a macro to help reinforce that " \t" are
> the IPE policy delimiters, how about IPE_POLICY_DELIM?
> 
> #define IPE_POLICY_DELIM " \t"
> 
Sure, this is better, I will take this idea.

> > +		int token;
> > +
> > +		if (*t == '\0')
> > +			continue;
> 
> Why would you want to continue if you run into a NUL byte?  You would
> only run into a NUL byte if the line was trimmed due to comments or
> whitespace, correct?  If that is the case, wouldn't you want to
> break out of this loop when hitting a NUL byte?
> 
This happens when two spaces are passed, for example "DEFAULT<space><space>action=DENY"
has two spaces inside, the strsep will create a NUL string when it sees the first space,
so for such cases I think we should just skip to the next token.

> > +		if (idx >= __IPE_HEADER_MAX) {
> > +			rc = -EBADMSG;
> > +			goto err;
> > +		}
> > +
> > +		token = match_token(t, header_tokens, args);
> > +		if (token != idx) {
> > +			rc = -EBADMSG;
> > +			goto err;
> > +		}
> > +
> > +		switch (token) {
> > +		case __IPE_HEADER_POLICY_NAME:
> > +			p->name = match_strdup(&args[0]);
> > +			if (!p->name)
> > +				rc = -ENOMEM;
> > +			break;
> > +		case __IPE_HEADER_POLICY_VERSION:
> > +			ver = match_strdup(&args[0]);
> > +			if (!ver) {
> > +				rc = -ENOMEM;
> > +				break;
> > +			}
> > +			rc = parse_version(ver, p);
> > +			break;
> > +		default:
> > +			rc = -EBADMSG;
> > +		}
> > +		if (rc)
> > +			goto err;
> > +		++idx;
> > +	}
> > +
> > +	if (idx != __IPE_HEADER_MAX) {
> > +		rc = -EBADMSG;
> > +		goto err;
> > +	}
> > +
> > +out:
> > +	kfree(ver);
> > +	return rc;
> > +err:
> > +	kfree(p->name);
> > +	p->name = NULL;
> > +	goto out;
> 
> Do we need to worry about ipe_parsed_policy::name here?  If we are
> returning an error the caller will call free_parsed_policy() for us,
> right?  This would allow us to get rid of the 'err' jump label and
> simply use 'out' for both success and failure.
> 
Yes this is not necessary, I will remove this part.

> > +}
> 
> ...
> 
> > +/**
> > + * 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, " \t"), line) {
> 
> See my previous comment about IPE_POLICY_DELIM.
> 
> > +		if (*t == '\0')
> > +			continue;
> 
> I still wonder why continuing here is the desired behavior, can you
> help me understand?
This one is the same to the parse header function, when two consecutive
delimitators is passed to strsep it will generate a '\0'.

> 
> > +		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;
> > +	}
> > +
> > +	if (rc)
> > +		goto err;
> > +	if (!is_default_rule)
> > +		list_add_tail(&r->next, &p->rules[op].rules);
> > +	else
> > +		free_rule(r);
> > +
> > +out:
> > +	return rc;
> > +err:
> > +	free_rule(r);
> > +	goto out;
> 
> In keeping with the rule of not jumping to a label only to
> immediately return, and considering that the only place where we jump
> to 'out' is in the 'err' code, let's get rid of the 'out' label and
> have 'err' "return rc" instead of "goto out".
> 
Sure I can change this part, yeah I agree this looks weird. 

-Fan
> > +}
> 
> --
> paul-moore.com