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
