Re: [PATCH bpf-next v1 00/13] MAC and Audit policy using eBPF (KRSI)

[Alexei Starovoitov]

On Fri, Dec 20, 2019 at 04:41:55PM +0100, KP Singh wrote:
> // Declare the eBPF program mprotect_audit which attaches to
> // to the file_mprotect LSM hook and accepts three arguments.
> BPF_TRACE_3("lsm/file_mprotect", mprotect_audit,
> 	    struct vm_area_struct *, vma,
> 	    unsigned long, reqprot, unsigned long, prot
> {
> 	unsigned long vm_start = _(vma->vm_start);
> 	return 0;
> }

I think the only sore point of the patchset is:
security/bpf/include/hooks.h   | 1015 ++++++++++++++++++++++++++++++++
With bpf trampoline this type of 'kernel types -> bpf types' converters
are no longer necessary. Please take a look at tcp-congestion-control patchset:
https://patchwork.ozlabs.org/cover/1214417/
Instead of doing similar thing (like your patch 1 plus patch 6) it's using
trampoline to provide bpf based congestion control callbacks into tcp stack.
The same trampoline-based mechanism can be reused by bpf_lsm.
Then all manual work of doing BPF_LSM_HOOK(...) for every hook won't be
necessary. It will also prove the point that attaching BPF to raw LSM hooks
doesn't freeze them into stable abi.
The programs can keep the same syntax as in your examples:
BPF_TRACE_3("lsm/file_mprotect", mprotect_audit,
libbpf will find file_mprotect's btf_id in kernel vmlinux and pass it to
the kernel for attaching. Just like fentry/fexit bpf progs are doing
and just like bpf-based cc is doing as well.

> In order to better illustrate the capabilities of the framework some
> more advanced prototype code has also been published separately:
> 
> * Logging execution events (including environment variables and arguments):
> https://github.com/sinkap/linux-krsi/blob/patch/v1/examples/samples/bpf/lsm_audit_env.c
> * Detecting deletion of running executables:
> https://github.com/sinkap/linux-krsi/blob/patch/v1/examples/samples/bpf/lsm_detect_exec_unlink.c
> * Detection of writes to /proc/<pid>/mem:
> https://github.com/sinkap/linux-krsi/blob/patch/v1/examples/samples/bpf/lsm_audit_env.c

Thank you for sharing these examples. That definitely helps to see more
complete picture. I noticed that the examples are using the pattern:
  u32 map_id = 0;
  env = bpf_map_lookup_elem(&env_map, &map_id);
Essentially they're global variables. libbpf already supports them.
bpf prog can use them as:
  struct env_value env;
  int bpf_prog(..)
  {
    env.name... env.value..
  }
That will make progs a bit easier to read and faster too.
Accesing global vars from user space is also trivial with skeleton work:
  lsm_audit_env_skel->bss->env.name... env.value.
Both bpf side and user side can access globals as normal C variables.

There is a small issue in the patches 8 and 10.
bpf program names are not unique and bpf-lsm should not require them to be different.
bpf_attr->prog_name is also short at 16 bytes. It's for introspection only.
Longer program names are supplied via btf's func_info.
It feels that:
cat /sys/kernel/security/bpf/process_execution
env_dumper__v2
is reinventing the wheel. bpftool is the main introspection tool.
It can print progs attached to perf, cgroup, networking. I think it's better to
stay consistent and do the same with bpf-lsm.

Another issue is in proposed attaching method:
hook_fd = open("/sys/kernel/security/bpf/process_execution");
sys_bpf(attach, prog_fd, hook_fd);
With bpf tracing we moved to FD-based attaching, because permanent attaching is
problematic in production. We're going to provide FD-based api to attach to
networking as well, because xdp/tc/cgroup prog attaching suffers from the same
production issues. Mainly with permanent attaching there is no ownership of
attachment. Everything is global and permanent. It's not clear what
process/script suppose to detach/cleanup. I suggest bpf-lsm use FD-based
attaching from the beginning. Take a look at raw_tp/tp_btf/fentry/fexit style
of attaching. All of them return FD which represents what libbpf calls
'bpf_link' concept. Once refcnt of that FD goes to zero that link (attachment)
is destroyed and program is detached _by the kernel_. To make such links
permanent the application can pin them in bpffs. The pinning patches haven't
landed yet, but the concept of the link is quite powerful and much more
production friendly than permanent attaching.
bpf-lsm will still be able to attach multiple progs to the same hook and
see what is attached via bpftool.

The rest looks good. Thank you for working on it.