Re: [PATCH bpf-next v1 06/13] bpf: lsm: Init Hooks and create files in securityfs

On Fri, Dec 20, 2019 at 7:43 AM KP Singh [off-list ref] wrote:

From: KP Singh <redacted>

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 <redacted>
---
 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?

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

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?

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

[...]