Re: [RFC PATCH v8 02/17] ipe: add policy parser
On Wed, Jun 8, 2022 at 3:03 PM Deven Bowers
<deven.desai(a)linux.microsoft.com> wrote:
IPE's interpretation of the what the user trusts is accomplished through
its policy. IPE's design is to not provide support for a single trust
provider, but to support multiple providers to enable the end-user to
choose the best one to seek their needs.
This requires the policy to be rather flexible and modular so that
integrity providers, like fs-verity, dm-verity, dm-integrity, or
some other system, can plug into the policy with minimal code changes,
and IPE can
Signed-off-by: Deven Bowers <deven.desai(a)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.
---
include/asm-generic/vmlinux.lds.h | 16 +
security/ipe/Makefile | 6 +
security/ipe/ipe.c | 61 ++
security/ipe/ipe.h | 5 +
security/ipe/ipe_parser.h | 59 ++
security/ipe/modules.c | 109 +++
security/ipe/modules.h | 17 +
security/ipe/modules/ipe_module.h | 33 +
security/ipe/parsers.c | 143 ++++
security/ipe/parsers/Makefile | 12 +
security/ipe/parsers/default.c | 106 +++
security/ipe/parsers/policy_header.c | 126 ++++
security/ipe/policy.c | 946 +++++++++++++++++++++++++++
security/ipe/policy.h | 97 +++
14 files changed, 1736 insertions(+)
create mode 100644 security/ipe/ipe_parser.h
create mode 100644 security/ipe/modules.c
create mode 100644 security/ipe/modules.h
create mode 100644 security/ipe/modules/ipe_module.h
create mode 100644 security/ipe/parsers.c
create mode 100644 security/ipe/parsers/Makefile
create mode 100644 security/ipe/parsers/default.c
create mode 100644 security/ipe/parsers/policy_header.c
create mode 100644 security/ipe/policy.c
create mode 100644 security/ipe/policy.h
I had a few small comments while reading through this code, e.g. try
to drop the support for quoted values, but I think my big issue here
is that non-trivial string parsers in the kernel make me nervous and
with +1700 lines spread across 14 files this is definitely a
non-trivial parser.
I understand the basic 'key=value' pair format, and I think that's
okay, but I worry about the added complexity in the parser brought
about by the need to introduce an abstraction layer between the core
parser(s) and modules. I realize flexibility is an important part of
IPE, and this relies on the ability to add support for new language
keys/modules, but I don't believe that requires the level of
indirection seen here.
I'm not asking you to make radical changes to the IPE policy language,
but I do believe spending some time to rethink how you parse the
language would be a good idea. When in doubt keep the parser as
simple as possible, you can always add complexity and more nuance in
the future when the language requires it. The IPE policy language
grammar is the immutable kernel/userspace API promise, not the parser
implementation.
--
paul-moore.com