On Sat, Apr 12, 2025 at 9:58 AM Blaise Boscaccy bboscaccy@linux.microsoft.com wrote:
Alexei Starovoitov alexei.starovoitov@gmail.com writes:
On Fri, Apr 4, 2025 at 2:56 PM Blaise Boscaccy bboscaccy@linux.microsoft.com wrote:
...
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.
I agree with Blaise on both the issue of iterating through the eBPF program as well as calling into EXPORT_SYMBOL'd functions; I see no reason why these things couldn't be used in a LSM. These are both "public" interfaces; reading/iterating through the eBPF instructions falls under a "don't break userspace" API, and EXPORT_SYMBOL is essentially public by definition.
It is a bit odd that the eBPF code is creating an exported symbol and not providing a declaration in a kernel wide header file, but that's a different issue.
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.
My understanding from the previous threads is that the recommendation from the BPF devs was that anyone wanting to implement BPF program signature validation at load time should implement a LSM that leverages a light skeleton based loading mechanism and implement a gatekeeper which would authorize BPF program loading based on signatures. From what I can see that is exactly what Blaise has done with Hornet. While Hornet is implemented in C, that alone should not be reason for rejection; from the perspective of the overall LSM framework, we don't accept or reject individual LSMs based on their source language, we have both BPF and C based LSMs today, and we've been working with the Rust folks to ensure we have the right things in place to support Rust in the future. If your response to Hornet is that it isn't acceptable because it is written in C and not BPF, you need to know that such a response isn't an acceptable objection.
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?
I worry that we are now circling back to the original idea of doing BPF program signature validation in the BPF subsystem itself. To be clear, I think that would be okay, if not ultimately preferable, but I think we've all seen this attempted numerous times in the past and it has been delayed, dismissed in favor of alternatives, or simply rejected for one reason or another. If there is a clearly defined path forward for validation of signatures on BPF programs within the context of the BPF subsystem that doesn't require a trusted userspace loader/library/etc. that is one thing, but I don't believe we currently have that, despite user/dev requests for such a feature stretching out over several years.
I believe there are a few questions/issues that have been identified in Hornet's latest round of reviews which may take Blaise a few days (week?) to address; if the BPF devs haven't provided a proposal in which one could acceptably implement in-kernel BPF signature validation by that time, I see no reason why development and review of Hornet shouldn't continue into a v3 revision.