Messages in this thread |  | | From | KP Singh <> | Date | Mon, 30 Dec 2019 15:58:46 +0100 | Subject | Re: [PATCH bpf-next v1 00/13] MAC and Audit policy using eBPF (KRSI) |
| |
On 21-Dez 17:27, Alexei Starovoitov wrote: > 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; > > } >
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/
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
|  |