Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 08 Jul 2023 01:36:55 -0400
Message-ID: <d4b1522c467839c02d38ff9da99ebbd3.paul@paul-moore.com>
From:   Paul Moore <paul@paul-moore.com>
To:     Fan Wu <wufan@linux.microsoft.com>, corbet@lwn.net,
        zohar@linux.ibm.com, jmorris@namei.org, serge@hallyn.com,
        tytso@mit.edu, ebiggers@kernel.org, axboe@kernel.dk,
        agk@redhat.com, snitzer@kernel.org, eparis@redhat.com
Cc:     linux-doc@vger.kernel.org, linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        linux-fscrypt@vger.kernel.org, linux-block@vger.kernel.org,
        dm-devel@redhat.com, audit@vger.kernel.org,
        roberto.sassu@huawei.com, linux-kernel@vger.kernel.org,
        Deven Bowers <deven.desai@linux.microsoft.com>,
        Fan Wu <wufan@linux.microsoft.com>
Subject: Re: [PATCH RFC v10 2/17] ipe: add policy parser
References: <1687986571-16823-3-git-send-email-wufan@linux.microsoft.com>
In-Reply-To: <1687986571-16823-3-git-send-email-wufan@linux.microsoft.com>
Precedence: bulk

On Jun 28, 2023 Fan Wu <wufan@linux.microsoft.com> 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@linux.microsoft.com>
> Signed-off-by: Fan Wu <wufan@linux.microsoft.com>
> ---
>  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?

> +	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.

> 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"

> +		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?

> +		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.

> +}

...

> +/**
> + * 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?

> +		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".

> +}

--
paul-moore.com