[lkml]   [2019]   [Dec]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [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:
> 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
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
* 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


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

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

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:

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?

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

 \ /
  Last update: 2019-12-30 15:59    [W:0.199 / U:0.992 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site