From: Ondrej Mosnacek <email@example.com> To: KP Singh <firstname.lastname@example.org> Cc: Linux kernel mailing list <email@example.com>, bpf <firstname.lastname@example.org>, Linux Security Module list <email@example.com>, Alexei Starovoitov <firstname.lastname@example.org>, Daniel Borkmann <email@example.com>, James Morris <firstname.lastname@example.org>, Anders Roxell <email@example.com>, Casey Schaufler <firstname.lastname@example.org>, Peter Zijlstra <email@example.com>, Josh Poimboeuf <firstname.lastname@example.org> Subject: Re: [PATCH bpf] security: Fix hook iteration for secid_to_secctx Date: Fri, 19 Jun 2020 16:16:56 +0200 Message-ID: <CAFqZXNuqNP4OMQGNunyUyyKBc_0-e_P+ogha08V6UsTNCATfLA@mail.gmail.com> (raw) In-Reply-To: <CACYkzJ5e_JOLS-gmNug6e4RJkSsv7sjMUfMWyfMCsQLSoxS8RQ@mail.gmail.com> On Fri, Jun 19, 2020 at 3:13 PM KP Singh <email@example.com> wrote: > Hi, > > On Fri, Jun 19, 2020 at 2:49 PM Ondrej Mosnacek <firstname.lastname@example.org> wrote: > > > > On Wed, May 20, 2020 at 2:56 PM KP Singh <email@example.com> wrote: > > > From: KP Singh <firstname.lastname@example.org> > > > > > > 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: //email@example.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 <firstname.lastname@example.org> > > > Signed-off-by: KP Singh <email@example.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://firstname.lastname@example.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://email@example.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. > > > 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.
next prev parent 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 [this message] 2020-06-21 21:54 ` KP Singh
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=CAFqZXNuqNP4OMQGNunyUyyKBc_0-e_P+ogha08V6UsTNCATfLA@mail.gmail.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.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
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 \ firstname.lastname@example.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