Re: [PATCH RFC v11 4/19] ipe: add LSM hooks on execution and kernel read

Paul Moore
2023-10-24T03:52:30Z

On Oct 4, 2023 Fan Wu <wufan@linux.microsoft.com> wrote:
>
> IPE's initial goal is to control both execution and the loading of
> kernel modules based on the system's definition of trust. It
> accomplishes this by plugging into the security hooks for
> bprm_check_security, file_mprotect, mmap_file, kernel_load_data,
> and kernel_read_data.
>
> 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 ipe_load_properties to patch 04.
> + Remove useless 0-initializations
> + Prefix extern variables with ipe_
> + Remove kernel module parameters, as these are
> exposed through sysctls.
> + Add more prose to the IPE base config option
> help text.
> + Use GFP_KERNEL for audit_log_start.
> + Remove unnecessary caching system.
> + Remove comments from headers
> + Use rcu_access_pointer for rcu-pointer null check
> + Remove usage of reqprot; use prot only.
> + Move policy load and activation audit event to 03/12
>
> v4:
> + Remove sysctls in favor of securityfs nodes
> + Re-add kernel module parameters, as these are now
> exposed through securityfs.
> + Refactor property audit loop to a separate function.
>
> v5:
> + fix minor grammatical errors
> + do not group rule by curly-brace in audit record,
> reconstruct the exact rule.
>
> v6:
> + No changes
>
> v7:
> + Further split lsm creation, the audit system, the evaluation loop
> and access control hooks into separate commits.
>
> v8:
> + Rename hook functions to follow the lsmname_hook_name convention
> + Remove ipe_hook enumeration, can be derived from correlation with
> syscall audit record.
>
> v9:
> + Minor changes for adapting to the new parser
>
> v10:
> + Remove @reqprot part
>
> v11:
> + Fix code style issues
> ---
> security/ipe/Makefile | 1 +
> security/ipe/eval.c | 14 ++++
> security/ipe/eval.h | 3 +
> security/ipe/hooks.c | 183 ++++++++++++++++++++++++++++++++++++++++++
> security/ipe/hooks.h | 25 ++++++
> security/ipe/ipe.c | 6 ++
> 6 files changed, 232 insertions(+)
> create mode 100644 security/ipe/hooks.c
> create mode 100644 security/ipe/hooks.h

...

> diff --git a/security/ipe/hooks.c b/security/ipe/hooks.c
> new file mode 100644
> index 000000000000..6164a9b53361
> --- /dev/null
> +++ b/security/ipe/hooks.c
> @@ -0,0 +1,183 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) Microsoft Corporation. All rights reserved.
> + */

...

> +/**
> + * ipe_kernel_read_file - ipe security hook function for kernel read.
> + * @file: Supplies a pointer to the file structure being read in from disk.
> + * @id: Supplies the enumeration identifying the purpose of the read.
> + * @contents: Unused.
> + *
> + * This LSM hook is called when a file is being read in from disk from
> + * the kernel.
> + *
> + * Return:
> + * 0 - OK
> + * !0 - Error
> + */
> +int ipe_kernel_read_file(struct file *file, enum kernel_read_file_id id,
> + bool contents)
> +{
> + enum ipe_op_type op;
> + struct ipe_eval_ctx ctx = IPE_EVAL_CTX_INIT;
> +
> + switch (id) {
> + case READING_FIRMWARE:
> + op = IPE_OP_FIRMWARE;
> + break;
> + case READING_MODULE:
> + op = IPE_OP_KERNEL_MODULE;
> + break;
> + case READING_KEXEC_INITRAMFS:
> + op = IPE_OP_KEXEC_INITRAMFS;
> + break;
> + case READING_KEXEC_IMAGE:
> + op = IPE_OP_KEXEC_IMAGE;
> + break;
> + case READING_POLICY:
> + op = IPE_OP_IMA_POLICY;
> + break;
> + case READING_X509_CERTIFICATE:
> + op = IPE_OP_IMA_X509;
> + break;
> + default:
> + op = IPE_OP_INVALID;
> + WARN(op == IPE_OP_INVALID, "no rule setup for enum %d", id);

I'm not sure you need to test @op above since you set @op on the line
above, just use true/1 to simplify things. It also seems like it
might be helpful to provice some context for the enum above in the
WARN() message. For example:

WARN(1, "no rule setup for kernel_read_file %d", id);

> + }
> +
> + build_eval_ctx(&ctx, file, op);
> + return ipe_evaluate_event(&ctx);
> +}
> +
> +/**
> + * ipe_kernel_load_data - ipe security hook function for kernel load data.
> + * @id: Supplies the enumeration identifying the purpose of the read.
> + * @contents: Unused.
> + *
> + * This LSM hook is called when a buffer is being read in from disk.
> + *
> + * Return:
> + * * 0 - OK
> + * * !0 - Error
> + */
> +int ipe_kernel_load_data(enum kernel_load_data_id id, bool contents)
> +{
> + enum ipe_op_type op;
> + struct ipe_eval_ctx ctx = IPE_EVAL_CTX_INIT;
> +
> + switch (id) {
> + case LOADING_FIRMWARE:
> + op = IPE_OP_FIRMWARE;
> + break;
> + case LOADING_MODULE:
> + op = IPE_OP_KERNEL_MODULE;
> + break;
> + case LOADING_KEXEC_INITRAMFS:
> + op = IPE_OP_KEXEC_INITRAMFS;
> + break;
> + case LOADING_KEXEC_IMAGE:
> + op = IPE_OP_KEXEC_IMAGE;
> + break;
> + case LOADING_POLICY:
> + op = IPE_OP_IMA_POLICY;
> + break;
> + case LOADING_X509_CERTIFICATE:
> + op = IPE_OP_IMA_X509;
> + break;
> + default:
> + op = IPE_OP_INVALID;
> + WARN(op == IPE_OP_INVALID, "no rule setup for enum %d", id);

See my comments in ipe_kernel_read_file(), they also apply here.

> + }
> +
> + build_eval_ctx(&ctx, NULL, op);
> + return ipe_evaluate_event(&ctx);
> +}

--
paul-moore.com