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

[Fan Wu]

On Sat, Jul 08, 2023 at 12:23:02AM -0400, Paul Moore wrote:
> On Jun 28, 2023 Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx>
> > ---
> >  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.
> 
Sorry again for the mistake I made here.

> > 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.
> 
Yes I agree this looks way better, I will update all the context init.

> > +	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
> 
Thanks for telling me this useful mark! I will add it.

-Fan
> > +{
> > +	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