Message-ID: <CAEf4BzaGkbcsL=NCVteeWEPZ5UMEV6vo+DqTn0YF+hg+y7TsDg@mail.gmail.com>
Date:   Mon, 30 Dec 2019 10:52:53 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     KP Singh <kpsingh@...omium.org>
Cc:     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 Mon, Dec 30, 2019 at 7:37 AM KP Singh <kpsingh@...omium.org> wrote:
>
> 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.

Cool, no worries.

>
> > >
> > > +/*
> > > + * 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?

If it doesn't generate "controllable" types (and seems like existing
types are not readily usable as well), then we can't use vmlinux's BTF
as is. But we can augment vmlinux.h header during generation time,
based on naming convention and extra post-processing of vmlinux BTF.
That's sort of a point of special `format core` mode in bpftool, which
we currently discuss on mailing list as well, see [0].

  [0] https://lore.kernel.org/bpf/CAEf4BzY4ffWaeFckPuqNGNAU1uBG3TmTK+CjY1LVa2G+RGz=cA@mail.gmail.com/T/#u


>
> >
> > 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.
>

Yes, that's what I think is preferable for tp_btf programs as well.

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

Powered by blists - more mailing lists