BPF Archive on lore.kernel.org
 help / color / Atom feed
From: Casey Schaufler <casey@schaufler-ca.com>
To: KP Singh <kpsingh@chromium.org>
Cc: Linux Security Module list 
	<linux-security-module@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	James Morris <jmorris@namei.org>,
	Kees Cook <keescook@chromium.org>,
	Casey Schaufler <casey@schaufler-ca.com>
Subject: Re: [PATCH bpf-next v4 0/8] MAC and Audit policy using eBPF (KRSI)
Date: Fri, 21 Feb 2020 14:31:18 -0800
Message-ID: <8a2a2d59-ec4b-80d1-2710-c2ead588e638@schaufler-ca.com> (raw)
In-Reply-To: <20200221194149.GA9207@chromium.org>

On 2/21/2020 11:41 AM, KP Singh wrote:
> On 21-Feb 11:19, Casey Schaufler wrote:
>> On 2/20/2020 9:52 AM, KP Singh wrote:
>>> From: KP Singh <kpsingh@google.com>
>> Again, apologies for the CC list trimming.
>>
>>> # v3 -> v4
>>>
>>>   https://lkml.org/lkml/2020/1/23/515
>>>
>>> * Moved away from allocating a separate security_hook_heads and adding a
>>>   new special case for arch_prepare_bpf_trampoline to using BPF fexit
>>>   trampolines called from the right place in the LSM hook and toggled by
>>>   static keys based on the discussion in:
>>>
>>>     https://lore.kernel.org/bpf/CAG48ez25mW+_oCxgCtbiGMX07g_ph79UOJa07h=o_6B6+Q-u5g@mail.gmail.com/
>>>
>>> * Since the code does not deal with security_hook_heads anymore, it goes
>>>   from "being a BPF LSM" to "BPF program attachment to LSM hooks".
>> I've finally been able to review the entire patch set.
>> I can't imagine how it can make sense to add this much
>> complexity to the LSM infrastructure in support of this
>> feature. There is macro magic going on that is going to
>> break, and soon. You are introducing dependencies on BPF
>> into the infrastructure, and that's unnecessary and most
>> likely harmful.
> We will be happy to document each of the macros in detail. Do note a
> few things here:
>
> * There is really nothing magical about them though,


+#define LSM_HOOK_void(NAME, ...) \
+	noinline void bpf_lsm_##NAME(__VA_ARGS__) {}
+
+#include <linux/lsm_hook_names.h>
+#undef LSM_HOOK

I haven't seen anything this ... novel ... in a very long time.
I see why you want to do this, but you're tying the two sets
of code together unnaturally. When (not if) the two sets diverge
you're going to be introducing another clever way to deal with
the special case.

It's not that I don't understand what you're doing. It's that
I don't like what you're doing. Explanation doesn't make me like
it better.

>  the LSM hooks are
>   collectively declared in lsm_hook_names.h and are used to delcare
>   the security_list_options and security_hook_heads for the LSM
>   framework (this was previously maitained in two different places):
>
>   For BPF, they declare:
>
>     * bpf_lsm_<name> attachment points and their prototypes.
>     * A static key (bpf_lsm_key_<name>) to enable and disable these
>        hooks with a function to set its value i.e.
>        (bpf_lsm_<name>_set_enabled).
>
> * We have kept the BPF related macros out of security/.
> * All the BPF calls in the LSM infrastructure are guarded by
>   CONFIG_BPF_LSM (there are only two main calls though, i.e.
>   call_int_hook, call_void_hook).
>
> Honestly, the macros aren't any more complicated than
> call_int_progs/call_void_progs.
>
> - KP
>
>> Would you please drop the excessive optimization? I understand
>> that there's been a lot of discussion and debate about it,
>> but this implementation is out of control, disruptive, and
>> dangerous to the code around it.
>>
>>


  reply index

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-20 17:52 KP Singh
2020-02-20 17:52 ` [PATCH bpf-next v4 1/8] bpf: Introduce BPF_PROG_TYPE_LSM KP Singh
2020-02-20 17:52 ` [PATCH bpf-next v4 2/8] security: Refactor declaration of LSM hooks KP Singh
2020-02-20 17:52 ` [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs KP Singh
2020-02-21  2:25   ` Alexei Starovoitov
2020-02-21 11:47     ` KP Singh
     [not found]   ` <0ef26943-9619-3736-4452-fec536a8d169@schaufler-ca.com>
2020-02-21 11:44     ` KP Singh
2020-02-21 18:23       ` Casey Schaufler
     [not found]     ` <202002211946.A23A987@keescook>
2020-02-23 22:08       ` Alexei Starovoitov
2020-02-24 16:32         ` Casey Schaufler
2020-02-24 17:13           ` KP Singh
2020-02-24 18:45             ` Casey Schaufler
2020-02-24 21:41               ` Kees Cook
2020-02-24 22:29                 ` Casey Schaufler
2020-02-25  5:41                 ` Alexei Starovoitov
2020-02-25 15:31                   ` Kees Cook
2020-02-25 19:31                   ` KP Singh
2020-02-26  0:30                   ` Casey Schaufler
2020-02-26  5:15                     ` KP Singh
2020-02-26 15:35                       ` Casey Schaufler
2020-02-25 19:29                 ` KP Singh
2020-02-20 17:52 ` [PATCH bpf-next v4 4/8] bpf: lsm: Add support for enabling/disabling BPF hooks KP Singh
2020-02-21 18:57   ` Casey Schaufler
2020-02-21 19:11     ` James Morris
2020-02-22  4:26   ` Kees Cook
2020-02-20 17:52 ` [PATCH bpf-next v4 5/8] bpf: lsm: Implement attach, detach and execution KP Singh
2020-02-21  2:17   ` Alexei Starovoitov
2020-02-21 12:02     ` KP Singh
2020-02-20 17:52 ` [PATCH bpf-next v4 6/8] tools/libbpf: Add support for BPF_PROG_TYPE_LSM KP Singh
2020-02-25  6:45   ` Andrii Nakryiko
2020-02-20 17:52 ` [PATCH bpf-next v4 7/8] bpf: lsm: Add selftests " KP Singh
2020-02-20 17:52 ` [PATCH bpf-next v4 8/8] bpf: lsm: Add Documentation KP Singh
2020-02-21 19:19 ` [PATCH bpf-next v4 0/8] MAC and Audit policy using eBPF (KRSI) Casey Schaufler
2020-02-21 19:41   ` KP Singh
2020-02-21 22:31     ` Casey Schaufler [this message]
2020-02-21 23:09       ` KP Singh
2020-02-21 23:49         ` Casey Schaufler
2020-02-22  0:22       ` Kees Cook
2020-02-22  1:04         ` Casey Schaufler
2020-02-22  3:36           ` Kees Cook
2020-02-27 18:40 ` Dr. Greg

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=8a2a2d59-ec4b-80d1-2710-c2ead588e638@schaufler-ca.com \
    --to=casey@schaufler-ca.com \
    --cc=bpf@vger.kernel.org \
    --cc=jmorris@namei.org \
    --cc=keescook@chromium.org \
    --cc=kpsingh@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    /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

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git