TAlexei Starovoitov <alexei.starovoi...@gmail.com> writes:
> On Fri, Apr 4, 2025 at 2:56 PM Blaise Boscaccy
> <bbosca...@linux.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. 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.
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, without adding extra non-code
signing requirements like attachment point verification, completely
eBPF-based solutions, or rich eBPF-based program run-time policy
enforcement?
Our entire use case for this is simply "we've signed all code running in
ring 0," nothing more. I'm concerned that code signing may be blocked
forever while eBPF attempts to reinvent its own runtime policy framework
that has absolutely nothing to do with proving code provenance.
> What LSM can do in addition is to say that if the signature is not
> specified in the prog_load command then deny such request outright.
> bpf syscall itself will deny program loading if signature is incorrect.
