BPF Archive on lore.kernel.org
 help / color / Atom feed
From: KP Singh <kpsingh@chromium.org>
To: Ondrej Mosnacek <omosnace@redhat.com>
Cc: Linux kernel mailing list <linux-kernel@vger.kernel.org>,
	bpf <bpf@vger.kernel.org>,
	Linux Security Module list 
	<linux-security-module@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	James Morris <jmorris@namei.org>,
	Anders Roxell <anders.roxell@linaro.org>,
	Casey Schaufler <casey@schaufler-ca.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Josh Poimboeuf <jpoimboe@redhat.com>
Subject: Re: [PATCH bpf] security: Fix hook iteration for secid_to_secctx
Date: Sun, 21 Jun 2020 23:54:35 +0200
Message-ID: <CACYkzJ4RidV2p7OxyVoMLnFM=CLuX1MNVyOk82XEHr1QhQFYvA@mail.gmail.com> (raw)
In-Reply-To: <CAFqZXNuqNP4OMQGNunyUyyKBc_0-e_P+ogha08V6UsTNCATfLA@mail.gmail.com>

On Fri, Jun 19, 2020 at 4:17 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Fri, Jun 19, 2020 at 3:13 PM KP Singh <kpsingh@chromium.org> wrote:
> > Hi,
> >
> > On Fri, Jun 19, 2020 at 2:49 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > On Wed, May 20, 2020 at 2:56 PM KP Singh <kpsingh@chromium.org> wrote:
> > > > From: KP Singh <kpsingh@google.com>
> > > >
> > > > secid_to_secctx is not stackable, and since the BPF LSM registers this
> > > > hook by default, the call_int_hook logic is not suitable which
> > > > "bails-on-fail" and casues issues when other LSMs register this hook and
> > > > eventually breaks Audit.
> > > >
> > > > In order to fix this, directly iterate over the security hooks instead
> > > > of using call_int_hook as suggested in:
> > > >
> > > > https: //lore.kernel.org/bpf/9d0eb6c6-803a-ff3a-5603-9ad6d9edfc00@schaufler-ca.com/#t
> > > >
> > > > Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
> > > > Fixes: 625236ba3832 ("security: Fix the default value of secid_to_secctx hook"
> > > > Reported-by: Alexei Starovoitov <ast@kernel.org>
> > > > Signed-off-by: KP Singh <kpsingh@google.com>
> > > [...]
> > >
> > > Sorry for being late to the party, but doesn't this (and the
> > > associated default return value patch) just paper over a bigger
> > > problem? What if I have only the BPF LSM enabled and I attach a BPF
> > > program to this hook that just returns 0? Doesn't that allow anything
> > > privileged enough to do this to force the kernel to try and send
> > > memory from uninitialized pointers to userspace and/or copy such
> > > memory around and/or free uninitialized pointers?
> > >
> > > Why on earth does the BPF LSM directly expose *all* of the hooks, even
> > > those that are not being used for any security decisions (and are
> > > "useful" in this context only for borking the kernel...)? Feel free to
> > > prove me wrong, but this lazy approach of "let's just take all the
> > > hooks as they are and stick BPF programs to them" doesn't seem like a
> >
> > The plan was definitely to not hook everywhere but only call the hooks
> > that do have a BPF program registered. This was one of the versions
> > we proposed in the initial patches where the call to the BPF LSM was
> > guarded by a static key with it being enabled only when there's a
> > BPF program attached to the hook.
> >
> > https://lore.kernel.org/bpf/20200220175250.10795-5-kpsingh@chromium.org/
> >
> > However, this special-cased BPF in the LSM framework, and, was met
> > with opposition. Our plan is to still achieve this, but we want to do this
> > with DEFINE_STATIC_CALL patches:
> >
> > https://lore.kernel.org/lkml/cover.1547073843.git.jpoimboe@redhat.com
> >
> > Using these, only can we enable the call into the hook based on whether
> > a program is attached, we can also eliminate the indirect call overhead which
> > currently affects the "slow" way which was decided in the discussion:
> >
> > https://lore.kernel.org/bpf/202002241136.C4F9F7DFF@keescook/
>
> Perhaps you are misunderstanding me... I don't have a problem with BPF
> LSM registering callbacks for all the hooks. My point is about what
> you can trigger once you attach programs to certain hooks. All the
> above seem to be just optimizations/implementation details that do not
> affect the problem I'm pointing to.
>

The immediate concern was to fix the issue caused by the default
callback (bpf_lsm_secid_to_secctx) which affected even the users
who were not deliberately attaching a BPF program to the hook.

We can probably restrict attachment of BPF programs to be fexit trampolines
instead of fmod_ret by using some of the work that is being done for
BTF ID whitelists for the d_path helper. (fexit trampolines cannot
change the return
value of the default callback).

https://lore.kernel.org/bpf/20200616100512.2168860-9-jolsa@kernel.org/

With some of the optimization work which should remove this
default callback, we can also prevent the non-deliberate errors (the ones
that occur even when a BPF program is not attached to the LSM hook).
IMHO, these are more important to fix.

- KP

> >
> > > good choice... IMHO you should either limit the set of hooks that can
> > > be attached to only those that aren't used to return back values via
> >
> > I am not sure if limiting the hooks is required here once we have
> > the ability to call into BPF only when a program is attached. If the
> > the user provides a BPF program, deliberately returns 0 (or any
> > other value) then it is working as intended. Even if we limit this in the
> > bpf LSM, deliberate privileged users can still achieve this with
> > other means.
>
> The point is that for this particular hook (secid_to_secctx) and a
> couple others, the consequences of having control over the return
> value are more serious than with other hooks. For most hooks, the
> implementation usually just returns 0 (OK), -EACCESS (access denied)
> or -E... (error) and the caller either continues as normal or handles
> the error. But here if you return 0, you signal that you have
> initialized the pointer and size to valid values. So suddenly the BPF
> prog doesn't just control allow/deny decisions, but can now easily
> trigger kernel panic. And when you look at the semantics of the hook,
> you will realize that it doesn't really make sense to implement it via
> BPF, since it can never populate the output values and the only
> meaningful implementation would be to just return -EOPNOTSUPP.
>
> Maybe I have it all wrong, but isn't the whole point of BPF programs
> to provide a tight sandbox where you can only implement pure input ->
> output functions + read/modify some internal state? Is it really
> "working as intended" if you can crash the kernel by attaching a
> simple BPF program to a certain hook? I mean yes, you can make the
> system pretty much unusable already using the classic hooks by simply
> returning -EACCESS for everything, but IMO that's quite different from
> causing the kernel to do an invalid memory access.
>
> --
> Ondrej Mosnacek
> Software Engineer, Platform Security - SELinux kernel
> Red Hat, Inc.
>

      reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-20 12:56 KP Singh
2020-05-20 15:15 ` Casey Schaufler
2020-05-21  1:35   ` Alexei Starovoitov
2020-05-21  2:02     ` James Morris
2020-05-21  3:12       ` Alexei Starovoitov
2020-06-19 12:49 ` Ondrej Mosnacek
2020-06-19 13:13   ` KP Singh
2020-06-19 14:16     ` Ondrej Mosnacek
2020-06-21 21:54       ` KP Singh [this message]

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='CACYkzJ4RidV2p7OxyVoMLnFM=CLuX1MNVyOk82XEHr1QhQFYvA@mail.gmail.com' \
    --to=kpsingh@chromium.org \
    --cc=anders.roxell@linaro.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=casey@schaufler-ca.com \
    --cc=daniel@iogearbox.net \
    --cc=jmorris@namei.org \
    --cc=jpoimboe@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=omosnace@redhat.com \
    --cc=peterz@infradead.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