Re: [PATCH v2 security-next 1/4] security: Hornet LSM

[Alexei Starovoitov]

On Mon, Apr 14, 2025 at 5:32 PM Blaise Boscaccy
<bboscaccy@xxxxxxxxxxxxxxxxxxx> wrote:
>
> Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> writes:
>
> > On Sat, Apr 12, 2025 at 6:58 AM Blaise Boscaccy
> > <bboscaccy@xxxxxxxxxxxxxxxxxxx> wrote:
> >>
> >> TAlexei Starovoitov <alexei.starovoitov@xxxxxxxxx> writes:
> >>
> >> > On Fri, Apr 4, 2025 at 2:56 PM Blaise Boscaccy
> >> > <bboscaccy@xxxxxxxxxxxxxxxxxxx> 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?

It was exported to modules to run lskel-s from modules.
It's bpf internal api, but seeing how you want to abuse it
the feature has to go. Sadly.

> >> 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/

Not abandoned.
bpf-based tuning of load conditions is still necessary.
The bpf_prog_load command will check the signature only.
It won't start rejecting progs that don't have a signature.
For that a one liner bpf-lsm or C-based lsm would be needed
to address your dont-trust-root use case.

>
> >> 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.

Correct.
For fentry prog type the attachment point is checked during the load,
but for tracepoints it's not, and anyone who is claiming that
their system is secure because the tracepoint prog was signed
is simply clueless in how bpf works.