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

Paul Moore
2023-07-08T05:37:40Z

On Jun 28, 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>
> ---
> security/ipe/eval.c | 14 ++++
> security/ipe/eval.h | 1 +
> security/ipe/hooks.c | 182 +++++++++++++++++++++++++++++++++++++++++++
> security/ipe/hooks.h | 25 ++++++
> security/ipe/ipe.c | 6 ++
> 5 files changed, 228 insertions(+)
> create mode 100644 security/ipe/hooks.c
> create mode 100644 security/ipe/hooks.h

Adding the 'hooks.h' header allows for much of code added in the
previous patches to finally compile and there are a number of errors,
too many to include here. Please fix those and ensure that each
point in the patchset compiles cleanly.

> diff --git a/security/ipe/hooks.c b/security/ipe/hooks.c
> new file mode 100644
> index 000000000000..d896a5a474bc
> --- /dev/null
> +++ b/security/ipe/hooks.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) Microsoft Corporation. All rights reserved.
> + */
> +
> +#include <linux/fs.h>
> +#include <linux/types.h>
> +#include <linux/binfmts.h>
> +#include <linux/mman.h>
> +
> +#include "ipe.h"
> +#include "hooks.h"
> +#include "eval.h"
> +
> +/**
> + * ipe_bprm_check_security - ipe security hook function for bprm check.
> + * @bprm: Supplies a pointer to a linux_binprm structure to source the file
> + * being evaluated.
> + *
> + * This LSM hook is called when a binary is loaded through the exec
> + * family of system calls.
> + * Return:
> + * *0 - OK
> + * *!0 - Error
> + */
> +int ipe_bprm_check_security(struct linux_binprm *bprm)
> +{
> + struct ipe_eval_ctx ctx = { 0 };

It's up to you, but when you have a fequently used initializer like
this it is often wrapped in a macro:

#define IPE_EVAL_CTX_INIT ((struct ipe_eval_ctx){ 0 })

... so that you can write the variable decalaration like this:

struct ipe_eval_ctx ctx = IPE_EVAL_CTX_INIT;

It's not a requirement, it just tends to look a little cleaner and
should you ever need to change the initializer it makes your life
a lot easier.

> + build_eval_ctx(&ctx, bprm->file, __IPE_OP_EXEC);
> + return ipe_evaluate_event(&ctx);
> +}
> +
> +/**
> + * ipe_mmap_file - ipe security hook function for mmap check.
> + * @f: File being mmap'd. Can be NULL in the case of anonymous memory.
> + * @reqprot: The requested protection on the mmap, passed from usermode.
> + * @prot: The effective protection on the mmap, resolved from reqprot and
> + * system configuration.
> + * @flags: Unused.
> + *
> + * This hook is called when a file is loaded through the mmap
> + * family of system calls.
> + *
> + * Return:
> + * * 0 - OK
> + * * !0 - Error
> + */
> +int ipe_mmap_file(struct file *f, unsigned long reqprot, unsigned long prot,
> + unsigned long flags)

Since @reqprot is always going to be unused in this function, you
might want to mark it as such to help prevent compiler
warnings/errors, for example:

unsigned long reqprot __always_unused

> +{
> + struct ipe_eval_ctx ctx = { 0 };
> +
> + if (prot & PROT_EXEC) {
> + build_eval_ctx(&ctx, f, __IPE_OP_EXEC);
> + return ipe_evaluate_event(&ctx);
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * ipe_file_mprotect - ipe security hook function for mprotect check.
> + * @vma: Existing virtual memory area created by mmap or similar.
> + * @reqprot: The requested protection on the mmap, passed from usermode.
> + * @prot: The effective protection on the mmap, resolved from reqprot and
> + * system configuration.
> + *
> + * This LSM hook is called when a mmap'd region of memory is changing
> + * its protections via mprotect.
> + *
> + * Return:
> + * * 0 - OK
> + * * !0 - Error
> + */
> +int ipe_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,

See my comment above about @reqprot.

> + unsigned long prot)
> +{
> + struct ipe_eval_ctx ctx = { 0 };
> +
> + /* Already Executable */
> + if (vma->vm_flags & VM_EXEC)
> + return 0;
> +
> + if (prot & PROT_EXEC) {
> + build_eval_ctx(&ctx, vma->vm_file, __IPE_OP_EXEC);
> + return ipe_evaluate_event(&ctx);
> + }
> +
> + return 0;
> +}

--
paul-moore.com