On 23-Dec 22:28, Andrii Nakryiko wrote:

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

quoted

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

[...]

quoted

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

quoted

+/*
+ * 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 (local)

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

quoted

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

[...]