| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230714214702.GC15267@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
Date: Fri, 14 Jul 2023 14:47:02 -0700
From: Fan Wu <wufan@...ux.microsoft.com>
To: Paul Moore <paul@...l-moore.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,
eparis@...hat.com, linux-doc@...r.kernel.org,
linux-integrity@...r.kernel.org,
linux-security-module@...r.kernel.org,
linux-fscrypt@...r.kernel.org, linux-block@...r.kernel.org,
dm-devel@...hat.com, audit@...r.kernel.org,
roberto.sassu@...wei.com, linux-kernel@...r.kernel.org,
Deven Bowers <deven.desai@...ux.microsoft.com>
Subject: Re: [PATCH RFC v10 4/17] ipe: add LSM hooks on execution and kernel
read
On Sat, Jul 08, 2023 at 12:23:02AM -0400, Paul Moore wrote:
> On Jun 28, 2023 Fan Wu <wufan@...ux.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@...ux.microsoft.com>
> > Signed-off-by: Fan Wu <wufan@...ux.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.
>
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
Powered by blists - more mailing lists