Message-ID: <87friajmd5.fsf@microsoft.com>
Date: Mon, 14 Apr 2025 17:32:06 -0700
From: Blaise Boscaccy <bboscaccy@...ux.microsoft.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Jonathan Corbet <corbet@....net>, David Howells <dhowells@...hat.com>,
 Herbert Xu <herbert@...dor.apana.org.au>, "David S. Miller"
 <davem@...emloft.net>, Paul Moore <paul@...l-moore.com>, James Morris
 <jmorris@...ei.org>, "Serge E. Hallyn" <serge@...lyn.com>, Masahiro Yamada
 <masahiroy@...nel.org>, Nathan Chancellor <nathan@...nel.org>, Nicolas
 Schier <nicolas@...sle.eu>, Shuah Khan <shuah@...nel.org>, Mickaël Salaün
 <mic@...ikod.net>, Günther Noack <gnoack@...gle.com>, Nick
 Desaulniers
 <nick.desaulniers+lkml@...il.com>, Bill Wendling <morbo@...gle.com>,
 Justin Stitt <justinstitt@...gle.com>, Jarkko Sakkinen
 <jarkko@...nel.org>, Jan Stancek <jstancek@...hat.com>, Neal Gompa
 <neal@...pa.dev>, "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
 LKML <linux-kernel@...r.kernel.org>, keyrings@...r.kernel.org, Linux
 Crypto Mailing List <linux-crypto@...r.kernel.org>, LSM List
 <linux-security-module@...r.kernel.org>, Linux Kbuild mailing list
 <linux-kbuild@...r.kernel.org>, "open list:KERNEL SELFTEST FRAMEWORK"
 <linux-kselftest@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
 clang-built-linux <llvm@...ts.linux.dev>, nkapron@...gle.com, Matteo Croce
 <teknoraver@...a.com>, Roberto Sassu <roberto.sassu@...wei.com>, Cong Wang
 <xiyou.wangcong@...il.com>
Subject: Re: [PATCH v2 security-next 1/4] security: Hornet LSM

Alexei Starovoitov <alexei.starovoitov@...il.com> writes:

> On Sat, Apr 12, 2025 at 6:58 AM Blaise Boscaccy
> <bboscaccy@...ux.microsoft.com> wrote:
>>
>> TAlexei Starovoitov <alexei.starovoitov@...il.com> writes:
>>
>> > On Fri, Apr 4, 2025 at 2:56 PM Blaise Boscaccy
>> > <bboscaccy@...ux.microsoft.com> wrote:
>> >> +
>> >> +static int hornet_find_maps(struct bpf_prog *prog, struct hornet_maps *maps)
>> >> +{
>> >> +       struct bpf_insn *insn = prog->insnsi;
>> >> +       int insn_cnt = prog->len;
>> >> +       int i;
>> >> +       int err;
>> >> +
>> >> +       for (i = 0; i < insn_cnt; i++, insn++) {
>> >> +               if (insn[0].code == (BPF_LD | BPF_IMM | BPF_DW)) {
>> >> +                       switch (insn[0].src_reg) {
>> >> +                       case BPF_PSEUDO_MAP_IDX_VALUE:
>> >> +                       case BPF_PSEUDO_MAP_IDX:
>> >> +                               err = add_used_map(maps, insn[0].imm);
>> >> +                               if (err < 0)
>> >> +                                       return err;
>> >> +                               break;
>> >> +                       default:
>> >> +                               break;
>> >> +                       }
>> >> +               }
>> >> +       }
>> >
>> > ...
>> >
>> >> +               if (!map->frozen) {
>> >> +                       attr.map_fd = fd;
>> >> +                       err = kern_sys_bpf(BPF_MAP_FREEZE, &attr, sizeof(attr));
>> >
>> > Sorry for the delay. Still swamped after conferences and the merge window.
>> >
>>
>> No worries.
>>
>> > Above are serious layering violations.
>> > LSMs should not be looking that deep into bpf instructions.
>>
>> These aren't BPF internals; this is data passed in from
>> userspace. Inspecting userspace function inputs is definitely within the
>> purview of an LSM.
>>
>> Lskel signature verification doesn't actually need a full disassembly,
>> but it does need all the maps used by the lskel. Due to API design
>> choices, this unfortunately requires disassembling the program to see
>> which array indexes are being used.
>>
>> > Calling into sys_bpf from LSM is plain nack.
>> >
>>
>> kern_sys_bpf is an EXPORT_SYMBOL, which means that it should be callable
>> from a module.
>
> It's a leftover.
> kern_sys_bpf() is not something that arbitrary kernel
> modules should call.
> It was added to work for cases where kernel modules
> carry their own lskels.
> That use case is gone, so EXPORT_SYMBOL will be removed.
>

I'm not following that at all. You recommended using module-based lskels
to get around code signing requirements at lsfmmbpf and now you want to
nuke that entire feature? And further, skel_internal will no longer be
usable from within the kernel and bpf_preload is no longer going to be
supported?

>> Lskels without frozen maps are vulnerable to a TOCTOU
>> attack from a sufficiently privileged user. Lskels currently pass
>> unfrozen maps into the kernel, and there is nothing stopping someone
>> from modifying them between BPF_PROG_LOAD and BPF_PROG_RUN.
>>
>> > The verification of module signatures is a job of the module loading process.
>> > The same thing should be done by the bpf system.
>> > The signature needs to be passed into sys_bpf syscall
>> > as a part of BPF_PROG_LOAD command.
>> > It probably should be two new fields in union bpf_attr
>> > (signature and length),
>> > and the whole thing should be processed as part of the loading
>> > with human readable error reported back through the verifier log
>> > in case of signature mismatch, etc.
>> >
>>
>> I don't necessarily disagree, but my main concern with this is that
>> previous code signing patchsets seem to get gaslit or have the goalposts
>> moved until they die or are abandoned.
>
> Previous attempts to add signing failed because
> 1. It's a difficult problem to solve
> 2. people only cared about their own narrow use case and not
> considering the needs of bpf ecosystem as a whole.
>
>> Are you saying that at this point, you would be amenable to an in-tree
>> set of patches that enforce signature verification of lskels during
>> BPF_PROG_LOAD that live in syscall.c,
>
> that's the only way to do it.
>

So the notion of forcing people into writing bpf-based gatekeeper programs
is being abandoned? e.g.

https://lore.kernel.org/bpf/bqxgv2tqk3hp3q3lcdqsw27btmlwqfkhyg6kohsw7lwdgbeol7@nkbxnrhpn7qr/#t
https://lore.kernel.org/bpf/61aae2da8c7b0_68de0208dd@john.notmuch/


>> without adding extra non-code
>> signing requirements like attachment point verification, completely
>> eBPF-based solutions, or rich eBPF-based program run-time policy
>> enforcement?
>
> Those are secondary considerations that should also be discussed.
> Not necessarily a blocker.

Again, I'm confused here since you recently stated this whole thing
was "questionable" without attachment point verification.

Powered by blists - more mailing lists