Message-ID: <20191230153711.GD70684@google.com>
Date:   Mon, 30 Dec 2019 16:37:11 +0100
From:   KP Singh <kpsingh@...omium.org>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     KP Singh <kpsingh@...omium.org>,
        open list <linux-kernel@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, linux-security-module@...r.kernel.org,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        James Morris <jmorris@...ei.org>,
        Kees Cook <keescook@...omium.org>,
        Thomas Garnier <thgarnie@...omium.org>,
        Michael Halcrow <mhalcrow@...gle.com>,
        Paul Turner <pjt@...gle.com>,
        Brendan Gregg <brendan.d.gregg@...il.com>,
        Jann Horn <jannh@...gle.com>,
        Matthew Garrett <mjg59@...gle.com>,
        Christian Brauner <christian@...uner.io>,
        Mickaël Salaün <mic@...ikod.net>,
        Florent Revest <revest@...omium.org>,
        Brendan Jackman <jackmanb@...omium.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Stanislav Fomichev <sdf@...gle.com>,
        Quentin Monnet <quentin.monnet@...ronome.com>,
        Andrey Ignatov <rdna@...com>, Joe Stringer <joe@...d.net.nz>
Subject: Re: [PATCH bpf-next v1 06/13] bpf: lsm: Init Hooks and create files
 in securityfs

On 23-Dec 22:28, Andrii Nakryiko wrote:
> On Fri, Dec 20, 2019 at 7:43 AM KP Singh <kpsingh@...omium.org> wrote:
> >
> > From: KP Singh <kpsingh@...gle.com>
> >
> > The LSM creates files in securityfs for each hook registered with the
> > LSM.
> >
> >     /sys/kernel/security/bpf/<h_name>
> >
> > The list of LSM hooks are maintained in an internal header "hooks.h"
> > Eventually, this list should either be defined collectively in
> > include/linux/lsm_hooks.h or auto-generated from it.
> >
> > * Creation of a file for the hook in the securityfs.
> > * Allocation of a bpf_lsm_hook data structure which stores
> >   a pointer to the dentry of the newly created file in securityfs.
> > * Creation of a typedef for the hook so that BTF information
> >   can be generated for the LSM hooks to:
> >
> >   - Make them "Compile Once, Run Everywhere".
> >   - Pass the right arguments when the attached programs are run.
> >   - Verify the accesses made by the program by using the BTF
> >     information.
> >
> > Signed-off-by: KP Singh <kpsingh@...gle.com>
> > ---
> >  include/linux/bpf_lsm.h        |   12 +
> >  security/bpf/Makefile          |    4 +-
> >  security/bpf/include/bpf_lsm.h |   63 ++
> >  security/bpf/include/fs.h      |   23 +
> >  security/bpf/include/hooks.h   | 1015 ++++++++++++++++++++++++++++++++
> >  security/bpf/lsm.c             |  138 ++++-
> >  security/bpf/lsm_fs.c          |   82 +++
> >  7 files changed, 1333 insertions(+), 4 deletions(-)
> >  create mode 100644 include/linux/bpf_lsm.h
> >  create mode 100644 security/bpf/include/bpf_lsm.h
> >  create mode 100644 security/bpf/include/fs.h
> >  create mode 100644 security/bpf/include/hooks.h
> >  create mode 100644 security/bpf/lsm_fs.c
> >
> 
> [...]
> 
> > +
> > +/*
> > + * The hooks can have an int or void return type, these macros allow having a
> > + * single implementation of DEFINE_LSM_HOOK irrespective of the return type.
> > + */
> > +#define LSM_HOOK_RET(ret, x) LSM_HOOK_RET_##ret(x)
> > +#define LSM_HOOK_RET_int(x) x
> > +#define LSM_HOOK_RET_void(x)
> > +
> > +/*
> > + * This macro defines the body of a LSM hook which runs the eBPF programs that
> > + * are attached to the hook and returns the error code from the eBPF programs if
> > + * the return type of the hook is int.
> > + */
> > +#define DEFINE_LSM_HOOK(hook, ret, proto, args)                                \
> > +typedef ret (*lsm_btf_##hook)(proto);                                  \
> > +static ret bpf_lsm_##hook(proto)                                       \
> > +{                                                                      \
> > +       return LSM_HOOK_RET(ret, LSM_RUN_PROGS(hook##_type, args));     \
> >  }
> 
> I'm probably missing something, but according to LSM_HOOK_RET
> definition for when ret==void, bpf_lsm_##hook will be a noop and won't
> call any BPF program. Did I miss some additional macro magic?
> 

Good catch! You're right. These macros will not be there in v2 as
we move to using trampolines based callbacks.

> >
> > +/*
> > + * Define the body of each of the LSM hooks defined in hooks.h.
> > + */
> > +#define BPF_LSM_HOOK(hook, ret, args, proto) \
> > +       DEFINE_LSM_HOOK(hook, ret, BPF_LSM_ARGS(args), BPF_LSM_ARGS(proto))
> > +#include "hooks.h"
> > +#undef BPF_LSM_HOOK
> > +#undef DEFINE_LSM_HOOK
> > +
> > +/*
> > + * Initialize the bpf_lsm_hooks_list for each of the hooks defined in hooks.h.
> > + * The list contains information for each of the hook and can be indexed by the
> > + * its type to initialize security FS, attach, detach and execute eBPF programs
> > + * for the hook.
> > + */
> > +struct bpf_lsm_hook bpf_lsm_hooks_list[] = {
> > +       #define BPF_LSM_HOOK(h, ...)                                    \
> > +               [h##_type] = {                                          \
> > +                       .h_type = h##_type,                             \
> > +                       .mutex = __MUTEX_INITIALIZER(                   \
> > +                               bpf_lsm_hooks_list[h##_type].mutex),    \
> > +                       .name = #h,                                     \
> > +                       .btf_hook_func =                                \
> > +                               (void *)(lsm_btf_##h)(bpf_lsm_##h),     \
> 
> this btf_hook_func, is it assigned just so that type information for
> bpf_lsm_xxx typedefs are preserved, is that right? It doesn't seem to
> be ever called or read. If I'm not missing anything, check out
> Martin's latest STRUCT_OPS patch set. He defines EMIT_TYPE_INFO(type)
> macro, which will ensure that BTF for specified type is emitted into
> vmlinux BTF, without actually using any extra space, defining extra
> fields or static variables, etc. I suggest using the same for the
> cleanest result.
> 
> One more thing regarding lsm_bpf_ typedefs. Currently you are defining
> them as a pointer to func_proto, matching LSM hook. There is an
> alternative approach, which has few benefits over using func_proto. If
> instead you define a struct, where each argument of func prototype is
> represented as 8-byte aligned field, this will contain all the
> necessary information for BPF verifier to do its job (just like
> func_proto). But in addition to that, when vmlinux.h is generated, it
> will contain a nice struct bpf_lsm_<hook_name> with correct structure
> to be used **directly** in BPF program, as a single context argument.
> So with vmlinux.h, users won't have to re-define all the argument
> types and names in their BPF_TRACE_x definition. Let me provide
> concrete example from your cover letter. This is what you provide as
> an example:

Is this also doable for the new approach suggsted by Alexei
and prototyped in?

https://lore.kernel.org/bpf/CAEf4BzYiUZtSJKh-UBL0jwyo6d=Cne2YtEyGU8ONykmSUSsuNA@mail.gmail.com/T/#m7c7ec0e7d8e803c6c357495d9eea59028a67cac6

which uses trampolines. The new approach gets rid of any type
generation and macros in security/bpf/lsm_hooks.h. Maybe the
btf_vmlinux can be augmented at runtime to generate context struct
upon attachment?

> 
> BPF_TRACE_3("lsm/file_mprotect", mprotect_audit,
>             struct vm_area_struct *, vma,
>             unsigned long, reqprot, unsigned long, prot) {...}
> 
> on kernel side, you'll have:
> 
> typedef int (*bpf_lsm_file_mprotect)(struct vm_area_struct *vma,
>                                      unsigned long reqprot,
>                                      unsigned long prot);
> 
> So you can see that user has to go and copy/paste all the arguments
> and their types and paste them in this verbose BPF_TRACE_3 macro to
> define correct BPF program.
> 
> Now, imagine that instead of typedef above, we define equivalent struct:
> 
> struct bpf_lsm_file_mprotect {
>     struct vm_area_struct *vma;
>     unsigned long reqprot;
>     unsigned long prot;
> };
> 
> This type will get dumped into vmlinux.h, which can be used from BPF
> user code as such:
> 
> SEC("lsm/file_mprotect")
> int mprotect_audito(struct bpf_lsm_file_mprotect *ctx)
> {
>     ... here you can use ctx->vma, ctx->reqprot, ctx->prot ...
> }
> 
> 
> Meanwhile, there will be just minimal changes to BPF verifier to use
> such struct instead of func_proto for verification of LSM programs.
> 
> We currently have similar issue with raw_tp programs and I've been
> thinking about switching that to structs instead of func_proto, so we
> might as well coordinate that and reuse the same logic in BPF
> verifier.
> 
> Thoughts?

Thanks for the explanation!

Using structs is definitely better if we chose to go with static type
generation.

- KP

> 
> 
> 
> > +               },
> > +       #include "hooks.h"
> > +       #undef BPF_LSM_HOOK
> > +};
> > +
> 
> [...]

Powered by blists - more mailing lists