On 21-Dez 17:27, Alexei Starovoitov wrote:

On Fri, Dec 20, 2019 at 04:41:55PM +0100, KP Singh wrote:

quoted

// 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;
}

Hi Alexei,

Thanks for the feedback. This is really helpful!

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.

Really cool!

I looked into how BPF trampolines are being used in tracing and the
new STRUCT_OPS patchset and was able protoype
(https://github.com/sinkap/linux-krsi/tree/patch/v1/trampoline_prototype,
not ready for review yet) which:

* Gets rid of security/bpf/include/hooks.h and all of the static
  macro magic essentially making the LSM ~truly instrumentable~ at
  runtime.
* Gets rid of the generation of any new types as we already have
  all the BTF information in the kernel in the following two types:

struct security_hook_heads {
        .
        .
        struct hlist_head file_mprotect;   <- Append the callback at this offset
        .
        .
};

and

union security_list_options {
	int (*file_mprotect)(struct vm_area_struct *vma, unsigned long reqprot,
				unsigned long prot);
};

Which is the same type as the typedef that's currently being generated
, i.e. lsm_btf_file_mprotect

In the current prototype, libbpf converts the name of the hook into an
offset into the security_hook_heads and the verifier does the
following when a program is loaded:

* Verifies the offset and the type at the offset (struct hlist_head).
* Resolves the func_proto (by looking up the type in
  security_list_options) and updates prog->aux with the name and
  func_proto which are then verified similar to raw_tp programs with
  btf_ctx_access.

On attachment:

* A trampoline is created and appended to the security_hook_heads
  for the BPF LSM.
* An anonymous FD is returned and the attachment is conditional on the
  references to FD (as suggested and similar to fentry/fexit tracing
  programs).

This implies that the BPF programs are "the LSM hook" as opposed to
being executed inside a statically defined hook body which requires
mutable LSM hooks for which I was able to re-use some of ideas in
Sargun's patch:

https://lore.kernel.org/lkml/20180408065916.GA2832@ircssh-2.c.rugged-nimbus-611.internal/ (local)

to maintain a separate security_hook_heads struct for dynamically
added LSM hooks by the BPF LSM which are executed after all the
statically defined hooks.

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.

I agree, based on the new feedback, I don't think we need securityFS
attachment points anymore. I was able to get rid of it completely.

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.

I like this. This also means we don't immediately need the handling of
duplicate names so I dropped that bit of the patch as well and updated
the attachment to use this mechanism.

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.

There are some choices we need to make here from an API perspective:

* Should we "repurpose" attr->attach_btf_id and use it as an offset
  into security_hook_heads or add a new attribute
  (e.g lsm_hook_offset) for the offset or use name of the LSM hook
  (e.g. lsm_hook_name).
* Since we don't have the files in securityFS, the attachment does not
  have a target_fd. Should we add a new type of BPF command?
  e.g. LSM_HOOK_OPEN?

I will clean up the prototype, incorporate some of the other feedback
received, and send a v2.

Wishing everyone a very Happy New Year!

- KP