From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: KP Singh <kpsingh@chromium.org>
Cc: "Alexei Starovoitov" <alexei.starovoitov@gmail.com>,
"open list" <linux-kernel@vger.kernel.org>,
bpf <bpf@vger.kernel.org>,
linux-security-module@vger.kernel.org,
"Alexei Starovoitov" <ast@kernel.org>,
"Daniel Borkmann" <daniel@iogearbox.net>,
"James Morris" <jmorris@namei.org>,
"Kees Cook" <keescook@chromium.org>,
"Thomas Garnier" <thgarnie@chromium.org>,
"Michael Halcrow" <mhalcrow@google.com>,
"Paul Turner" <pjt@google.com>,
"Brendan Gregg" <brendan.d.gregg@gmail.com>,
"Jann Horn" <jannh@google.com>,
"Matthew Garrett" <mjg59@google.com>,
"Christian Brauner" <christian@brauner.io>,
"Mickaël Salaün" <mic@digikod.net>,
"Florent Revest" <revest@chromium.org>,
"Brendan Jackman" <jackmanb@chromium.org>,
"Martin KaFai Lau" <kafai@fb.com>,
"Song Liu" <songliubraving@fb.com>, "Yonghong Song" <yhs@fb.com>,
"Serge E. Hallyn" <serge@hallyn.com>,
"Mauro Carvalho Chehab" <mchehab+samsung@kernel.org>,
"David S. Miller" <davem@davemloft.net>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Nicolas Ferre" <nicolas.ferre@microchip.com>,
"Stanislav Fomichev" <sdf@google.com>,
"Quentin Monnet" <quentin.monnet@netronome.com>,
"Andrey Ignatov" <rdna@fb.com>, "Joe Stringer" <joe@wand.net.nz>
Subject: Re: [PATCH bpf-next v1 00/13] MAC and Audit policy using eBPF (KRSI)
Date: Mon, 30 Dec 2019 11:14:29 -0800 [thread overview]
Message-ID: <CAEf4Bzbytf6ZJp_-Y66k5LzB46dBqs=d79VN82mP0UqzBKnDGg@mail.gmail.com> (raw)
In-Reply-To: <20191230145846.GA70684@google.com>
On Mon, Dec 30, 2019 at 6:59 AM KP Singh <kpsingh@chromium.org> wrote:
>
> 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).
I think setting this to member index inside union
security_list_options will be better? Or member index inside struct
security_hook_heads. Seems like kernel will have to "join" those two
anyways, right (one for type info, another for trampoline)? Offset is
less convenient either way.
> * 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?
Semantics of LSM program seems closer to fentry/fexit/raw_tp, so maybe
instead use BPF_RAW_TRACEPOINT_OPEN command? On libbpf side it's all
going to be abstracted behind bpf_program__attach() anyways.
>
> I will clean up the prototype, incorporate some of the other feedback
> received, and send a v2.
>
> Wishing everyone a very Happy New Year!
Thanks, you too!
>
> - KP
>
next prev parent reply other threads:[~2019-12-30 19:14 UTC|newest]
Thread overview: 74+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-20 15:41 [PATCH bpf-next v1 00/13] MAC and Audit policy using eBPF (KRSI) KP Singh
2019-12-20 15:41 ` [PATCH bpf-next v1 01/13] bpf: Refactor BPF_EVENT context macros to its own header KP Singh
2019-12-20 20:10 ` Andrii Nakryiko
2019-12-20 20:26 ` KP Singh
2019-12-20 15:41 ` [PATCH bpf-next v1 02/13] bpf: lsm: Add a skeleton and config options KP Singh
2020-01-07 21:13 ` James Morris
2019-12-20 15:41 ` [PATCH bpf-next v1 03/13] bpf: lsm: Introduce types for eBPF based LSM KP Singh
2019-12-20 15:41 ` [PATCH bpf-next v1 04/13] bpf: lsm: Allow btf_id based attachment for LSM hooks KP Singh
2019-12-23 23:54 ` Andrii Nakryiko
2019-12-30 19:22 ` KP Singh
2019-12-20 15:42 ` [PATCH bpf-next v1 05/13] tools/libbpf: Add support in libbpf for BPF_PROG_TYPE_LSM KP Singh
2019-12-24 0:07 ` Andrii Nakryiko
2019-12-24 0:09 ` Andrii Nakryiko
2020-01-03 23:59 ` KP Singh
2019-12-20 15:42 ` [PATCH bpf-next v1 06/13] bpf: lsm: Init Hooks and create files in securityfs KP Singh
2019-12-24 6:28 ` Andrii Nakryiko
2019-12-30 15:37 ` KP Singh
2019-12-30 18:52 ` Andrii Nakryiko
2019-12-30 19:20 ` Kees Cook
2020-01-03 23:53 ` KP Singh
2020-01-07 21:22 ` James Morris
2019-12-20 15:42 ` [PATCH bpf-next v1 07/13] bpf: lsm: Implement attach, detach and execution KP Singh
2019-12-24 5:48 ` Andrii Nakryiko
2020-01-07 21:27 ` James Morris
2019-12-20 15:42 ` [PATCH bpf-next v1 08/13] bpf: lsm: Show attached program names in hook read handler KP Singh
2020-01-07 21:28 ` James Morris
2019-12-20 15:42 ` [PATCH bpf-next v1 09/13] bpf: lsm: Add a helper function bpf_lsm_event_output KP Singh
2019-12-24 6:36 ` Andrii Nakryiko
2019-12-30 15:11 ` KP Singh
2019-12-30 18:56 ` Andrii Nakryiko
2019-12-20 15:42 ` [PATCH bpf-next v1 10/13] bpf: lsm: Handle attachment of the same program KP Singh
2019-12-24 6:38 ` Andrii Nakryiko
2020-01-08 18:21 ` James Morris
2019-12-20 15:42 ` [PATCH bpf-next v1 11/13] tools/libbpf: Add bpf_program__attach_lsm KP Singh
2019-12-24 6:44 ` Andrii Nakryiko
2020-01-08 18:24 ` James Morris
2019-12-20 15:42 ` [PATCH bpf-next v1 12/13] bpf: lsm: Add selftests for BPF_PROG_TYPE_LSM KP Singh
2019-12-24 6:49 ` Andrii Nakryiko
2020-01-04 0:09 ` KP Singh
2020-01-09 17:59 ` Andrii Nakryiko
2020-01-08 18:25 ` James Morris
2019-12-20 15:42 ` [PATCH bpf-next v1 13/13] bpf: lsm: Add Documentation KP Singh
2019-12-20 17:17 ` [PATCH bpf-next v1 00/13] MAC and Audit policy using eBPF (KRSI) Casey Schaufler
2019-12-20 17:38 ` KP Singh
2019-12-30 19:15 ` Kees Cook
2020-01-08 15:25 ` Stephen Smalley
2020-01-08 18:58 ` James Morris
2020-01-08 19:33 ` Stephen Smalley
2020-01-09 18:11 ` James Morris
2020-01-09 18:23 ` Greg Kroah-Hartman
2020-01-09 18:58 ` Stephen Smalley
2020-01-09 19:07 ` James Morris
2020-01-09 19:43 ` KP Singh
2020-01-09 19:47 ` Stephen Smalley
2020-01-10 15:27 ` KP Singh
2020-01-10 17:48 ` James Morris
2020-01-10 17:53 ` Alexei Starovoitov
2020-01-14 16:54 ` Stephen Smalley
2020-01-14 17:42 ` Stephen Smalley
2020-01-15 2:48 ` Alexei Starovoitov
2020-01-15 13:59 ` Stephen Smalley
2020-01-15 14:09 ` Greg Kroah-Hartman
2020-01-15 22:23 ` Alexei Starovoitov
2020-01-09 19:11 ` KP Singh
2020-01-08 18:27 ` James Morris
2019-12-20 22:46 ` Mickaël Salaün
2019-12-30 19:30 ` Kees Cook
2019-12-31 12:11 ` Mickaël Salaün
2019-12-22 1:27 ` Alexei Starovoitov
2019-12-30 14:58 ` KP Singh
2019-12-30 19:14 ` Andrii Nakryiko [this message]
2019-12-24 6:51 ` Andrii Nakryiko
2019-12-30 15:04 ` KP Singh
2019-12-30 18:58 ` Andrii Nakryiko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAEf4Bzbytf6ZJp_-Y66k5LzB46dBqs=d79VN82mP0UqzBKnDGg@mail.gmail.com' \
--to=andrii.nakryiko@gmail.com \
--cc=alexei.starovoitov@gmail.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=brendan.d.gregg@gmail.com \
--cc=christian@brauner.io \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=gregkh@linuxfoundation.org \
--cc=jackmanb@chromium.org \
--cc=jannh@google.com \
--cc=jmorris@namei.org \
--cc=joe@wand.net.nz \
--cc=kafai@fb.com \
--cc=keescook@chromium.org \
--cc=kpsingh@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=mchehab+samsung@kernel.org \
--cc=mhalcrow@google.com \
--cc=mic@digikod.net \
--cc=mjg59@google.com \
--cc=nicolas.ferre@microchip.com \
--cc=pjt@google.com \
--cc=quentin.monnet@netronome.com \
--cc=rdna@fb.com \
--cc=revest@chromium.org \
--cc=sdf@google.com \
--cc=serge@hallyn.com \
--cc=songliubraving@fb.com \
--cc=thgarnie@chromium.org \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).