Message-ID: <20240810155000.GA35219@mail.hallyn.com>
Date: Sat, 10 Aug 2024 10:50:00 -0500
From: "Serge E. Hallyn" <serge@...lyn.com>
To: Fan Wu <wufan@...ux.microsoft.com>
Cc: corbet@....net, zohar@...ux.ibm.com, jmorris@...ei.org,
	serge@...lyn.com, tytso@....edu, ebiggers@...nel.org,
	axboe@...nel.dk, agk@...hat.com, snitzer@...nel.org,
	mpatocka@...hat.com, eparis@...hat.com, paul@...l-moore.com,
	linux-doc@...r.kernel.org, linux-integrity@...r.kernel.org,
	linux-security-module@...r.kernel.org, fsverity@...ts.linux.dev,
	linux-block@...r.kernel.org, dm-devel@...ts.linux.dev,
	audit@...r.kernel.org, linux-kernel@...r.kernel.org,
	Deven Bowers <deven.desai@...ux.microsoft.com>
Subject: Re: [PATCH v20 02/20] ipe: add policy parser

On Fri, Aug 02, 2024 at 11:08:16PM -0700, Fan Wu wrote:
> From: Deven Bowers <deven.desai@...ux.microsoft.com>
> 
> IPE's interpretation of the what the user trusts is accomplished through

nit: "of what the user trusts" (drop the extra 'the')

> 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, or some other system,
> can plug into the policy with minimal code changes.
> 
> Signed-off-by: Deven Bowers <deven.desai@...ux.microsoft.com>
> Signed-off-by: Fan Wu <wufan@...ux.microsoft.com>

This all looks fine.  Just one comment below.


> +/**
> + * parse_rule() - parse a policy rule line.
> + * @line: Supplies rule line to be parsed.
> + * @p: Supplies the partial parsed policy.
> + *
> + * Return:
> + * * 0		- Success
> + * * %-ENOMEM	- Out of memory (OOM)
> + * * %-EBADMSG	- Policy syntax error
> + */
> +static int parse_rule(char *line, struct ipe_parsed_policy *p)
> +{
> +	enum ipe_action_type action = IPE_ACTION_INVALID;
> +	enum ipe_op_type op = IPE_OP_INVALID;
> +	bool is_default_rule = false;
> +	struct ipe_rule *r = NULL;
> +	bool first_token = true;
> +	bool op_parsed = false;
> +	int rc = 0;
> +	char *t;
> +
> +	r = kzalloc(sizeof(*r), GFP_KERNEL);
> +	if (!r)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&r->next);
> +	INIT_LIST_HEAD(&r->props);
> +
> +	while (t = strsep(&line, IPE_POLICY_DELIM), line) {

If line is passed in as NULL, t will be NULL on the first test.  Then
you'll break out and call parse_action(NULL), which calls
match_token(NULL, ...), which I do not think is safe.

I realize the current caller won't pass in NULL, but it seems worth
checking for here in case some future caller is added by someone
who's unaware.

Or, maybe add 'line must not be null' to the function description.

> +		if (*t == '\0')
> +			continue;
> +		if (first_token && token_default(t)) {
> +			is_default_rule = true;
> +		} else {
> +			if (!op_parsed) {
> +				op = parse_operation(t);
> +				if (op == IPE_OP_INVALID)
> +					rc = -EBADMSG;
> +				else
> +					op_parsed = true;
> +			} else {
> +				rc = parse_property(t, r);
> +			}
> +		}
> +
> +		if (rc)
> +			goto err;
> +		first_token = false;
> +	}
> +
> +	action = parse_action(t);
> +	if (action == IPE_ACTION_INVALID) {
> +		rc = -EBADMSG;
> +		goto err;
> +	}
> +
> +	if (is_default_rule) {
> +		if (!list_empty(&r->props)) {
> +			rc = -EBADMSG;
> +		} else if (op == IPE_OP_INVALID) {
> +			if (p->global_default_action != IPE_ACTION_INVALID)
> +				rc = -EBADMSG;
> +			else
> +				p->global_default_action = action;
> +		} else {
> +			if (p->rules[op].default_action != IPE_ACTION_INVALID)
> +				rc = -EBADMSG;
> +			else
> +				p->rules[op].default_action = action;
> +		}
> +	} else if (op != IPE_OP_INVALID && action != IPE_ACTION_INVALID) {
> +		r->op = op;
> +		r->action = action;
> +	} else {
> +		rc = -EBADMSG;
> +	}
> +
> +	if (rc)
> +		goto err;
> +	if (!is_default_rule)
> +		list_add_tail(&r->next, &p->rules[op].rules);
> +	else
> +		free_rule(r);
> +
> +	return rc;
> +err:
> +	free_rule(r);
> +	return rc;
> +}
> +
> +/**
> + * ipe_free_parsed_policy() - free a parsed policy structure.
> + * @p: Supplies the parsed policy.
> + */
> +void ipe_free_parsed_policy(struct ipe_parsed_policy *p)
> +{
> +	struct ipe_rule *pp, *t;
> +	size_t i = 0;
> +
> +	if (IS_ERR_OR_NULL(p))
> +		return;
> +
> +	for (i = 0; i < ARRAY_SIZE(p->rules); ++i)
> +		list_for_each_entry_safe(pp, t, &p->rules[i].rules, next) {
> +			list_del(&pp->next);
> +			free_rule(pp);
> +		}
> +
> +	kfree(p->name);
> +	kfree(p);
> +}
> +
> +/**
> + * validate_policy() - validate a parsed policy.
> + * @p: Supplies the fully parsed policy.
> + *
> + * Given a policy structure that was just parsed, validate that all
> + * operations have their default rules or a global default rule is set.
> + *
> + * Return:
> + * * %0		- Success
> + * * %-EBADMSG	- Policy is invalid
> + */
> +static int validate_policy(const struct ipe_parsed_policy *p)
> +{
> +	size_t i = 0;
> +
> +	if (p->global_default_action != IPE_ACTION_INVALID)
> +		return 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(p->rules); ++i) {
> +		if (p->rules[i].default_action == IPE_ACTION_INVALID)
> +			return -EBADMSG;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * ipe_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		- Success
> + * * %-EBADMSG	- Policy is invalid
> + * * %-ENOMEM	- Out of Memory
> + * * %-ERANGE	- Policy version number overflow
> + * * %-EINVAL	- Policy version parsing error
> + */
> +int ipe_parse_policy(struct ipe_policy *p)
> +{
> +	struct ipe_parsed_policy *pp = NULL;
> +	char *policy = NULL, *dup = NULL;
> +	bool header_parsed = false;
> +	char *line = NULL;
> +	size_t len;
> +	int rc = 0;
> +
> +	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, IPE_LINE_DELIM)) != NULL) {
> +		remove_comment(line);
> +		len = remove_trailing_spaces(line);
> +		if (!len)
> +			continue;
> +
> +		if (!header_parsed) {
> +			rc = parse_header(line, pp);
> +			if (rc)
> +				goto err;
> +			header_parsed = true;
> +		} else {
> +			rc = parse_rule(line, pp);
> +			if (rc)
> +				goto err;
> +		}
> +	}
> +
> +	if (!header_parsed || validate_policy(pp)) {
> +		rc = -EBADMSG;
> +		goto err;
> +	}
> +
> +	p->parsed = pp;
> +
> +out:
> +	kfree(dup);
> +	return rc;
> +err:
> +	ipe_free_parsed_policy(pp);
> +	goto out;
> +}
> diff --git a/security/ipe/policy_parser.h b/security/ipe/policy_parser.h
> new file mode 100644
> index 000000000000..62b6209019a2
> --- /dev/null
> +++ b/security/ipe/policy_parser.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2020-2024 Microsoft Corporation. All rights reserved.
> + */
> +#ifndef _IPE_POLICY_PARSER_H
> +#define _IPE_POLICY_PARSER_H
> +
> +int ipe_parse_policy(struct ipe_policy *p);
> +void ipe_free_parsed_policy(struct ipe_parsed_policy *p);
> +
> +#endif /* _IPE_POLICY_PARSER_H */
> -- 
> 2.44.0

Powered by blists - more mailing lists