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

  From: Deven Bowers <deven.desai@linux.microsoft.com>

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>

---
v2:
  + Split evaluation loop, access control hooks,
    and evaluation loop from policy parser and userspace
    interface to pass mailing list character limit

v3:
  + Move policy load and activation audit event to 03/12
  + Fix a potential panic when a policy failed to load.
  + use pr_warn for a failure to parse instead of an
    audit record
  + Remove comments from headers
  + Add lockdep assertions to ipe_update_active_policy and
    ipe_activate_policy
  + Fix up warnings with checkpatch --strict
  + Use file_ns_capable for CAP_MAC_ADMIN for securityfs
    nodes.
  + Use memdup_user instead of kzalloc+simple_write_to_buffer.
  + Remove strict_parse command line parameter, as it is added
    by the sysctl command line.
  + Prefix extern variables with ipe_

v4:
  + Remove securityfs to reverse-dependency
  + Add SHA1 reverse dependency.
  + Add versioning scheme for IPE properties, and associated
    interface to query the versioning scheme.
  + Cause a parser to always return an error on unknown syntax.
  + Remove strict_parse option
  + Change active_policy interface from sysctl, to securityfs,
    and change scheme.

v5:
  + Cause an error if a default action is not defined for each
    operaiton.
  + Minor function renames

v6:
  + No changes

v7:
  + Further split parser and userspace interface into two
    separate commits, for easier review.

  + Refactor policy parser to make code cleaner via introducing a
    more modular design, for easier extension of policy, and
    easier review.

v8:
  + remove unnecessary pr_info emission on parser loading

  + add explicit newline to the pr_err emitted when a parser
    fails to load.

v9:
  + switch to match table to parse policy

  + remove quote syntax and KERNEL_READ operation
---
 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
  

On Mon, 2023-01-30 at 14:57 -0800, Fan Wu wrote:
> From: Deven Bowers <deven.desai@linux.microsoft.com>
> 
> 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>
> 
> ---
> v2:
>   + Split evaluation loop, access control hooks,
>     and evaluation loop from policy parser and userspace
>     interface to pass mailing list character limit
> 
> v3:
>   + Move policy load and activation audit event to 03/12
>   + Fix a potential panic when a policy failed to load.
>   + use pr_warn for a failure to parse instead of an
>     audit record
>   + Remove comments from headers
>   + Add lockdep assertions to ipe_update_active_policy and
>     ipe_activate_policy
>   + Fix up warnings with checkpatch --strict
>   + Use file_ns_capable for CAP_MAC_ADMIN for securityfs
>     nodes.
>   + Use memdup_user instead of kzalloc+simple_write_to_buffer.
>   + Remove strict_parse command line parameter, as it is added
>     by the sysctl command line.
>   + Prefix extern variables with ipe_
> 
> v4:
>   + Remove securityfs to reverse-dependency
>   + Add SHA1 reverse dependency.
>   + Add versioning scheme for IPE properties, and associated
>     interface to query the versioning scheme.
>   + Cause a parser to always return an error on unknown syntax.
>   + Remove strict_parse option
>   + Change active_policy interface from sysctl, to securityfs,
>     and change scheme.
> 
> v5:
>   + Cause an error if a default action is not defined for each
>     operaiton.
>   + Minor function renames
> 
> v6:
>   + No changes
> 
> v7:
>   + Further split parser and userspace interface into two
>     separate commits, for easier review.
> 
>   + Refactor policy parser to make code cleaner via introducing a
>     more modular design, for easier extension of policy, and
>     easier review.
> 
> v8:
>   + remove unnecessary pr_info emission on parser loading
> 
>   + add explicit newline to the pr_err emitted when a parser
>     fails to load.
> 
> v9:
>   + switch to match table to parse policy
> 
>   + remove quote syntax and KERNEL_READ operation
> ---
>  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/Makefile b/security/ipe/Makefile
> index 571648579991..16bbe80991f1 100644
> --- a/security/ipe/Makefile
> +++ b/security/ipe/Makefile
> @@ -8,3 +8,5 @@
>  obj-$(CONFIG_SECURITY_IPE) += \
>  	hooks.o \
>  	ipe.o \
> +	policy.o \
> +	policy_parser.o \
> diff --git a/security/ipe/policy.c b/security/ipe/policy.c
> new file mode 100644
> index 000000000000..e446f4b84152
> --- /dev/null
> +++ b/security/ipe/policy.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) Microsoft Corporation. All rights reserved.
> + */
> +
> +#include "ipe.h"
> +#include "policy.h"
> +#include "policy_parser.h"
> +#include "digest.h"
> +
> +#include <linux/verification.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);
> +	kfree(p->pkcs7);
> +	kfree(p);
> +}
> +
> +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;
> +}
> +
> +/**
> + * ipe_new_policy - Allocate and parse an ipe_policy structure.
> + *
> + * @text: Supplies a pointer to the plain-text policy to parse.
> + * @textlen: Supplies the length of @text.
> + * @pkcs7: Supplies a pointer to a pkcs7-signed IPE policy.
> + * @pkcs7len: Supplies the length of @pkcs7.
> + *
> + * @text/@textlen Should be NULL/0 if @pkcs7/@pkcs7len is set.
> + *
> + * The result will still need to be associated with a context via
> + * ipe_add_policy.
> + *
> + * Return:
> + * * !IS_ERR	- Success
> + * * -EBADMSG	- Policy is invalid
> + * * -ENOMEM	- Out of memory
> + */
> +struct ipe_policy *ipe_new_policy(const char *text, size_t textlen,
> +				  const char *pkcs7, size_t pkcs7len)
> +{
> +	int rc = 0;
> +	struct ipe_policy *new = NULL;
> +
> +	new = kzalloc(sizeof(*new), GFP_KERNEL);
> +	if (!new)
> +		return ERR_PTR(-ENOMEM);
> +
> +	if (!text) {
> +		new->pkcs7len = pkcs7len;
> +		new->pkcs7 = kmemdup(pkcs7, pkcs7len, GFP_KERNEL);
> +		if (!new->pkcs7) {
> +			rc = -ENOMEM;
> +			goto err;
> +		}

Uhm, memory leak? Also below. I suggest to use kmemleak.

Roberto

> +
> +		rc = verify_pkcs7_signature(NULL, 0, new->pkcs7, pkcs7len, NULL,
> +					    VERIFYING_UNSPECIFIED_SIGNATURE,
> +					    set_pkcs7_data, new);
> +		if (rc)
> +			goto err;
> +	} else {
> +		new->textlen = textlen;
> +		new->text = kstrdup(text, GFP_KERNEL);
> +		if (!new->text) {
> +			rc = -ENOMEM;
> +			goto err;
> +		}
> +	}
> +
> +	rc = parse_policy(new);
> +	if (rc)
> +		goto err;
> +
> +	return new;
> +err:
> +	return ERR_PTR(rc);
> +}
> diff --git a/security/ipe/policy.h b/security/ipe/policy.h
> new file mode 100644
> index 000000000000..6af2d9a811ec
> --- /dev/null
> +++ b/security/ipe/policy.h
> @@ -0,0 +1,77 @@
> +/* 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
> +};
> +
> +enum ipe_action_type {
> +	ipe_action_allow = 0,
> +	ipe_action_deny,
> +	ipe_action_max
> +};
> +
> +enum ipe_prop_type {
> +	ipe_prop_max
> +};
> +
> +struct ipe_prop {
> +	struct list_head next;
> +	enum ipe_prop_type type;
> +	void *value;
> +};
> +
> +struct ipe_rule {
> +	enum ipe_op_type op;
> +	enum ipe_action_type action;
> +	struct list_head props;
> +	struct list_head next;
> +};
> +
> +struct ipe_op_table {
> +	struct list_head rules;
> +	enum ipe_action_type default_action;
> +};
> +
> +struct ipe_parsed_policy {
> +	const char *name;
> +	struct {
> +		u16 major;
> +		u16 minor;
> +		u16 rev;
> +	} version;
> +
> +	enum ipe_action_type global_default_action;
> +
> +	struct ipe_op_table rules[ipe_op_max];
> +};
> +
> +struct ipe_policy {
> +	const char     *pkcs7;
> +	size_t		pkcs7len;
> +
> +	const char     *text;
> +	size_t		textlen;
> +
> +	struct ipe_parsed_policy *parsed;
> +};
> +
> +struct ipe_policy *ipe_new_policy(const char *text, size_t textlen,
> +				  const char *pkcs7, size_t pkcs7len);
> +void ipe_free_policy(struct ipe_policy *pol);
> +
> +#endif /* IPE_POLICY_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;
> +
> +	for (i = 0; i < ARRAY_SIZE(p->rules); ++i) {
> +		t = &p->rules[i];
> +
> +		t->default_action = ipe_action_max;
> +		INIT_LIST_HEAD(&t->rules);
> +	}
> +
> +	return p;
> +}
> +
> +/**
> + * remove_comment - Truncate all chars following START_COMMENT in a string.
> + *
> + * @line: Supplies a poilcy line string for preprocessing.
> + */
> +static void remove_comment(char *line)
> +{
> +	size_t i, len = 0;
> +
> +	len = strlen(line);
> +	for (i = 0; i < len && line[i] != START_COMMENT; ++i)
> +		;
> +
> +	line[i] = '\0';
> +}
> +
> +/**
> + * remove_trailing_spaces - Truncate all trailing spaces in a string.
> + *
> + * @line: Supplies a poilcy line string for preprocessing.
> + */
> +static void remove_trailing_spaces(char *line)
> +{
> +	size_t i, len = 0;
> +
> +	len = strlen(line);
> +	for (i = len; i > 0 && (line[i - 1] == ' ' || line[i - 1] == '\t'); --i)
> +		;
> +
> +	line[i] = '\0';
> +}
> +
> +/**
> + * parse_version - Parse policy version.
> + * @ver: Supplies a version string to be parsed.
> + * @p: Supplies the partial parsed policy.
> + *
> + * Return:
> + * * 0	- OK
> + * * !0	- Standard errno
> + */
> +static int parse_version(char *ver, struct ipe_parsed_policy *p)
> +{
> +	int rc = 0;
> +	size_t sep_count = 0;
> +	char *token;
> +	u16 *const cv[] = { &p->version.major, &p->version.minor, &p->version.rev };
> +
> +	while ((token = strsep(&ver, ".")) != NULL) {
> +		/* prevent overflow */
> +		if (sep_count >= ARRAY_SIZE(cv)) {
> +			rc = -EBADMSG;
> +			goto err;
> +		}
> +
> +		rc = kstrtou16(token, 10, cv[sep_count]);
> +		if (rc)
> +			goto err;
> +
> +		++sep_count;
> +	}
> +
> +	/* prevent underflow */
> +	if (sep_count != ARRAY_SIZE(cv))
> +		rc = -EBADMSG;
> +
> +err:
> +	return rc;
> +}
> +
> +enum header_opt {
> +	ipe_header_policy_name = 0,
> +	ipe_header_policy_version,
> +	ipe_header_max
> +};
> +
> +static const match_table_t header_tokens = {
> +	{ipe_header_policy_name,	"policy_name=%s"},
> +	{ipe_header_policy_version,	"policy_version=%s"},
> +	{ipe_header_max,		NULL}
> +};
> +
> +/**
> + * 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) {
> +		int token;
> +
> +		if (*t == '\0')
> +			continue;
> +		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;
> +	}
> +	goto out;
> +
> +err:
> +	kfree(p->name);
> +	p->name = NULL;
> +out:
> +	kfree(ver);
> +	return rc;
> +}
> +
> +/**
> + * is_default - Determine if the given token is "DEFAULT".
> + * @token: Supplies the token string to be compared.
> + *
> + * Return:
> + * * 0	- The token is not "DEFAULT"
> + * * !0	- The token is "DEFAULT"
> + */
> +static bool is_default(char *token)
> +{
> +	return !strcmp(token, "DEFAULT");
> +}
> +
> +/**
> + * free_rule - Free the supplied ipe_rule struct.
> + * @r: Supplies the ipe_rule struct to be freed.
> + */
> +static void free_rule(struct ipe_rule *r)
> +{
> +	struct ipe_prop *p, *t;
> +
> +	if (IS_ERR_OR_NULL(r))
> +		return;
> +
> +	list_for_each_entry_safe(p, t, &r->props, next) {
> +		kfree(p);
> +	}
> +
> +	kfree(r);
> +}
> +
> +static const match_table_t operation_tokens = {
> +	{ipe_op_exec,			"op=EXECUTE"},
> +	{ipe_op_firmware,		"op=FIRMWARE"},
> +	{ipe_op_kernel_module,		"op=KMODULE"},
> +	{ipe_op_kexec_image,		"op=KEXEC_IMAGE"},
> +	{ipe_op_kexec_initramfs,	"op=KEXEC_INITRAMFS"},
> +	{ipe_op_ima_policy,		"op=IMA_POLICY"},
> +	{ipe_op_ima_x509,		"op=IMA_X509_CERT"},
> +	{ipe_op_max,			NULL}
> +};
> +
> +/**
> + * parse_operation - Parse the opeartion type given a token string.
> + * @t: Supplies the token string to be parsed.
> + *
> + * Return: The parsed opeartion type.
> + */
> +static enum ipe_op_type parse_operation(char *t)
> +{
> +	substring_t args[MAX_OPT_ARGS];
> +
> +	return match_token(t, operation_tokens, args);
> +}
> +
> +static const match_table_t action_tokens = {
> +	{ipe_action_allow,	"action=ALLOW"},
> +	{ipe_action_deny,	"action=DENY"},
> +	{ipe_action_max,	NULL}
> +};
> +
> +/**
> + * parse_action - Parse the action type given a token string.
> + * @t: Supplies the token string to be parsed.
> + *
> + * Return: The parsed action type.
> + */
> +static enum ipe_action_type parse_action(char *t)
> +{
> +	substring_t args[MAX_OPT_ARGS];
> +
> +	return match_token(t, action_tokens, args);
> +}
> +
> +static const match_table_t property_tokens = {
> +	{ipe_prop_max,					NULL}
> +};
> +
> +/**
> + * parse_property - Parse the property type given a token string.
> + * @t: Supplies the token string to be parsed.
> + * @r: Supplies the ipe_rule the parsed property will be associated with.
> + *
> + * Return:
> + * * !IS_ERR	- OK
> + * * -ENOMEM	- Out of memory
> + * * -EBADMSG	- The supplied token cannot be parsed
> + */
> +int parse_property(char *t, struct ipe_rule *r)
> +{
> +	substring_t args[MAX_OPT_ARGS];
> +	struct ipe_prop *p = NULL;
> +	int rc = 0;
> +	int token;
> +	char *dup = NULL;
> +
> +	p = kzalloc(sizeof(*p), GFP_KERNEL);
> +	if (!p) {
> +		rc = -ENOMEM;
> +		goto err;
> +	}
> +
> +	token = match_token(t, property_tokens, args);
> +
> +	switch (token) {
> +	case ipe_prop_max:
> +	default:
> +		rc = -EBADMSG;
> +		break;
> +	}
> +	list_add_tail(&p->next, &r->props);
> +
> +err:
> +	kfree(dup);
> +	return rc;
> +}
> +
> +/**
> + * 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_max;
> +	enum ipe_action_type action = ipe_action_max;
> +	struct ipe_rule *r = NULL;
> +	char *t;
> +
> +	r = kzalloc(sizeof(*r), GFP_KERNEL);
> +	if (!r) {
> +		rc = -ENOMEM;
> +		goto err;
> +	}
> +
> +	INIT_LIST_HEAD(&r->next);
> +	INIT_LIST_HEAD(&r->props);
> +
> +	while (t = strsep(&line, " \t"), line) {
> +		if (*t == '\0')
> +			continue;
> +		if (first_token && is_default(t)) {
> +			is_default_rule = true;
> +		} else {
> +			if (!op_parsed) {
> +				op = parse_operation(t);
> +				if (op == ipe_op_max)
> +					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_max) {
> +		rc = -EBADMSG;
> +		goto err;
> +	}
> +
> +	if (is_default_rule) {
> +		if (op == ipe_op_max) {
> +			if (p->global_default_action != ipe_action_max)
> +				rc = -EBADMSG;
> +			else
> +				p->global_default_action = action;
> +		} else {
> +			if (p->rules[op].default_action != ipe_action_max)
> +				rc = -EBADMSG;
> +			else
> +				p->rules[op].default_action = action;
> +		}
> +		free_rule(r);
> +	} else if (op != ipe_op_max && action != ipe_action_max) {
> +		r->op = op;
> +		r->action = action;
> +		list_add_tail(&r->next, &p->rules[op].rules);
> +	} else {
> +		rc = -EBADMSG;
> +	}
> +
> +	if (rc)
> +		goto err;
> +
> +	goto out;
> +
> +err:
> +	free_rule(r);
> +out:
> +	return rc;
> +}
> +
> +/**
> + * 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)
> +			free_rule(pp);
> +
> +	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, and all lines
> + * parsed are have been consumed.
> + *
> + * A parsed policy can be an invalid state for use (a default was
> + * undefined, a header was undefined) by just parsing the policy.
> + *
> + * Return:
> + * * 0		- OK
> + * * -EBADMSG	- Policy is invalid
> + */
> +static int validate_policy(const struct ipe_parsed_policy *p)
> +{
> +	int i = 0;
> +
> +	if (p->global_default_action != ipe_action_max)
> +		return 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(p->rules); ++i) {
> +		if (p->rules[i].default_action == ipe_action_max)
> +			return -EBADMSG;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * parse_policy - Given a string, parse the string into an IPE policy.
> + * @p: partially filled ipe_policy structure to populate with the result.
> + *     it must have text and textlen set.
> + *
> + * Return:
> + * * 0		- OK
> + * * -EBADMSG	- Policy is invalid
> + * * -ENOMEM	- Out of Memory
> + */
> +int parse_policy(struct ipe_policy *p)
> +{
> +	int rc = 0;
> +	size_t len;
> +	char *policy = NULL, *dup = NULL;
> +	char *line = NULL;
> +	bool header_parsed = false;
> +	struct ipe_parsed_policy *pp = NULL;
> +
> +	if (!p->textlen)
> +		return -EBADMSG;
> +
> +	policy = kmemdup_nul(p->text, p->textlen, GFP_KERNEL);
> +	if (!policy)
> +		return -ENOMEM;
> +	dup = policy;
> +
> +	pp = new_parsed_policy();
> +	if (IS_ERR(pp)) {
> +		rc = PTR_ERR(pp);
> +		goto out;
> +	}
> +
> +	while ((line = strsep(&policy, "\n\r")) != NULL) {
> +		remove_comment(line);
> +		remove_trailing_spaces(line);
> +		len = strlen(line);
> +		if (!len)
> +			continue;
> +
> +		if (!header_parsed) {
> +			rc = parse_header(line, pp);
> +			if (rc)
> +				goto err;
> +			header_parsed = true;
> +			continue;
> +		}
> +
> +		rc = parse_rule(line, pp);
> +		if (rc)
> +			goto err;
> +	}
> +
> +	if (!header_parsed || validate_policy(pp)) {
> +		rc = -EBADMSG;
> +		goto err;
> +	}
> +
> +	p->parsed = pp;
> +
> +	goto out;
> +err:
> +	free_parsed_policy(pp);
> +out:
> +	kfree(dup);
> +
> +	return rc;
> +}
> diff --git a/security/ipe/policy_parser.h b/security/ipe/policy_parser.h
> new file mode 100644
> index 000000000000..699ca58a5a32
> --- /dev/null
> +++ b/security/ipe/policy_parser.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) Microsoft Corporation. All rights reserved.
> + */
> +#ifndef IPE_POLICY_PARSER_H
> +#define IPE_POLICY_PARSER_H
> +
> +int parse_policy(struct ipe_policy *p);
> +void free_parsed_policy(struct ipe_parsed_policy *p);
> +
> +#endif /* IPE_POLICY_PARSER */
  

On Tue, Jan 31, 2023 at 11:53:27AM +0100, Roberto Sassu wrote:
> On Mon, 2023-01-30 at 14:57 -0800, Fan Wu wrote:
> > From: Deven Bowers <deven.desai@linux.microsoft.com>
> 
> Uhm, memory leak? Also below. I suggest to use kmemleak.
> 
> Roberto
> 
Nice catch and thanks for the suggestion, I used kmemleak
and detected several incomplete cleanup. Will be fixed in
the next version.
-Fan
  

On Mon, Jan 30, 2023 at 5:58 PM Fan Wu <wufan@linux.microsoft.com> wrote:
>
> From: Deven Bowers <deven.desai@linux.microsoft.com>
>
> 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        |  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/Makefile b/security/ipe/Makefile
> index 571648579991..16bbe80991f1 100644
> --- a/security/ipe/Makefile
> +++ b/security/ipe/Makefile
> @@ -8,3 +8,5 @@
>  obj-$(CONFIG_SECURITY_IPE) += \
>         hooks.o \
>         ipe.o \
> +       policy.o \
> +       policy_parser.o \
> diff --git a/security/ipe/policy.c b/security/ipe/policy.c
> new file mode 100644
> index 000000000000..e446f4b84152
> --- /dev/null
> +++ b/security/ipe/policy.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) Microsoft Corporation. All rights reserved.
> + */
> +
> +#include "ipe.h"
> +#include "policy.h"
> +#include "policy_parser.h"
> +#include "digest.h"
> +
> +#include <linux/verification.h>

Generally speaking the system/kernel-wide header files, e.g. headers
using '<...>', tend to come before the local header files, e.g.
headers using '"..."'.  I wouldn't consider this a hard rule, but
unless you have a reason to put the local header files first I would
stick with convention here.

> +/**
> + * 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);
> +       kfree(p->pkcs7);
> +       kfree(p);
> +}
> +
> +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;
> +}
> +
> +/**
> + * ipe_new_policy - Allocate and parse an ipe_policy structure.
> + *
> + * @text: Supplies a pointer to the plain-text policy to parse.
> + * @textlen: Supplies the length of @text.
> + * @pkcs7: Supplies a pointer to a pkcs7-signed IPE policy.
> + * @pkcs7len: Supplies the length of @pkcs7.
> + *
> + * @text/@textlen Should be NULL/0 if @pkcs7/@pkcs7len is set.
> + *
> + * The result will still need to be associated with a context via
> + * ipe_add_policy.
> + *
> + * Return:
> + * * !IS_ERR   - Success
> + * * -EBADMSG  - Policy is invalid
> + * * -ENOMEM   - Out of memory
> + */
> +struct ipe_policy *ipe_new_policy(const char *text, size_t textlen,
> +                                 const char *pkcs7, size_t pkcs7len)
> +{
> +       int rc = 0;
> +       struct ipe_policy *new = NULL;
> +
> +       new = kzalloc(sizeof(*new), GFP_KERNEL);
> +       if (!new)
> +               return ERR_PTR(-ENOMEM);
> +
> +       if (!text) {
> +               new->pkcs7len = pkcs7len;
> +               new->pkcs7 = kmemdup(pkcs7, pkcs7len, GFP_KERNEL);
> +               if (!new->pkcs7) {
> +                       rc = -ENOMEM;
> +                       goto err;

As Roberto already pointed out, and you acknowledged, this leaks @new.
However, as a FYI for future work, if you have a label with only one
return instruction after the jump, e.g. the 'err' label here, you
should replace the 'goto' with the single return instruction.  Jumping
just to immediately return is a bit silly, but if you also need to
cleanup, e.g. free some memory, that's okay to use the goto/jump.

> +               }
> +
> +               rc = verify_pkcs7_signature(NULL, 0, new->pkcs7, pkcs7len, NULL,
> +                                           VERIFYING_UNSPECIFIED_SIGNATURE,
> +                                           set_pkcs7_data, new);
> +               if (rc)
> +                       goto err;
> +       } else {
> +               new->textlen = textlen;
> +               new->text = kstrdup(text, GFP_KERNEL);
> +               if (!new->text) {
> +                       rc = -ENOMEM;
> +                       goto err;
> +               }
> +       }
> +
> +       rc = parse_policy(new);
> +       if (rc)
> +               goto err;
> +
> +       return new;
> +err:
> +       return ERR_PTR(rc);
> +}

...

> diff --git a/security/ipe/policy.h b/security/ipe/policy.h
> new file mode 100644
> index 000000000000..6af2d9a811ec
> --- /dev/null
> +++ b/security/ipe/policy.h
> @@ -0,0 +1,77 @@
> +/* 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
> +};

I'm used to seeing enum values defined in ALL_CAPS to help visually
distinguish them from variables and other assorted symbols, for
example:

  enum ipe_op_type {
    IPE_OP_EXEC = 0,
    ...
    IPE_OP_MAX,
  };

You might also want to consider prefixing IPE_OP_MAX with a couple
underscores, e.g. __IPE_OP_MAX, to help distinguish it as a sentinel
value and provide some protection in case you think you might ever
want an op named "max".  However, this really is a judgement call that
is up to you.

(Similar comments apply to the other IPE enums)

> +enum ipe_action_type {
> +       ipe_action_allow = 0,
> +       ipe_action_deny,
> +       ipe_action_max
> +};
> +
> +enum ipe_prop_type {
> +       ipe_prop_max
> +};
> +
> +struct ipe_prop {
> +       struct list_head next;
> +       enum ipe_prop_type type;
> +       void *value;
> +};
> +
> +struct ipe_rule {
> +       enum ipe_op_type op;
> +       enum ipe_action_type action;
> +       struct list_head props;
> +       struct list_head next;
> +};
> +
> +struct ipe_op_table {
> +       struct list_head rules;
> +       enum ipe_action_type default_action;
> +};
> +
> +struct ipe_parsed_policy {
> +       const char *name;
> +       struct {
> +               u16 major;
> +               u16 minor;
> +               u16 rev;
> +       } version;
> +
> +       enum ipe_action_type global_default_action;
> +
> +       struct ipe_op_table rules[ipe_op_max];
> +};
> +
> +struct ipe_policy {
> +       const char     *pkcs7;
> +       size_t          pkcs7len;
> +
> +       const char     *text;
> +       size_t          textlen;
> +
> +       struct ipe_parsed_policy *parsed;
> +};

None of the other structs in this header file have horizontally
aligned variable names, you should pick one style and stick with it
... and that style should be the un-aligned style, e.g. what was used
in 'struct ipe_rule'

> +struct ipe_policy *ipe_new_policy(const char *text, size_t textlen,
> +                                 const char *pkcs7, size_t pkcs7len);
> +void ipe_free_policy(struct ipe_policy *pol);
> +
> +#endif /* IPE_POLICY_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,
  };

> +       for (i = 0; i < ARRAY_SIZE(p->rules); ++i) {
> +               t = &p->rules[i];
> +
> +               t->default_action = ipe_action_max;
> +               INIT_LIST_HEAD(&t->rules);
> +       }
> +
> +       return p;
> +}
> +
> +/**
> + * remove_comment - Truncate all chars following START_COMMENT in a string.
> + *
> + * @line: Supplies a poilcy line string for preprocessing.

"policy" :)

I'm definitely guilty of adding a lot of silly spelling errors to
codebases over the years, so no worries here, but in case you haven't
seen the codespell tool already, it might be something worth taking a
look at sometime.

* https://github.com/codespell-project/codespell

> + */
> +static void remove_comment(char *line)
> +{
> +       size_t i, len = 0;
> +
> +       len = strlen(line);
> +       for (i = 0; i < len && line[i] != START_COMMENT; ++i)
> +               ;

The kernel has a strchr() implementation which could simplify this,
and possibly speed things up if there is an arch specific optimized
implementation.

> +       line[i] = '\0';
> +}
> +
> +/**
> + * remove_trailing_spaces - Truncate all trailing spaces in a string.
> + *
> + * @line: Supplies a poilcy line string for preprocessing.
> + */
> +static void remove_trailing_spaces(char *line)
> +{
> +       size_t i, len = 0;
> +
> +       len = strlen(line);
> +       for (i = len; i > 0 && (line[i - 1] == ' ' || line[i - 1] == '\t'); --i)
> +               ;

You can probably drop the @len variable and just assign 'i =
strlen(line)' in the for-loop initializer.

> +       line[i] = '\0';
> +}
> +
> +/**
> + * parse_version - Parse policy version.
> + * @ver: Supplies a version string to be parsed.
> + * @p: Supplies the partial parsed policy.
> + *
> + * Return:
> + * * 0 - OK
> + * * !0        - Standard errno
> + */
> +static int parse_version(char *ver, struct ipe_parsed_policy *p)
> +{
> +       int rc = 0;
> +       size_t sep_count = 0;
> +       char *token;
> +       u16 *const cv[] = { &p->version.major, &p->version.minor, &p->version.rev };
> +
> +       while ((token = strsep(&ver, ".")) != NULL) {
> +               /* prevent overflow */
> +               if (sep_count >= ARRAY_SIZE(cv)) {
> +                       rc = -EBADMSG;
> +                       goto err;

Remember what I said above about not needing a 'goto' here? ;)

> +               }
> +
> +               rc = kstrtou16(token, 10, cv[sep_count]);
> +               if (rc)
> +                       goto err;
> +
> +               ++sep_count;
> +       }
> +
> +       /* prevent underflow */
> +       if (sep_count != ARRAY_SIZE(cv))
> +               rc = -EBADMSG;
> +
> +err:
> +       return rc;
> +}
> +
> +enum header_opt {
> +       ipe_header_policy_name = 0,
> +       ipe_header_policy_version,
> +       ipe_header_max
> +};
> +
> +static const match_table_t header_tokens = {
> +       {ipe_header_policy_name,        "policy_name=%s"},
> +       {ipe_header_policy_version,     "policy_version=%s"},
> +       {ipe_header_max,                NULL}
> +};
> +
> +/**
> + * 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) {
> +               int token;
> +
> +               if (*t == '\0')
> +                       continue;
> +               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;
> +       }
> +       goto out;

Generally the normal, non-error case is structured so that the
function can continue to fall through to the correct code without
needed a 'goto'.  I would suggest moving the 'err' label/code *after*
the 'out' label/code so the normal case can just fall through without
the goto; you will have to add a 'goto out' at the end of 'err', but
that's the error case so we aren't going to worry too much about that.

Put another (shorter) way, structure your code to optimize for the
common, non-error case.

Needless to say, this applies to other functions in this patch(set).

> +err:
> +       kfree(p->name);
> +       p->name = NULL;
> +out:
> +       kfree(ver);
> +       return rc;
> +}
> +
> +/**
> + * is_default - Determine if the given token is "DEFAULT".
> + * @token: Supplies the token string to be compared.
> + *
> + * Return:
> + * * 0 - The token is not "DEFAULT"
> + * * !0        - The token is "DEFAULT"
> + */
> +static bool is_default(char *token)
> +{
> +       return !strcmp(token, "DEFAULT");
> +}

Let's be honest, "is_default()" isn't a great name, and it's a pretty
trivial function too; I'm wondering if hiding the simple strcmp()
behind an oddly named function is really all that helpful.  I'm okay
if you want to keep the function, but can we name it something else?
Maybe "token_default(...)" or something similar?

> +/**
> + * free_rule - Free the supplied ipe_rule struct.
> + * @r: Supplies the ipe_rule struct to be freed.
> + */

It might be worth mentioning that @r should be removed from any lists,
e.g. list_empty() is true.

> +static void free_rule(struct ipe_rule *r)
> +{
> +       struct ipe_prop *p, *t;
> +
> +       if (IS_ERR_OR_NULL(r))
> +               return;
> +
> +       list_for_each_entry_safe(p, t, &r->props, next) {
> +               kfree(p);
> +       }

That's interesting, I'm used to seeing a 'list_del()' call (or
similar) before the list entry is freed.  Although looking at
list_for_each_entry_safe() I guess it is safe with the current
implementation ... did you see this pattern elsewhere in the kernel?
If so, where?

Unless this is performance critical (I don't think it is?), it might
be safer to do an explicit `list_del()` before free'ing the entries
... unless this is now a common pattern in the kernel and I just
missed the memo.

> +       kfree(r);
> +}
> +
> +static const match_table_t operation_tokens = {
> +       {ipe_op_exec,                   "op=EXECUTE"},
> +       {ipe_op_firmware,               "op=FIRMWARE"},
> +       {ipe_op_kernel_module,          "op=KMODULE"},
> +       {ipe_op_kexec_image,            "op=KEXEC_IMAGE"},
> +       {ipe_op_kexec_initramfs,        "op=KEXEC_INITRAMFS"},
> +       {ipe_op_ima_policy,             "op=IMA_POLICY"},
> +       {ipe_op_ima_x509,               "op=IMA_X509_CERT"},
> +       {ipe_op_max,                    NULL}
> +};
> +
> +/**
> + * parse_operation - Parse the opeartion type given a token string.
> + * @t: Supplies the token string to be parsed.
> + *
> + * Return: The parsed opeartion type.
> + */
> +static enum ipe_op_type parse_operation(char *t)
> +{
> +       substring_t args[MAX_OPT_ARGS];
> +
> +       return match_token(t, operation_tokens, args);
> +}
> +
> +static const match_table_t action_tokens = {
> +       {ipe_action_allow,      "action=ALLOW"},
> +       {ipe_action_deny,       "action=DENY"},
> +       {ipe_action_max,        NULL}
> +};
> +
> +/**
> + * parse_action - Parse the action type given a token string.
> + * @t: Supplies the token string to be parsed.
> + *
> + * Return: The parsed action type.
> + */
> +static enum ipe_action_type parse_action(char *t)
> +{
> +       substring_t args[MAX_OPT_ARGS];
> +
> +       return match_token(t, action_tokens, args);
> +}
> +
> +static const match_table_t property_tokens = {
> +       {ipe_prop_max,                                  NULL}
> +};
> +
> +/**
> + * parse_property - Parse the property type given a token string.
> + * @t: Supplies the token string to be parsed.
> + * @r: Supplies the ipe_rule the parsed property will be associated with.
> + *
> + * Return:
> + * * !IS_ERR   - OK
> + * * -ENOMEM   - Out of memory
> + * * -EBADMSG  - The supplied token cannot be parsed
> + */
> +int parse_property(char *t, struct ipe_rule *r)
> +{
> +       substring_t args[MAX_OPT_ARGS];
> +       struct ipe_prop *p = NULL;
> +       int rc = 0;
> +       int token;
> +       char *dup = NULL;
> +
> +       p = kzalloc(sizeof(*p), GFP_KERNEL);
> +       if (!p) {
> +               rc = -ENOMEM;
> +               goto err;
> +       }
> +
> +       token = match_token(t, property_tokens, args);
> +
> +       switch (token) {
> +       case ipe_prop_max:
> +       default:
> +               rc = -EBADMSG;
> +               break;
> +       }
> +       list_add_tail(&p->next, &r->props);
> +
> +err:
> +       kfree(dup);
> +       return rc;
> +}

There is a lot of stuff in 'parse_property()' that doesn't make sense
at this point in the patchset, including lots of unused variables.
Considering that no valid properties are defined yet, why not just
make this function return -EBADMSG in this patch?  You can always
populate it later when it becomes useful.

int parse_property(...)
{
  return -EBADMSG;
}

> +/**
> + * 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_max;
> +       enum ipe_action_type action = ipe_action_max;
> +       struct ipe_rule *r = NULL;
> +       char *t;
> +
> +       r = kzalloc(sizeof(*r), GFP_KERNEL);
> +       if (!r) {
> +               rc = -ENOMEM;
> +               goto err;
> +       }
> +
> +       INIT_LIST_HEAD(&r->next);
> +       INIT_LIST_HEAD(&r->props);
> +
> +       while (t = strsep(&line, " \t"), line) {
> +               if (*t == '\0')
> +                       continue;
> +               if (first_token && is_default(t)) {
> +                       is_default_rule = true;
> +               } else {
> +                       if (!op_parsed) {
> +                               op = parse_operation(t);
> +                               if (op == ipe_op_max)
> +                                       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_max) {
> +               rc = -EBADMSG;
> +               goto err;
> +       }
> +
> +       if (is_default_rule) {
> +               if (op == ipe_op_max) {
> +                       if (p->global_default_action != ipe_action_max)
> +                               rc = -EBADMSG;
> +                       else
> +                               p->global_default_action = action;
> +               } else {
> +                       if (p->rules[op].default_action != ipe_action_max)
> +                               rc = -EBADMSG;
> +                       else
> +                               p->rules[op].default_action = action;
> +               }
> +               free_rule(r);
> +       } else if (op != ipe_op_max && action != ipe_action_max) {
> +               r->op = op;
> +               r->action = action;
> +               list_add_tail(&r->next, &p->rules[op].rules);

There is no way @rc could be non-zero here, right?  If there is some
chance of it being non-zero we could have a problem with the @rc check
below jumping us to the 'err' label and free'ing a rule that has been
added to the list.

It might be better to move the list addition after the last error check.

> +       } else {
> +               rc = -EBADMSG;
> +       }
> +
> +       if (rc)
> +               goto err;
> +
> +       goto out;
> +
> +err:
> +       free_rule(r);
> +out:
> +       return rc;
> +}
> +
> +/**
> + * 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)
> +                       free_rule(pp);
> +
> +       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, and all lines
> + * parsed are have been consumed.
> + *
> + * A parsed policy can be an invalid state for use (a default was
> + * undefined, a header was undefined) by just parsing the policy.
> + *
> + * Return:
> + * * 0         - OK
> + * * -EBADMSG  - Policy is invalid
> + */
> +static int validate_policy(const struct ipe_parsed_policy *p)
> +{
> +       int i = 0;
> +
> +       if (p->global_default_action != ipe_action_max)
> +               return 0;
> +
> +       for (i = 0; i < ARRAY_SIZE(p->rules); ++i) {
> +               if (p->rules[i].default_action == ipe_action_max)
> +                       return -EBADMSG;
> +       }
> +
> +       return 0;
> +}
> +
> +/**
> + * parse_policy - Given a string, parse the string into an IPE policy.
> + * @p: partially filled ipe_policy structure to populate with the result.
> + *     it must have text and textlen set.
> + *
> + * Return:
> + * * 0         - OK
> + * * -EBADMSG  - Policy is invalid
> + * * -ENOMEM   - Out of Memory
> + */
> +int parse_policy(struct ipe_policy *p)
> +{
> +       int rc = 0;
> +       size_t len;
> +       char *policy = NULL, *dup = NULL;
> +       char *line = NULL;
> +       bool header_parsed = false;
> +       struct ipe_parsed_policy *pp = NULL;
> +
> +       if (!p->textlen)
> +               return -EBADMSG;
> +
> +       policy = kmemdup_nul(p->text, p->textlen, GFP_KERNEL);
> +       if (!policy)
> +               return -ENOMEM;
> +       dup = policy;
> +
> +       pp = new_parsed_policy();
> +       if (IS_ERR(pp)) {
> +               rc = PTR_ERR(pp);
> +               goto out;
> +       }
> +
> +       while ((line = strsep(&policy, "\n\r")) != NULL) {
> +               remove_comment(line);
> +               remove_trailing_spaces(line);

I think it might be very easy for 'remove_trailing_spaces()' to return
the length of the string as it already knows where the string ends;
perhaps the function could return the string length and we could get
rid of the 'strlen()' call below?

> +               len = strlen(line);
> +               if (!len)
> +                       continue;
> +
> +               if (!header_parsed) {
> +                       rc = parse_header(line, pp);
> +                       if (rc)
> +                               goto err;
> +                       header_parsed = true;
> +                       continue;

Instead of the 'continue' above, why not just put the 'parse_rule()'
call into the 'else' block of this if-then-else?

> +               }
> +
> +               rc = parse_rule(line, pp);
> +               if (rc)
> +                       goto err;
> +       }
> +
> +       if (!header_parsed || validate_policy(pp)) {
> +               rc = -EBADMSG;
> +               goto err;
> +       }
> +
> +       p->parsed = pp;
> +
> +       goto out;
> +err:
> +       free_parsed_policy(pp);
> +out:
> +       kfree(dup);
> +
> +       return rc;
> +}
> diff --git a/security/ipe/policy_parser.h b/security/ipe/policy_parser.h
> new file mode 100644
> index 000000000000..699ca58a5a32
> --- /dev/null
> +++ b/security/ipe/policy_parser.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) Microsoft Corporation. All rights reserved.
> + */
> +#ifndef IPE_POLICY_PARSER_H
> +#define IPE_POLICY_PARSER_H
> +
> +int parse_policy(struct ipe_policy *p);
> +void free_parsed_policy(struct ipe_parsed_policy *p);
> +
> +#endif /* IPE_POLICY_PARSER */
> --
> 2.39.0

--
paul-moore.com
  

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@linux.microsoft.com> wrote:
> >
> > From: Deven Bowers <deven.desai@linux.microsoft.com>
> >
> > 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        |  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/Makefile b/security/ipe/Makefile
> > index 571648579991..16bbe80991f1 100644
> > --- a/security/ipe/Makefile
> > +++ b/security/ipe/Makefile
> > @@ -8,3 +8,5 @@
> >  obj-$(CONFIG_SECURITY_IPE) += \
> >         hooks.o \
> >         ipe.o \
> > +       policy.o \
> > +       policy_parser.o \
> > diff --git a/security/ipe/policy.c b/security/ipe/policy.c
> > new file mode 100644
> > index 000000000000..e446f4b84152
> > --- /dev/null
> > +++ b/security/ipe/policy.c
> > @@ -0,0 +1,99 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) Microsoft Corporation. All rights reserved.
> > + */
> > +
> > +#include "ipe.h"
> > +#include "policy.h"
> > +#include "policy_parser.h"
> > +#include "digest.h"
> > +
> > +#include <linux/verification.h>
> 
> Generally speaking the system/kernel-wide header files, e.g. headers
> using '<...>', tend to come before the local header files, e.g.
> headers using '"..."'.  I wouldn't consider this a hard rule, but
> unless you have a reason to put the local header files first I would
> stick with convention here.
> 

I agree we didn't follow the correct common pattern here, I will update
all header lines in the next version.

> > +/**
> > + * 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);
> > +       kfree(p->pkcs7);
> > +       kfree(p);
> > +}
> > +
> > +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;
> > +}
> > +
> > +/**
> > + * ipe_new_policy - Allocate and parse an ipe_policy structure.
> > + *
> > + * @text: Supplies a pointer to the plain-text policy to parse.
> > + * @textlen: Supplies the length of @text.
> > + * @pkcs7: Supplies a pointer to a pkcs7-signed IPE policy.
> > + * @pkcs7len: Supplies the length of @pkcs7.
> > + *
> > + * @text/@textlen Should be NULL/0 if @pkcs7/@pkcs7len is set.
> > + *
> > + * The result will still need to be associated with a context via
> > + * ipe_add_policy.
> > + *
> > + * Return:
> > + * * !IS_ERR   - Success
> > + * * -EBADMSG  - Policy is invalid
> > + * * -ENOMEM   - Out of memory
> > + */
> > +struct ipe_policy *ipe_new_policy(const char *text, size_t textlen,
> > +                                 const char *pkcs7, size_t pkcs7len)
> > +{
> > +       int rc = 0;
> > +       struct ipe_policy *new = NULL;
> > +
> > +       new = kzalloc(sizeof(*new), GFP_KERNEL);
> > +       if (!new)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       if (!text) {
> > +               new->pkcs7len = pkcs7len;
> > +               new->pkcs7 = kmemdup(pkcs7, pkcs7len, GFP_KERNEL);
> > +               if (!new->pkcs7) {
> > +                       rc = -ENOMEM;
> > +                       goto err;
> 
> As Roberto already pointed out, and you acknowledged, this leaks @new.
> However, as a FYI for future work, if you have a label with only one
> return instruction after the jump, e.g. the 'err' label here, you
> should replace the 'goto' with the single return instruction.  Jumping
> just to immediately return is a bit silly, but if you also need to
> cleanup, e.g. free some memory, that's okay to use the goto/jump.
> 
> > +               }
> > +
> > +               rc = verify_pkcs7_signature(NULL, 0, new->pkcs7, pkcs7len, NULL,
> > +                                           VERIFYING_UNSPECIFIED_SIGNATURE,
> > +                                           set_pkcs7_data, new);
> > +               if (rc)
> > +                       goto err;
> > +       } else {
> > +               new->textlen = textlen;
> > +               new->text = kstrdup(text, GFP_KERNEL);
> > +               if (!new->text) {
> > +                       rc = -ENOMEM;
> > +                       goto err;
> > +               }
> > +       }
> > +
> > +       rc = parse_policy(new);
> > +       if (rc)
> > +               goto err;
> > +
> > +       return new;
> > +err:
> > +       return ERR_PTR(rc);
> > +}
> 
> ...
> 
> > diff --git a/security/ipe/policy.h b/security/ipe/policy.h
> > new file mode 100644
> > index 000000000000..6af2d9a811ec
> > --- /dev/null
> > +++ b/security/ipe/policy.h
> > @@ -0,0 +1,77 @@
> > +/* 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
> > +};
> 
> I'm used to seeing enum values defined in ALL_CAPS to help visually
> distinguish them from variables and other assorted symbols, for
> example:
> 
>   enum ipe_op_type {
>     IPE_OP_EXEC = 0,
>     ...
>     IPE_OP_MAX,
>   };
> 
> You might also want to consider prefixing IPE_OP_MAX with a couple
> underscores, e.g. __IPE_OP_MAX, to help distinguish it as a sentinel
> value and provide some protection in case you think you might ever
> want an op named "max".  However, this really is a judgement call that
> is up to you.
> 
> (Similar comments apply to the other IPE enums)
> 

Yes this looks much better, I will also update all enums. Thanks for the advice.

> > +enum ipe_action_type {
> > +       ipe_action_allow = 0,
> > +       ipe_action_deny,
> > +       ipe_action_max
> > +};
> > +
> > +enum ipe_prop_type {
> > +       ipe_prop_max
> > +};
> > +
> > +struct ipe_prop {
> > +       struct list_head next;
> > +       enum ipe_prop_type type;
> > +       void *value;
> > +};
> > +
> > +struct ipe_rule {
> > +       enum ipe_op_type op;
> > +       enum ipe_action_type action;
> > +       struct list_head props;
> > +       struct list_head next;
> > +};
> > +
> > +struct ipe_op_table {
> > +       struct list_head rules;
> > +       enum ipe_action_type default_action;
> > +};
> > +
> > +struct ipe_parsed_policy {
> > +       const char *name;
> > +       struct {
> > +               u16 major;
> > +               u16 minor;
> > +               u16 rev;
> > +       } version;
> > +
> > +       enum ipe_action_type global_default_action;
> > +
> > +       struct ipe_op_table rules[ipe_op_max];
> > +};
> > +
> > +struct ipe_policy {
> > +       const char     *pkcs7;
> > +       size_t          pkcs7len;
> > +
> > +       const char     *text;
> > +       size_t          textlen;
> > +
> > +       struct ipe_parsed_policy *parsed;
> > +};
> 
> None of the other structs in this header file have horizontally
> aligned variable names, you should pick one style and stick with it
> ... and that style should be the un-aligned style, e.g. what was used
> in 'struct ipe_rule'
> 

Thanks for pointing that out, will also update this part.

> > +struct ipe_policy *ipe_new_policy(const char *text, size_t textlen,
> > +                                 const char *pkcs7, size_t pkcs7len);
> > +void ipe_free_policy(struct ipe_policy *pol);
> > +
> > +#endif /* IPE_POLICY_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. For example, to get the linked-list head of rules for the
EXEC operation, we would have to use index IPE_OP_EXEC-1. Is this
acceptable?


> > +       for (i = 0; i < ARRAY_SIZE(p->rules); ++i) {
> > +               t = &p->rules[i];
> > +
> > +               t->default_action = ipe_action_max;
> > +               INIT_LIST_HEAD(&t->rules);
> > +       }
> > +
> > +       return p;
> > +}
> > +
> > +/**
> > + * remove_comment - Truncate all chars following START_COMMENT in a string.
> > + *
> > + * @line: Supplies a poilcy line string for preprocessing.
> 
> "policy" :)
> 
> I'm definitely guilty of adding a lot of silly spelling errors to
> codebases over the years, so no worries here, but in case you haven't
> seen the codespell tool already, it might be something worth taking a
> look at sometime.
> 
> * https://github.com/codespell-project/codespell
> 

Thanks for the suggestion. I have found several other typos by using this.

> > + */
> > +static void remove_comment(char *line)
> > +{
> > +       size_t i, len = 0;
> > +
> > +       len = strlen(line);
> > +       for (i = 0; i < len && line[i] != START_COMMENT; ++i)
> > +               ;
> 
> The kernel has a strchr() implementation which could simplify this,
> and possibly speed things up if there is an arch specific optimized
> implementation.
> 

Yep this one is better, will switch to use it.

> > +       line[i] = '\0';
> > +}
> > +
> > +/**
> > + * remove_trailing_spaces - Truncate all trailing spaces in a string.
> > + *
> > + * @line: Supplies a poilcy line string for preprocessing.
> > + */
> > +static void remove_trailing_spaces(char *line)
> > +{
> > +       size_t i, len = 0;
> > +
> > +       len = strlen(line);
> > +       for (i = len; i > 0 && (line[i - 1] == ' ' || line[i - 1] == '\t'); --i)
> > +               ;
> 
> You can probably drop the @len variable and just assign 'i =
> strlen(line)' in the for-loop initializer.
> 

Yep, len is redundant here, will remove it.

> > +       line[i] = '\0';
> > +}
> > +
> > +/**
> > + * parse_version - Parse policy version.
> > + * @ver: Supplies a version string to be parsed.
> > + * @p: Supplies the partial parsed policy.
> > + *
> > + * Return:
> > + * * 0 - OK
> > + * * !0        - Standard errno
> > + */
> > +static int parse_version(char *ver, struct ipe_parsed_policy *p)
> > +{
> > +       int rc = 0;
> > +       size_t sep_count = 0;
> > +       char *token;
> > +       u16 *const cv[] = { &p->version.major, &p->version.minor, &p->version.rev };
> > +
> > +       while ((token = strsep(&ver, ".")) != NULL) {
> > +               /* prevent overflow */
> > +               if (sep_count >= ARRAY_SIZE(cv)) {
> > +                       rc = -EBADMSG;
> > +                       goto err;
> 
> Remember what I said above about not needing a 'goto' here? ;)
> 

Yes just return is enough, will update it.

> > +               }
> > +
> > +               rc = kstrtou16(token, 10, cv[sep_count]);
> > +               if (rc)
> > +                       goto err;
> > +
> > +               ++sep_count;
> > +       }
> > +
> > +       /* prevent underflow */
> > +       if (sep_count != ARRAY_SIZE(cv))
> > +               rc = -EBADMSG;
> > +
> > +err:
> > +       return rc;
> > +}
> > +
> > +enum header_opt {
> > +       ipe_header_policy_name = 0,
> > +       ipe_header_policy_version,
> > +       ipe_header_max
> > +};
> > +
> > +static const match_table_t header_tokens = {
> > +       {ipe_header_policy_name,        "policy_name=%s"},
> > +       {ipe_header_policy_version,     "policy_version=%s"},
> > +       {ipe_header_max,                NULL}
> > +};
> > +
> > +/**
> > + * 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) {
> > +               int token;
> > +
> > +               if (*t == '\0')
> > +                       continue;
> > +               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;
> > +       }
> > +       goto out;
> 
> Generally the normal, non-error case is structured so that the
> function can continue to fall through to the correct code without
> needed a 'goto'.  I would suggest moving the 'err' label/code *after*
> the 'out' label/code so the normal case can just fall through without
> the goto; you will have to add a 'goto out' at the end of 'err', but
> that's the error case so we aren't going to worry too much about that.
> 
> Put another (shorter) way, structure your code to optimize for the
> common, non-error case.
> 
> Needless to say, this applies to other functions in this patch(set).
> 

Thanks for pointing that out, I will update all the cases like this one.

> > +err:
> > +       kfree(p->name);
> > +       p->name = NULL;
> > +out:
> > +       kfree(ver);
> > +       return rc;
> > +}
> > +
> > +/**
> > + * is_default - Determine if the given token is "DEFAULT".
> > + * @token: Supplies the token string to be compared.
> > + *
> > + * Return:
> > + * * 0 - The token is not "DEFAULT"
> > + * * !0        - The token is "DEFAULT"
> > + */
> > +static bool is_default(char *token)
> > +{
> > +       return !strcmp(token, "DEFAULT");
> > +}
> 
> Let's be honest, "is_default()" isn't a great name, and it's a pretty
> trivial function too; I'm wondering if hiding the simple strcmp()
> behind an oddly named function is really all that helpful.  I'm okay
> if you want to keep the function, but can we name it something else?
> Maybe "token_default(...)" or something similar?
> 

I agree, I will take the name token_default.

> > +/**
> > + * free_rule - Free the supplied ipe_rule struct.
> > + * @r: Supplies the ipe_rule struct to be freed.
> > + */
> 
> It might be worth mentioning that @r should be removed from any lists,
> e.g. list_empty() is true.
> 
> > +static void free_rule(struct ipe_rule *r)
> > +{
> > +       struct ipe_prop *p, *t;
> > +
> > +       if (IS_ERR_OR_NULL(r))
> > +               return;
> > +
> > +       list_for_each_entry_safe(p, t, &r->props, next) {
> > +               kfree(p);
> > +       }
> 
> That's interesting, I'm used to seeing a 'list_del()' call (or
> similar) before the list entry is freed.  Although looking at
> list_for_each_entry_safe() I guess it is safe with the current
> implementation ... did you see this pattern elsewhere in the kernel?
> If so, where?
> 
> Unless this is performance critical (I don't think it is?), it might
> be safer to do an explicit `list_del()` before free'ing the entries
> ... unless this is now a common pattern in the kernel and I just
> missed the memo.
> 

I have double checked other use cases and found we were indeed wrong
here, I will add list_del calls in the next version.

> > +       kfree(r);
> > +}
> > +
> > +static const match_table_t operation_tokens = {
> > +       {ipe_op_exec,                   "op=EXECUTE"},
> > +       {ipe_op_firmware,               "op=FIRMWARE"},
> > +       {ipe_op_kernel_module,          "op=KMODULE"},
> > +       {ipe_op_kexec_image,            "op=KEXEC_IMAGE"},
> > +       {ipe_op_kexec_initramfs,        "op=KEXEC_INITRAMFS"},
> > +       {ipe_op_ima_policy,             "op=IMA_POLICY"},
> > +       {ipe_op_ima_x509,               "op=IMA_X509_CERT"},
> > +       {ipe_op_max,                    NULL}
> > +};
> > +
> > +/**
> > + * parse_operation - Parse the opeartion type given a token string.
> > + * @t: Supplies the token string to be parsed.
> > + *
> > + * Return: The parsed opeartion type.
> > + */
> > +static enum ipe_op_type parse_operation(char *t)
> > +{
> > +       substring_t args[MAX_OPT_ARGS];
> > +
> > +       return match_token(t, operation_tokens, args);
> > +}
> > +
> > +static const match_table_t action_tokens = {
> > +       {ipe_action_allow,      "action=ALLOW"},
> > +       {ipe_action_deny,       "action=DENY"},
> > +       {ipe_action_max,        NULL}
> > +};
> > +
> > +/**
> > + * parse_action - Parse the action type given a token string.
> > + * @t: Supplies the token string to be parsed.
> > + *
> > + * Return: The parsed action type.
> > + */
> > +static enum ipe_action_type parse_action(char *t)
> > +{
> > +       substring_t args[MAX_OPT_ARGS];
> > +
> > +       return match_token(t, action_tokens, args);
> > +}
> > +
> > +static const match_table_t property_tokens = {
> > +       {ipe_prop_max,                                  NULL}
> > +};
> > +
> > +/**
> > + * parse_property - Parse the property type given a token string.
> > + * @t: Supplies the token string to be parsed.
> > + * @r: Supplies the ipe_rule the parsed property will be associated with.
> > + *
> > + * Return:
> > + * * !IS_ERR   - OK
> > + * * -ENOMEM   - Out of memory
> > + * * -EBADMSG  - The supplied token cannot be parsed
> > + */
> > +int parse_property(char *t, struct ipe_rule *r)
> > +{
> > +       substring_t args[MAX_OPT_ARGS];
> > +       struct ipe_prop *p = NULL;
> > +       int rc = 0;
> > +       int token;
> > +       char *dup = NULL;
> > +
> > +       p = kzalloc(sizeof(*p), GFP_KERNEL);
> > +       if (!p) {
> > +               rc = -ENOMEM;
> > +               goto err;
> > +       }
> > +
> > +       token = match_token(t, property_tokens, args);
> > +
> > +       switch (token) {
> > +       case ipe_prop_max:
> > +       default:
> > +               rc = -EBADMSG;
> > +               break;
> > +       }
> > +       list_add_tail(&p->next, &r->props);
> > +
> > +err:
> > +       kfree(dup);
> > +       return rc;
> > +}
> 
> There is a lot of stuff in 'parse_property()' that doesn't make sense
> at this point in the patchset, including lots of unused variables.
> Considering that no valid properties are defined yet, why not just
> make this function return -EBADMSG in this patch?  You can always
> populate it later when it becomes useful.
> 
> int parse_property(...)
> {
>   return -EBADMSG;
> }
> 

Sure, I can restructure the patch to add them in the later patches.

> > +/**
> > + * 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_max;
> > +       enum ipe_action_type action = ipe_action_max;
> > +       struct ipe_rule *r = NULL;
> > +       char *t;
> > +
> > +       r = kzalloc(sizeof(*r), GFP_KERNEL);
> > +       if (!r) {
> > +               rc = -ENOMEM;
> > +               goto err;
> > +       }
> > +
> > +       INIT_LIST_HEAD(&r->next);
> > +       INIT_LIST_HEAD(&r->props);
> > +
> > +       while (t = strsep(&line, " \t"), line) {
> > +               if (*t == '\0')
> > +                       continue;
> > +               if (first_token && is_default(t)) {
> > +                       is_default_rule = true;
> > +               } else {
> > +                       if (!op_parsed) {
> > +                               op = parse_operation(t);
> > +                               if (op == ipe_op_max)
> > +                                       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_max) {
> > +               rc = -EBADMSG;
> > +               goto err;
> > +       }
> > +
> > +       if (is_default_rule) {
> > +               if (op == ipe_op_max) {
> > +                       if (p->global_default_action != ipe_action_max)
> > +                               rc = -EBADMSG;
> > +                       else
> > +                               p->global_default_action = action;
> > +               } else {
> > +                       if (p->rules[op].default_action != ipe_action_max)
> > +                               rc = -EBADMSG;
> > +                       else
> > +                               p->rules[op].default_action = action;
> > +               }
> > +               free_rule(r);
> > +       } else if (op != ipe_op_max && action != ipe_action_max) {
> > +               r->op = op;
> > +               r->action = action;
> > +               list_add_tail(&r->next, &p->rules[op].rules);
> 
> There is no way @rc could be non-zero here, right?  If there is some
> chance of it being non-zero we could have a problem with the @rc check
> below jumping us to the 'err' label and free'ing a rule that has been
> added to the list.
> 
> It might be better to move the list addition after the last error check.
> 

Yes, rc cannot be non-zero here because all the rc assignments will just
go to the 'err' label. But I also agree that moving the list addition after
the last error check can avoid potential bugs in the future. I will update this part.

> > +       } else {
> > +               rc = -EBADMSG;
> > +       }
> > +
> > +       if (rc)
> > +               goto err;
> > +
> > +       goto out;
> > +
> > +err:
> > +       free_rule(r);
> > +out:
> > +       return rc;
> > +}
> > +
> > +/**
> > + * 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)
> > +                       free_rule(pp);
> > +
> > +       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, and all lines
> > + * parsed are have been consumed.
> > + *
> > + * A parsed policy can be an invalid state for use (a default was
> > + * undefined, a header was undefined) by just parsing the policy.
> > + *
> > + * Return:
> > + * * 0         - OK
> > + * * -EBADMSG  - Policy is invalid
> > + */
> > +static int validate_policy(const struct ipe_parsed_policy *p)
> > +{
> > +       int i = 0;
> > +
> > +       if (p->global_default_action != ipe_action_max)
> > +               return 0;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(p->rules); ++i) {
> > +               if (p->rules[i].default_action == ipe_action_max)
> > +                       return -EBADMSG;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * parse_policy - Given a string, parse the string into an IPE policy.
> > + * @p: partially filled ipe_policy structure to populate with the result.
> > + *     it must have text and textlen set.
> > + *
> > + * Return:
> > + * * 0         - OK
> > + * * -EBADMSG  - Policy is invalid
> > + * * -ENOMEM   - Out of Memory
> > + */
> > +int parse_policy(struct ipe_policy *p)
> > +{
> > +       int rc = 0;
> > +       size_t len;
> > +       char *policy = NULL, *dup = NULL;
> > +       char *line = NULL;
> > +       bool header_parsed = false;
> > +       struct ipe_parsed_policy *pp = NULL;
> > +
> > +       if (!p->textlen)
> > +               return -EBADMSG;
> > +
> > +       policy = kmemdup_nul(p->text, p->textlen, GFP_KERNEL);
> > +       if (!policy)
> > +               return -ENOMEM;
> > +       dup = policy;
> > +
> > +       pp = new_parsed_policy();
> > +       if (IS_ERR(pp)) {
> > +               rc = PTR_ERR(pp);
> > +               goto out;
> > +       }
> > +
> > +       while ((line = strsep(&policy, "\n\r")) != NULL) {
> > +               remove_comment(line);
> > +               remove_trailing_spaces(line);
> 
> I think it might be very easy for 'remove_trailing_spaces()' to return
> the length of the string as it already knows where the string ends;
> perhaps the function could return the string length and we could get
> rid of the 'strlen()' call below?
> 

Yes that's very easy, I will add a return value to remove_trailing_spaces().

> > +               len = strlen(line);
> > +               if (!len)
> > +                       continue;
> > +
> > +               if (!header_parsed) {
> > +                       rc = parse_header(line, pp);
> > +                       if (rc)
> > +                               goto err;
> > +                       header_parsed = true;
> > +                       continue;
> 
> Instead of the 'continue' above, why not just put the 'parse_rule()'
> call into the 'else' block of this if-then-else?
> 

I agree that's a better structure, I will switch to use 'else'.

-Fan

> > +               }
> > +
> > +               rc = parse_rule(line, pp);
> > +               if (rc)
> > +                       goto err;
> > +       }
> > +
> > +       if (!header_parsed || validate_policy(pp)) {
> > +               rc = -EBADMSG;
> > +               goto err;
> > +       }
> > +
> > +       p->parsed = pp;
> > +
> > +       goto out;
> > +err:
> > +       free_parsed_policy(pp);
> > +out:
> > +       kfree(dup);
> > +
> > +       return rc;
> > +}
> > diff --git a/security/ipe/policy_parser.h b/security/ipe/policy_parser.h
> > new file mode 100644
> > index 000000000000..699ca58a5a32
> > --- /dev/null
> > +++ b/security/ipe/policy_parser.h
> > @@ -0,0 +1,11 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) Microsoft Corporation. All rights reserved.
> > + */
> > +#ifndef IPE_POLICY_PARSER_H
> > +#define IPE_POLICY_PARSER_H
> > +
> > +int parse_policy(struct ipe_policy *p);
> > +void free_parsed_policy(struct ipe_parsed_policy *p);
> > +
> > +#endif /* IPE_POLICY_PARSER */
> > --
> > 2.39.0
> 
> --
> paul-moore.com
  

On Thu, Apr 6, 2023 at 4:00 PM Fan Wu <wufan@linux.microsoft.com> wrote:
> 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@linux.microsoft.com> wrote:
> > >
> > > From: Deven Bowers <deven.desai@linux.microsoft.com>
> > >
> > > 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        |  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.

Gotcha.  Thanks for the explanation, that hadn't occurred to me while
I was reviewing the code.

Another option would be to create a macro to help reinforce that the
"max" value is being used as an "invalid" value, for example:

#define IPE_OP_INVALID IPE_OP_MAX