| Message ID | 20250404215527.1563146-2-bboscaccy@linux.microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introducing Hornet LSM | expand |
Hi Blaise, kernel test robot noticed the following build errors: [auto build test ERROR on shuah-kselftest/next] [also build test ERROR on shuah-kselftest/fixes herbert-cryptodev-2.6/master herbert-crypto-2.6/master masahiroy-kbuild/for-next masahiroy-kbuild/fixes v6.14] [cannot apply to linus/master next-20250404] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Blaise-Boscaccy/security-Hornet-LSM/20250405-055741 base: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git next patch link: https://lore.kernel.org/r/20250404215527.1563146-2-bboscaccy%40linux.microsoft.com patch subject: [PATCH v2 security-next 1/4] security: Hornet LSM config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20250406/202504061441.FMnrO665-lkp@intel.com/config) compiler: sh4-linux-gcc (GCC) 14.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250406/202504061441.FMnrO665-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202504061441.FMnrO665-lkp@intel.com/ All errors (new ones prefixed by >>): In file included from security/hornet/hornet_lsm.c:10: >> security/hornet/hornet_lsm.c:221:38: error: initialization of 'int (*)(struct bpf_prog *, union bpf_attr *, struct bpf_token *)' from incompatible pointer type 'int (*)(struct bpf_prog *, union bpf_attr *, struct bpf_token *, bool)' {aka 'int (*)(struct bpf_prog *, union bpf_attr *, struct bpf_token *, _Bool)'} [-Wincompatible-pointer-types] 221 | LSM_HOOK_INIT(bpf_prog_load, hornet_bpf_prog_load), | ^~~~~~~~~~~~~~~~~~~~ include/linux/lsm_hooks.h:136:35: note: in definition of macro 'LSM_HOOK_INIT' 136 | .hook = { .NAME = HOOK } \ | ^~~~ security/hornet/hornet_lsm.c:221:38: note: (near initialization for 'hornet_hooks[0].hook.bpf_prog_load') 221 | LSM_HOOK_INIT(bpf_prog_load, hornet_bpf_prog_load), | ^~~~~~~~~~~~~~~~~~~~ include/linux/lsm_hooks.h:136:35: note: in definition of macro 'LSM_HOOK_INIT' 136 | .hook = { .NAME = HOOK } \ | ^~~~ vim +221 security/hornet/hornet_lsm.c 219 220 static struct security_hook_list hornet_hooks[] __ro_after_init = { > 221 LSM_HOOK_INIT(bpf_prog_load, hornet_bpf_prog_load), 222 }; 223
Hi Blaise, kernel test robot noticed the following build errors: [auto build test ERROR on shuah-kselftest/next] [also build test ERROR on shuah-kselftest/fixes herbert-cryptodev-2.6/master herbert-crypto-2.6/master masahiroy-kbuild/for-next masahiroy-kbuild/fixes v6.14] [cannot apply to linus/master next-20250404] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Blaise-Boscaccy/security-Hornet-LSM/20250405-055741 base: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git next patch link: https://lore.kernel.org/r/20250404215527.1563146-2-bboscaccy%40linux.microsoft.com patch subject: [PATCH v2 security-next 1/4] security: Hornet LSM config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20250407/202504070413.eDHSjWGP-lkp@intel.com/config) compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250407/202504070413.eDHSjWGP-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202504070413.eDHSjWGP-lkp@intel.com/ All errors (new ones prefixed by >>): >> security/hornet/hornet_lsm.c:221:31: error: incompatible function pointer types initializing 'int (*)(struct bpf_prog *, union bpf_attr *, struct bpf_token *)' with an expression of type 'int (struct bpf_prog *, union bpf_attr *, struct bpf_token *, bool)' (aka 'int (struct bpf_prog *, union bpf_attr *, struct bpf_token *, _Bool)') [-Wincompatible-function-pointer-types] 221 | LSM_HOOK_INIT(bpf_prog_load, hornet_bpf_prog_load), | ^~~~~~~~~~~~~~~~~~~~ include/linux/lsm_hooks.h:136:21: note: expanded from macro 'LSM_HOOK_INIT' 136 | .hook = { .NAME = HOOK } \ | ^~~~ 1 error generated. vim +221 security/hornet/hornet_lsm.c 219 220 static struct security_hook_list hornet_hooks[] __ro_after_init = { > 221 LSM_HOOK_INIT(bpf_prog_load, hornet_bpf_prog_load), 222 }; 223
On 2025-04-04 14:54:50, Blaise Boscaccy wrote: > +static int hornet_verify_lskel(struct bpf_prog *prog, struct hornet_maps *maps, > + void *sig, size_t sig_len) > +{ > + int fd; > + u32 i; > + void *buf; > + void *new; > + size_t buf_sz; > + struct bpf_map *map; > + int err = 0; > + int key = 0; > + union bpf_attr attr = {0}; > + > + buf = kmalloc_array(prog->len, sizeof(struct bpf_insn), GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + buf_sz = prog->len * sizeof(struct bpf_insn); > + memcpy(buf, prog->insnsi, buf_sz); > + > + for (i = 0; i < maps->used_map_cnt; i++) { > + err = copy_from_bpfptr_offset(&fd, maps->fd_array, > + maps->used_idx[i] * sizeof(fd), > + sizeof(fd)); > + if (err < 0) > + continue; > + if (fd < 1) > + continue; > + > + map = bpf_map_get(fd); I'm not very familiar with BPF map lifetimes but I'd assume we need to have a corresponding bpf_map_put(map) before returning. > + if (IS_ERR(map)) > + continue; > + > + /* don't allow userspace to change map data used for signature verification */ > + if (!map->frozen) { > + attr.map_fd = fd; > + err = kern_sys_bpf(BPF_MAP_FREEZE, &attr, sizeof(attr)); > + if (err < 0) > + goto out; > + } > + > + new = krealloc(buf, buf_sz + map->value_size, GFP_KERNEL); > + if (!new) { > + err = -ENOMEM; > + goto out; > + } > + buf = new; > + new = map->ops->map_lookup_elem(map, &key); > + if (!new) { > + err = -ENOENT; > + goto out; > + } > + memcpy(buf + buf_sz, new, map->value_size); > + buf_sz += map->value_size; > + } > + > + err = verify_pkcs7_signature(buf, buf_sz, sig, sig_len, > + VERIFY_USE_SECONDARY_KEYRING, > + VERIFYING_EBPF_SIGNATURE, > + NULL, NULL); > +out: > + kfree(buf); > + return err; > +} > + > +static int hornet_check_binary(struct bpf_prog *prog, union bpf_attr *attr, > + struct hornet_maps *maps) > +{ > + struct file *file = get_task_exe_file(current); We should handle get_task_exe_file() returning NULL. I don't think it is likely to happen when passing `current` but kernel_read_file() doesn't protect against it and we'll have a NULL pointer dereference when it calls file_inode(NULL). > + const unsigned long markerlen = sizeof(EBPF_SIG_STRING) - 1; > + void *buf = NULL; > + size_t sz = 0, sig_len, prog_len, buf_sz; > + int err = 0; > + struct module_signature sig; > + > + buf_sz = kernel_read_file(file, 0, &buf, INT_MAX, &sz, READING_EBPF); We are leaking buf in this function. kernel_read_file() allocates the memory for us but we never kfree(buf). > + fput(file); > + if (!buf_sz) > + return -1; > + > + prog_len = buf_sz; > + > + if (prog_len > markerlen && > + memcmp(buf + prog_len - markerlen, EBPF_SIG_STRING, markerlen) == 0) > + prog_len -= markerlen; Why is the marker optional? Looking at module_sig_check(), which verifies the signature on kernel modules, I see that it refuses to proceed if the marker is not found. Should we do the same and refuse to operate on any unexpected input? > + > + memcpy(&sig, buf + (prog_len - sizeof(sig)), sizeof(sig)); We should make sure that prog_len is larger than sizeof(sig) prior to this memcpy(). It is probably not a real issue in practice but it would be good to ensure that we can't be tricked to copy and operate on any bytes proceeding buf. Tyler > + sig_len = be32_to_cpu(sig.sig_len); > + prog_len -= sig_len + sizeof(sig); > + > + err = mod_check_sig(&sig, prog->len * sizeof(struct bpf_insn), "ebpf"); > + if (err) > + return err; > + return hornet_verify_lskel(prog, maps, buf + prog_len, sig_len); > +}
On Apr 4, 2025 Blaise Boscaccy <bboscaccy@linux.microsoft.com> wrote: > > This adds the Hornet Linux Security Module which provides signature > verification of eBPF programs. This allows users to continue to > maintain an invariant that all code running inside of the kernel has > been signed. > > The primary target for signature verification is light-skeleton based > eBPF programs which was introduced here: > https://lore.kernel.org/bpf/20220209054315.73833-1-alexei.starovoitov@gmail.com/ > > eBPF programs, before loading, undergo a complex set of operations > which transform pseudo-values within the immediate operands of > instructions into concrete values based on the running > system. Typically, this is done by libbpf in > userspace. Light-skeletons were introduced in order to support > preloading of bpf programs and user-mode-drivers by removing the > dependency on libbpf and userspace-based operations. > > Userpace modifications, which may change every time a program gets > loaded or runs on a slightly different kernel, break known signature > verification algorithms. A method is needed for passing unadulterated > binary buffers into the kernel in-order to use existing signature > verification algorithms. Light-skeleton loaders with their support of > only in-kernel relocations fit that constraint. > > Hornet employs a signature verification scheme similar to that of > kernel modules. A signature is appended to the end of an > executable file. During an invocation of the BPF_PROG_LOAD subcommand, > a signature is extracted from the current task's executable file. That > signature is used to verify the integrity of the bpf instructions and > maps which were passed into the kernel. Additionally, Hornet > implicitly trusts any programs which were loaded from inside kernel > rather than userspace, which allows BPF_PRELOAD programs along with > outputs for BPF_SYSCALL programs to run. > > The validation check consists of checking a PKCS#7 formatted signature > against a data buffer containing the raw instructions of an eBPF > program, followed by the initial values of any maps used by the > program. Maps are frozen before signature verification checking to > stop TOCTOU attacks. > > Signed-off-by: Blaise Boscaccy <bboscaccy@linux.microsoft.com> > --- > Documentation/admin-guide/LSM/Hornet.rst | 55 ++++++ > Documentation/admin-guide/LSM/index.rst | 1 + > MAINTAINERS | 9 + > crypto/asymmetric_keys/pkcs7_verify.c | 10 + > include/linux/kernel_read_file.h | 1 + > include/linux/verification.h | 1 + > include/uapi/linux/lsm.h | 1 + > security/Kconfig | 3 +- > security/Makefile | 1 + > security/hornet/Kconfig | 11 ++ > security/hornet/Makefile | 4 + > security/hornet/hornet_lsm.c | 239 +++++++++++++++++++++++ > 12 files changed, 335 insertions(+), 1 deletion(-) > create mode 100644 Documentation/admin-guide/LSM/Hornet.rst > create mode 100644 security/hornet/Kconfig > create mode 100644 security/hornet/Makefile > create mode 100644 security/hornet/hornet_lsm.c ... > diff --git a/security/hornet/hornet_lsm.c b/security/hornet/hornet_lsm.c > new file mode 100644 > index 000000000000..d9e36764f968 > --- /dev/null > +++ b/security/hornet/hornet_lsm.c ... > +/* kern_sys_bpf is declared as an EXPORT_SYMBOL in kernel/bpf/syscall.c, however no definition is > + * provided in any bpf header files. If/when this function has a proper definition provided > + * somewhere this declaration should be removed > + */ > +int kern_sys_bpf(int cmd, union bpf_attr *attr, unsigned int size); I believe the maximum generally accepted line length is now up to 100 characters, but I remain a big fan of the ol' 80 character terminal width and would encourage you to stick to that if possible. However, you're the one who is signing on for maintenence of Hornet, not me, so if you love those >80 char lines, you do you :) I also understand why you are doing the kern_sys_bpf() declaration here, but once this lands in Linus' tree I would encourage you to try moving the declaration into a kernel-wide BPF header where it really belongs. > +static int hornet_check_binary(struct bpf_prog *prog, union bpf_attr *attr, > + struct hornet_maps *maps) > +{ > + struct file *file = get_task_exe_file(current); > + const unsigned long markerlen = sizeof(EBPF_SIG_STRING) - 1; > + void *buf = NULL; > + size_t sz = 0, sig_len, prog_len, buf_sz; > + int err = 0; > + struct module_signature sig; > + > + buf_sz = kernel_read_file(file, 0, &buf, INT_MAX, &sz, READING_EBPF); > + fput(file); > + if (!buf_sz) > + return -1; I'm pretty sure I asked you about this already off-list, but I can't remember the answer so I'm going to bring this up again :) This file read makes me a bit nervous about a mismatch between the program copy operation done in the main BPF code and the copy we do here in kernel_read_file(). Is there not some way to build up the buffer with the BPF program from the attr passed into this function (e.g. attr.insns?)? If there is some clever reason why all of this isn't an issue, it might be a good idea to put a small comment above the kernel_read_file() explaining why it is both safe and good. > + prog_len = buf_sz; > + > + if (prog_len > markerlen && > + memcmp(buf + prog_len - markerlen, EBPF_SIG_STRING, markerlen) == 0) > + prog_len -= markerlen; > + > + memcpy(&sig, buf + (prog_len - sizeof(sig)), sizeof(sig)); > + sig_len = be32_to_cpu(sig.sig_len); > + prog_len -= sig_len + sizeof(sig); > + > + err = mod_check_sig(&sig, prog->len * sizeof(struct bpf_insn), "ebpf"); > + if (err) > + return err; > + return hornet_verify_lskel(prog, maps, buf + prog_len, sig_len); > +} -- paul-moore.com
On Fri, Apr 4, 2025 at 2:56 PM Blaise Boscaccy <bboscaccy@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. Above are serious layering violations. LSMs should not be looking that deep into bpf instructions. Calling into sys_bpf from LSM is plain nack. 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. 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.
Il giorno sab 12 apr 2025 alle ore 02:19 Alexei Starovoitov <alexei.starovoitov@gmail.com> ha scritto: Similar to what I proposed here? https://lore.kernel.org/bpf/20211203191844.69709-2-mcroce@linux.microsoft.com/ > 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. static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr) { @@ -2302,6 +2306,43 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr) > It probably should be two new fields in union bpf_attr > (signature and length), @@ -1346,6 +1346,8 @@ union bpf_attr { __aligned_u64 fd_array; /* array of FDs */ __aligned_u64 core_relos; __u32 core_relo_rec_size; /* sizeof(struct bpf_core_relo) */ + __aligned_u64 signature; /* instruction's signature */ + __u32 sig_len; /* signature size */ > 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. + if (err) { + pr_warn("Invalid BPF signature for '%s': %pe\n", + prog->aux->name, ERR_PTR(err)); + goto free_prog_sec; + } It's been four years since my submission and the discussion was lengthy, what was the problem with the proposed signature in bpf_attr? Regards,
TAlexei Starovoitov <alexei.starovoitov@gmail.com> writes: > On Fri, Apr 4, 2025 at 2:56 PM Blaise Boscaccy > <bboscaccy@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.
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.
Tyler Hicks <code@tyhicks.com> writes: > On 2025-04-04 14:54:50, Blaise Boscaccy wrote: >> +static int hornet_verify_lskel(struct bpf_prog *prog, struct hornet_maps *maps, >> + void *sig, size_t sig_len) >> +{ >> + int fd; >> + u32 i; >> + void *buf; >> + void *new; >> + size_t buf_sz; >> + struct bpf_map *map; >> + int err = 0; >> + int key = 0; >> + union bpf_attr attr = {0}; >> + >> + buf = kmalloc_array(prog->len, sizeof(struct bpf_insn), GFP_KERNEL); >> + if (!buf) >> + return -ENOMEM; >> + buf_sz = prog->len * sizeof(struct bpf_insn); >> + memcpy(buf, prog->insnsi, buf_sz); >> + >> + for (i = 0; i < maps->used_map_cnt; i++) { >> + err = copy_from_bpfptr_offset(&fd, maps->fd_array, >> + maps->used_idx[i] * sizeof(fd), >> + sizeof(fd)); >> + if (err < 0) >> + continue; >> + if (fd < 1) >> + continue; >> + >> + map = bpf_map_get(fd); > > I'm not very familiar with BPF map lifetimes but I'd assume we need to have a > corresponding bpf_map_put(map) before returning. > >> + if (IS_ERR(map)) >> + continue; >> + >> + /* don't allow userspace to change map data used for signature verification */ >> + if (!map->frozen) { >> + attr.map_fd = fd; >> + err = kern_sys_bpf(BPF_MAP_FREEZE, &attr, sizeof(attr)); >> + if (err < 0) >> + goto out; >> + } >> + >> + new = krealloc(buf, buf_sz + map->value_size, GFP_KERNEL); >> + if (!new) { >> + err = -ENOMEM; >> + goto out; >> + } >> + buf = new; >> + new = map->ops->map_lookup_elem(map, &key); >> + if (!new) { >> + err = -ENOENT; >> + goto out; >> + } >> + memcpy(buf + buf_sz, new, map->value_size); >> + buf_sz += map->value_size; >> + } >> + >> + err = verify_pkcs7_signature(buf, buf_sz, sig, sig_len, >> + VERIFY_USE_SECONDARY_KEYRING, >> + VERIFYING_EBPF_SIGNATURE, >> + NULL, NULL); >> +out: >> + kfree(buf); >> + return err; >> +} >> + >> +static int hornet_check_binary(struct bpf_prog *prog, union bpf_attr *attr, >> + struct hornet_maps *maps) >> +{ >> + struct file *file = get_task_exe_file(current); > > We should handle get_task_exe_file() returning NULL. I don't think it is likely > to happen when passing `current` but kernel_read_file() doesn't protect against > it and we'll have a NULL pointer dereference when it calls file_inode(NULL). > >> + const unsigned long markerlen = sizeof(EBPF_SIG_STRING) - 1; >> + void *buf = NULL; >> + size_t sz = 0, sig_len, prog_len, buf_sz; >> + int err = 0; >> + struct module_signature sig; >> + >> + buf_sz = kernel_read_file(file, 0, &buf, INT_MAX, &sz, READING_EBPF); > > We are leaking buf in this function. kernel_read_file() allocates the memory > for us but we never kfree(buf). > >> + fput(file); >> + if (!buf_sz) >> + return -1; >> + >> + prog_len = buf_sz; >> + >> + if (prog_len > markerlen && >> + memcmp(buf + prog_len - markerlen, EBPF_SIG_STRING, markerlen) == 0) >> + prog_len -= markerlen; > > Why is the marker optional? Looking at module_sig_check(), which verifies the > signature on kernel modules, I see that it refuses to proceed if the marker is > not found. Should we do the same and refuse to operate on any unexpected input? > Looking at this again, there doesn't seem to be a good reason to have an optional marker. I'll get that fixed in v3 along with the rest of these suggestions. >> + >> + memcpy(&sig, buf + (prog_len - sizeof(sig)), sizeof(sig)); > > We should make sure that prog_len is larger than sizeof(sig) prior to this > memcpy(). It is probably not a real issue in practice but it would be good to > ensure that we can't be tricked to copy and operate on any bytes proceeding > buf. > > Tyler > >> + sig_len = be32_to_cpu(sig.sig_len); >> + prog_len -= sig_len + sizeof(sig); >> + >> + err = mod_check_sig(&sig, prog->len * sizeof(struct bpf_insn), "ebpf"); >> + if (err) >> + return err; >> + return hornet_verify_lskel(prog, maps, buf + prog_len, sig_len); >> +}
Paul Moore <paul@paul-moore.com> writes: > On Apr 4, 2025 Blaise Boscaccy <bboscaccy@linux.microsoft.com> wrote: >> >> This adds the Hornet Linux Security Module which provides signature >> verification of eBPF programs. This allows users to continue to >> maintain an invariant that all code running inside of the kernel has >> been signed. >> >> The primary target for signature verification is light-skeleton based >> eBPF programs which was introduced here: >> https://lore.kernel.org/bpf/20220209054315.73833-1-alexei.starovoitov@gmail.com/ >> >> eBPF programs, before loading, undergo a complex set of operations >> which transform pseudo-values within the immediate operands of >> instructions into concrete values based on the running >> system. Typically, this is done by libbpf in >> userspace. Light-skeletons were introduced in order to support >> preloading of bpf programs and user-mode-drivers by removing the >> dependency on libbpf and userspace-based operations. >> >> Userpace modifications, which may change every time a program gets >> loaded or runs on a slightly different kernel, break known signature >> verification algorithms. A method is needed for passing unadulterated >> binary buffers into the kernel in-order to use existing signature >> verification algorithms. Light-skeleton loaders with their support of >> only in-kernel relocations fit that constraint. >> >> Hornet employs a signature verification scheme similar to that of >> kernel modules. A signature is appended to the end of an >> executable file. During an invocation of the BPF_PROG_LOAD subcommand, >> a signature is extracted from the current task's executable file. That >> signature is used to verify the integrity of the bpf instructions and >> maps which were passed into the kernel. Additionally, Hornet >> implicitly trusts any programs which were loaded from inside kernel >> rather than userspace, which allows BPF_PRELOAD programs along with >> outputs for BPF_SYSCALL programs to run. >> >> The validation check consists of checking a PKCS#7 formatted signature >> against a data buffer containing the raw instructions of an eBPF >> program, followed by the initial values of any maps used by the >> program. Maps are frozen before signature verification checking to >> stop TOCTOU attacks. >> >> Signed-off-by: Blaise Boscaccy <bboscaccy@linux.microsoft.com> >> --- >> Documentation/admin-guide/LSM/Hornet.rst | 55 ++++++ >> Documentation/admin-guide/LSM/index.rst | 1 + >> MAINTAINERS | 9 + >> crypto/asymmetric_keys/pkcs7_verify.c | 10 + >> include/linux/kernel_read_file.h | 1 + >> include/linux/verification.h | 1 + >> include/uapi/linux/lsm.h | 1 + >> security/Kconfig | 3 +- >> security/Makefile | 1 + >> security/hornet/Kconfig | 11 ++ >> security/hornet/Makefile | 4 + >> security/hornet/hornet_lsm.c | 239 +++++++++++++++++++++++ >> 12 files changed, 335 insertions(+), 1 deletion(-) >> create mode 100644 Documentation/admin-guide/LSM/Hornet.rst >> create mode 100644 security/hornet/Kconfig >> create mode 100644 security/hornet/Makefile >> create mode 100644 security/hornet/hornet_lsm.c > > ... > >> diff --git a/security/hornet/hornet_lsm.c b/security/hornet/hornet_lsm.c >> new file mode 100644 >> index 000000000000..d9e36764f968 >> --- /dev/null >> +++ b/security/hornet/hornet_lsm.c > > ... > >> +/* kern_sys_bpf is declared as an EXPORT_SYMBOL in kernel/bpf/syscall.c, however no definition is >> + * provided in any bpf header files. If/when this function has a proper definition provided >> + * somewhere this declaration should be removed >> + */ >> +int kern_sys_bpf(int cmd, union bpf_attr *attr, unsigned int size); > > I believe the maximum generally accepted line length is now up to 100 > characters, but I remain a big fan of the ol' 80 character terminal > width and would encourage you to stick to that if possible. However, > you're the one who is signing on for maintenence of Hornet, not me, so > if you love those >80 char lines, you do you :) > > I also understand why you are doing the kern_sys_bpf() declaration here, > but once this lands in Linus' tree I would encourage you to try moving > the declaration into a kernel-wide BPF header where it really belongs. > >> +static int hornet_check_binary(struct bpf_prog *prog, union bpf_attr *attr, >> + struct hornet_maps *maps) >> +{ >> + struct file *file = get_task_exe_file(current); >> + const unsigned long markerlen = sizeof(EBPF_SIG_STRING) - 1; >> + void *buf = NULL; >> + size_t sz = 0, sig_len, prog_len, buf_sz; >> + int err = 0; >> + struct module_signature sig; >> +>> + buf_sz = kernel_read_file(file, 0, &buf, INT_MAX, &sz, READING_EBPF); >> + fput(file); >> + if (!buf_sz) >> + return -1; > > I'm pretty sure I asked you about this already off-list, but I can't > remember the answer so I'm going to bring this up again :) > > This file read makes me a bit nervous about a mismatch between the > program copy operation done in the main BPF code and the copy we do > here in kernel_read_file(). Is there not some way to build up the > buffer with the BPF program from the attr passed into this function > (e.g. attr.insns?)? > There is. That would require modifying the BPF_PROG_LOAD subcommand along with modifying the skeletobn generator to use it. I don't know if there is enough buy-in from the ebpf developers to do that currently. Tacking the signature to the end of of the light-skeleton binary allows Hornet to operate without modifying the bpf subsystem and is very similar to how module signing currently works. Modules have the advantage of having a working in-kernel loader, which makes all of this a non-issue with modules. > If there is some clever reason why all of this isn't an issue, it might > be a good idea to put a small comment above the kernel_read_file() > explaining why it is both safe and good. > Will do. I don't see this being an issue. In practice it's not much different than auth schemes that use a separate passkey. The instructions and maps are passed into the kernel during BPF_PROG_LOAD via a syscall, they aren't copied from the binary. The only part that gets copied during kernel_read_file() is the signature. If there was a mismatch between what was on-disk and what was passed in via the syscall, the signature verification would fail. As long as a signature can be found somewhere for the loader program and map, that signature is valid, and that program and map can't be modified by the user after the signature is checked, it means that someone trusted signed that blob at some point in time and only signed blobs are going to run. It shouldn't matter from a math standpoint where that signature physically lives, be it in a binary image, a buffer in a syscall or even an additional map. >> + prog_len = buf_sz; >> + >> + if (prog_len > markerlen && >> + memcmp(buf + prog_len - markerlen, EBPF_SIG_STRING, markerlen) == 0) >> + prog_len -= markerlen; >> + >> + memcpy(&sig, buf + (prog_len - sizeof(sig)), sizeof(sig)); >> + sig_len = be32_to_cpu(sig.sig_len); >> + prog_len -= sig_len + sizeof(sig); >> + >> + err = mod_check_sig(&sig, prog->len * sizeof(struct bpf_insn), "ebpf"); >> + if (err) >> + return err; >> + return hornet_verify_lskel(prog, maps, buf + prog_len, sig_len); >> +} > > -- > paul-moore.com
On Sat, Apr 12, 2025 at 6:58 AM Blaise Boscaccy <bboscaccy@linux.microsoft.com> wrote: > > TAlexei Starovoitov <alexei.starovoitov@gmail.com> writes: > > > On Fri, Apr 4, 2025 at 2:56 PM Blaise Boscaccy > > <bboscaccy@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. 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. > 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. > 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.
Alexei Starovoitov <alexei.starovoitov@gmail.com> writes: > On Sat, Apr 12, 2025 at 6:58 AM Blaise Boscaccy > <bboscaccy@linux.microsoft.com> wrote: >> >> TAlexei Starovoitov <alexei.starovoitov@gmail.com> writes: >> >> > On Fri, Apr 4, 2025 at 2:56 PM Blaise Boscaccy >> > <bboscaccy@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. > > 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.
On Mon, Apr 14, 2025 at 5:32 PM Blaise Boscaccy <bboscaccy@linux.microsoft.com> wrote: > > Alexei Starovoitov <alexei.starovoitov@gmail.com> writes: > > > On Sat, Apr 12, 2025 at 6:58 AM Blaise Boscaccy > > <bboscaccy@linux.microsoft.com> wrote: > >> > >> TAlexei Starovoitov <alexei.starovoitov@gmail.com> writes: > >> > >> > On Fri, Apr 4, 2025 at 2:56 PM Blaise Boscaccy > >> > <bboscaccy@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. > > > > 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.
Blaise Boscaccy <bboscaccy@linux.microsoft.com> writes: > Alexei Starovoitov <alexei.starovoitov@gmail.com> writes: > >> On Mon, Apr 14, 2025 at 5:32 PM Blaise Boscaccy >> <bboscaccy@linux.microsoft.com> wrote: >>> >>> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes: >>> >>> > On Sat, Apr 12, 2025 at 6:58 AM Blaise Boscaccy >>> > <bboscaccy@linux.microsoft.com> wrote: >>> >> >>> >> TAlexei Starovoitov <alexei.starovoitov@gmail.com> writes: >>> >> >>> >> > On Fri, Apr 4, 2025 at 2:56 PM Blaise Boscaccy >>> >> > <bboscaccy@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. >>> > >>> > 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? > > The eBPF dev community has spent what, 4-5 years on this, with little to > no progress. I have little faith that this is going to progress on your > end in a timely manner or at all, and frankly we (and others) needed > this yesterday. Hornet has zero impact on the bpf subsystem, yet you > seem viscerally opposed to us doing this. Why are you trying to stop us > from securing our cloud? > >> >> 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. >> In the interest of not harming downstream users that possibly rely on BPF_PRELOAD, it would be completely fine on our end to not call kern_sys_bpf since maps can easily be frozen in userspace by the loader. I'd prefer LSMs to be not mutating things if at all possible as well. If we agreed to not call that function so-as you can keep that feature for everyone, would you be ammenable to a simple patch in skel_internal.h that freezes maps? e.g diff --git a/tools/lib/bpf/skel_internal.h b/tools/lib/bpf/skel_internal.h index 4d5fa079b5d6..51e72dc23062 100644 --- a/tools/lib/bpf/skel_internal.h +++ b/tools/lib/bpf/skel_internal.h @@ -263,6 +263,17 @@ static inline int skel_map_delete_elem(int fd, const void *key) return skel_sys_bpf(BPF_MAP_DELETE_ELEM, &attr, attr_sz); } +static inline int skel_map_freeze(int fd) +{ + const size_t attr_sz = offsetofend(union bpf_attr, map_fd); + union bpf_attr attr; + + memset(&attr, 0, attr_sz); + attr.map_fd = fd; + + return skel_sys_bpf(BPF_MAP_FREEZE, &attr, attr_sz); +} + static inline int skel_map_get_fd_by_id(__u32 id) { const size_t attr_sz = offsetofend(union bpf_attr, flags); @@ -327,6 +338,13 @@ static inline int bpf_load_and_run(struct bpf_load_and_run_opts *opts) goto out; } + err = skel_map_freeze(map_fd); + if (err < 0) { + opts->errstr = "failed to freeze map"; + set_err; + goto out; + } + memset(&attr, 0, prog_load_attr_sz); attr.prog_type = BPF_PROG_TYPE_SYSCALL; attr.insns = (long) opts->insns; >>> >> 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. >> > > Since this will require an LSM no matter what, there is zero reason for > us not to proceed with Hornet. If or when you actually figure out how to > sign an lskel and upstream updated LSM hooks, I can always rework Hornet > to use that instead. > >>> >>> >> 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. > > No one is making that claim, although I do appreciate the lovely > ad-hominem attack and absolutist standpoint. It's not like we invented > code signing last week. All we are trying to do is make our cloud > ever-so-slightly more secure and share the results with the community. > > The attack vectors I'm looking at are things like CVE-2021-33200. ACLs > for attachment points do nothing to stop that whereas code signing is a > possible countermeasure. This kind of thing is probably a non-issue with > your private cloud, but it's a very real issue with publicly accessible > ones.
On Tue, Apr 15, 2025 at 8:45 AM Blaise Boscaccy <bboscaccy@linux.microsoft.com> wrote: > > The eBPF dev community has spent what, 4-5 years on this, with little to > no progress. I have little faith that this is going to progress on your > end in a timely manner or at all, and frankly we (and others) needed > this yesterday. History repeats itself. 1. the problem is hard. 2. you're only interested in addressing your own use case. There is no end-to-end design here and no attempt to think it through how it will work for others. > Hornet has zero impact on the bpf subsystem, yet you > seem viscerally opposed to us doing this. Hacking into bpf internal objects like maps is not acceptable. > Why are you trying to stop us > from securing our cloud? Keep your lsm hack out-of-tree, please. > Since this will require an LSM no matter what, there is zero reason for > us not to proceed with Hornet. If or when you actually figure out how to > sign an lskel and upstream updated LSM hooks, I can always rework Hornet > to use that instead. You can do whatever you want out-of-tree including re-exporting kern_sys_bpf. > code signing last week. All we are trying to do is make our cloud > ever-so-slightly more secure and share the results with the community. You're pushing for a custom microsoft specific hack while ignoring community feedback. > The attack vectors I'm looking at are things like CVE-2021-33200. 4 year old bug ? If your kernels are so old you have lots of other vulnerabilities.
On Tue, Apr 15, 2025 at 3:08 PM Blaise Boscaccy <bboscaccy@linux.microsoft.com> wrote: > ... would you be ammenable to a simple patch in > skel_internal.h that freezes maps? e.g I have limited network access at the moment, so it is possible I've missed it, but I think it would be helpful to get a verdict on the RFC-esque patch from Blaise below. > diff --git a/tools/lib/bpf/skel_internal.h b/tools/lib/bpf/skel_internal.h > index 4d5fa079b5d6..51e72dc23062 100644 > --- a/tools/lib/bpf/skel_internal.h > +++ b/tools/lib/bpf/skel_internal.h > @@ -263,6 +263,17 @@ static inline int skel_map_delete_elem(int fd, const void *key) > return skel_sys_bpf(BPF_MAP_DELETE_ELEM, &attr, attr_sz); > } > > +static inline int skel_map_freeze(int fd) > +{ > + const size_t attr_sz = offsetofend(union bpf_attr, map_fd); > + union bpf_attr attr; > + > + memset(&attr, 0, attr_sz); > + attr.map_fd = fd; > + > + return skel_sys_bpf(BPF_MAP_FREEZE, &attr, attr_sz); > +} > + > static inline int skel_map_get_fd_by_id(__u32 id) > { > const size_t attr_sz = offsetofend(union bpf_attr, flags); > @@ -327,6 +338,13 @@ static inline int bpf_load_and_run(struct bpf_load_and_run_opts *opts) > goto out; > } > > + err = skel_map_freeze(map_fd); > + if (err < 0) { > + opts->errstr = "failed to freeze map"; > + set_err; > + goto out; > + } > + > memset(&attr, 0, prog_load_attr_sz); > attr.prog_type = BPF_PROG_TYPE_SYSCALL; > attr.insns = (long) opts->insns; >
On Fri, 2025-04-04 at 14:54 -0700, Blaise Boscaccy wrote: [...] > diff --git a/include/linux/kernel_read_file.h > b/include/linux/kernel_read_file.h > index 90451e2e12bd..7ed9337be542 100644 > --- a/include/linux/kernel_read_file.h > +++ b/include/linux/kernel_read_file.h > @@ -14,6 +14,7 @@ > id(KEXEC_INITRAMFS, kexec-initramfs) \ > id(POLICY, security-policy) \ > id(X509_CERTIFICATE, x509-certificate) \ > + id(EBPF, ebpf) \ This causes a BUILD_BUG_ON for me in security/selinux/hooks.c with CONFIG_SECURITY_SELINUX=y because READING_MAX_ID and LOADING_MAX_ID become 8. Below is what I had to do to get the compile to work. Regards, James --- diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index e7a7dcab81db..9a7ed0b4b08d 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4133,7 +4133,7 @@ static int selinux_kernel_read_file(struct file *file, { int rc = 0; - BUILD_BUG_ON_MSG(READING_MAX_ID > 7, + BUILD_BUG_ON_MSG(READING_MAX_ID > 8, "New kernel_read_file_id introduced; update SELinux!"); switch (id) { @@ -4158,6 +4158,10 @@ static int selinux_kernel_read_file(struct file *file, rc = selinux_kernel_load_from_file(file, SYSTEM__X509_CERTIFICATE_LOAD); break; + case READING_EBPF: + rc = selinux_kernel_load_from_file(file, + SYSTEM__EBPF_LOAD); + break; default: break; } @@ -4169,7 +4173,7 @@ static int selinux_kernel_load_data(enum kernel_load_data_id id, bool contents) { int rc = 0; - BUILD_BUG_ON_MSG(LOADING_MAX_ID > 7, + BUILD_BUG_ON_MSG(LOADING_MAX_ID > 8, "New kernel_load_data_id introduced; update SELinux!"); switch (id) { @@ -4195,6 +4199,10 @@ static int selinux_kernel_load_data(enum kernel_load_data_id id, bool contents) rc = selinux_kernel_load_from_file(NULL, SYSTEM__X509_CERTIFICATE_LOAD); break; + case LOADING_EBPF: + rc = selinux_kernel_load_from_file(NULL, + SYSTEM__EBPF_LOAD); + break; default: break; } diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h index 04a9b480885e..671db23451df 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -65,7 +65,7 @@ const struct security_class_mapping secclass_map[] = { { "ipc_info", "syslog_read", "syslog_mod", "syslog_console", "module_request", "module_load", "firmware_load", "kexec_image_load", "kexec_initramfs_load", "policy_load", - "x509_certificate_load", NULL } }, + "x509_certificate_load", "ebpf_load", NULL } }, { "capability", { COMMON_CAP_PERMS, NULL } }, { "filesystem", { "mount", "remount", "unmount", "getattr", "relabelfrom",
On Mon, 2025-04-21 at 14:52 -0400, Paul Moore wrote: > On Sat, Apr 19, 2025 at 2:43 PM James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > On Fri, 2025-04-04 at 14:54 -0700, Blaise Boscaccy wrote: > > [...] > > > diff --git a/include/linux/kernel_read_file.h > > > b/include/linux/kernel_read_file.h > > > index 90451e2e12bd..7ed9337be542 100644 > > > --- a/include/linux/kernel_read_file.h > > > +++ b/include/linux/kernel_read_file.h > > > @@ -14,6 +14,7 @@ > > > id(KEXEC_INITRAMFS, kexec-initramfs) \ > > > id(POLICY, security-policy) \ > > > id(X509_CERTIFICATE, x509-certificate) \ > > > + id(EBPF, ebpf) \ > > > > This causes a BUILD_BUG_ON for me in security/selinux/hooks.c with > > CONFIG_SECURITY_SELINUX=y because READING_MAX_ID and LOADING_MAX_ID > > become 8. > > > > Below is what I had to do to get the compile to work. > > That code was updated during the v6.15 merge window, depending on > what kernel sources Blaise is using for development it's possible he > didn't bump into this even if he was building with SELinux enabled. Sorry I should have said I pulled the patches into 6.15-rc2 to play with them (hence I did pick up everything in the recent merge window). Regards, James
On Wed, Apr 16, 2025 at 10:31 AM Blaise Boscaccy <bboscaccy@linux.microsoft.com> wrote: > > > Hacking into bpf internal objects like maps is not acceptable. > > We've heard your concerns about kern_sys_bpf and we agree that the LSM > should not be calling it. The proposal in this email should meet both of > our needs > https://lore.kernel.org/bpf/874iypjl8t.fsf@microsoft.com/ kern_sys_bpf was one example of a layering violation. Calling bpf_map_get() and map->ops->map_lookup_elem() from a module is not ok either. lskel doing skel_map_freeze is not solving the issue. It is still broken from TOCTOU pov. freeze only makes a map readonly to user space. Any program can still read/write it. That's why freeze wasn't done back then when lskel was introduced. There is still work to do to make signing practical. One needs to think of libbpf equivalent loaders in golang and rust. The solution has to work for them too. In that sense bpf signing is not analogous to kernel module signing. Programs are not distributed as elf files. elf is an intermediate step in a build process. bpftool takes elf and generates skel or lskel and user space does #include <skel.h> to access maps and global variables directly. See how systemd does it. bpf progs are part of various skel.h-s in there. systemd is also using an old style bpf progs written in bpf assembly. We need to figure out how to make them signed too. The signing problem is big and difficult. It will take time to figure out all these challenges. Introduction of lskel back in 2021 was the first step towards signing (as the commit log clearly states). lskel approach is likely a solution for a large class of bpf users, but not for all. It won't work for bpftrace and bcc. lskel loading is also opaque. The verifier errors are not propagated from the loader prog back to the user. To load normal skeleton the user space can do: LIBBPF_OPTS(bpf_object_open_opts, opts); opts.kernel_log_buf = my_verifier_log_buf; myskel__open_opts(&opts); There is no __open_opts() equivalent for lskel. It needs to be fixed before we can recommend lksel as a solution to signing.
On Mon, Apr 21, 2025 at 4:13 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > On Wed, Apr 16, 2025 at 10:31 AM Blaise Boscaccy > <bboscaccy@linux.microsoft.com> wrote: > > > > > Hacking into bpf internal objects like maps is not acceptable. > > > > We've heard your concerns about kern_sys_bpf and we agree that the LSM > > should not be calling it. The proposal in this email should meet both of > > our needs > > https://lore.kernel.org/bpf/874iypjl8t.fsf@microsoft.com/ ... > Calling bpf_map_get() and > map->ops->map_lookup_elem() from a module is not ok either. A quick look uncovers code living under net/ which calls into these APIs. > lskel doing skel_map_freeze is not solving the issue. > It is still broken from TOCTOU pov. > freeze only makes a map readonly to user space. > Any program can still read/write it. When you say "any program" you are referring to any BPF program loaded into the kernel, correct? At least that is my understanding of "freezing" a BPF map, while userspace is may be unable to modify the map's contents, it is still possible for a BPF program to modify it. If I'm mistaken, I would appreciate a pointer to a correct description of map freezing. Assuming the above is correct, that a malicious bit of code running in kernel context could cause mischief, isn't a new concern, and in fact it is one of the reasons why Hornet is valuable. Hornet allows admins/users to have some assurance that the BPF programs they load into their system come from a trusted source (trusted not to intentionally do Bad Things in the kernel) and haven't been modified to do Bad Things (like modify lskel maps). > One needs to think of libbpf equivalent loaders in golang and rust. ... > systemd is also using an old style bpf progs written in bpf assembly. I've briefly talked with Blaise about the systemd issue in particular, and I believe there are some relatively easy ways to work around the ELF issue in the current version of Hornet. I know Blaise is tied up for the next couple of days on another fire, but I'm sure the next revision will have a solution for this. > Introduction of lskel back in 2021 was the first step towards signing > (as the commit log clearly states). > lskel approach is likely a solution for a large class of bpf users, > but not for all. It won't work for bpftrace and bcc. As most everyone on the To/CC line already knows, Linux kernel development is often iterative. Not only is it easier for the kernel devs to develop and review incremental additions to functionality, it also enables a feedback loop where users can help drive the direction of the functionality as it is built. I view Hornet as an iterative improvement, building on the lskel concept, that helps users towards their goal of load time verification of code running inside the kernel. Hornet, as currently described, may not be the solution for everything, but it can be the solution for something that users are desperately struggling with today and as far as I'm concerned, that is a good thing worth supporting.
On Mon, Apr 21, 2025 at 3:04 PM Paul Moore <paul@paul-moore.com> wrote: > > On Mon, Apr 21, 2025 at 4:13 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > On Wed, Apr 16, 2025 at 10:31 AM Blaise Boscaccy > > <bboscaccy@linux.microsoft.com> wrote: > > > > > > > Hacking into bpf internal objects like maps is not acceptable. > > > > > > We've heard your concerns about kern_sys_bpf and we agree that the LSM > > > should not be calling it. The proposal in this email should meet both of > > > our needs > > > https://lore.kernel.org/bpf/874iypjl8t.fsf@microsoft.com/ > > ... > > > Calling bpf_map_get() and > > map->ops->map_lookup_elem() from a module is not ok either. > > A quick look uncovers code living under net/ which calls into these APIs. and your point is ? Again, Nack to hacking into bpf internals from LSM, module or kernel subsystem.
On Wed, Apr 23, 2025 at 10:12 AM James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > On Mon, 2025-04-21 at 13:12 -0700, Alexei Starovoitov wrote: > [...] > > Calling bpf_map_get() and > > map->ops->map_lookup_elem() from a module is not ok either. > > I don't understand this objection. The program just got passed in to > bpf_prog_load() as a set of attributes which, for a light skeleton, > directly contain the code as a blob and have the various BTF > relocations as a blob in a single element array map. I think everyone > agrees that the integrity of the program would be compromised by > modifications to the relocations, so the security_bpf_prog_load() hook > can't make an integrity determination without examining both. If the > hook can't use the bpf_maps.. APIs directly is there some other API it > should be using to get the relocations, or are you saying that the > security_bpf_prog_load() hook isn't fit for purpose and it should be > called after the bpf core has loaded the relocations so they can be > provided to the hook as an argument? > > The above, by the way, is independent of signing, because it applies to > any determination that might be made in the security_bpf_prog_load() > hook regardless of purpose. I've also been worrying that some of the unspoken motivation behind the objection is simply that Hornet is not BPF. If/when we get to a point where Hornet is sent up to Linus and Alexei's objection to the Hornet LSM inspecting BPF maps stands, it seems as though *any* LSM, including BPF LSMs, would need to be prevented from accessing BPF maps. I'm fairly certain no one wants to see it come to that.
On Wed, Apr 23, 2025 at 7:12 AM James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > > On Mon, 2025-04-21 at 13:12 -0700, Alexei Starovoitov wrote: > [...] > > Calling bpf_map_get() and > > map->ops->map_lookup_elem() from a module is not ok either. > > I don't understand this objection. Consider an LSM that hooks into security_bprm_*(bprm), parses something in linux_binprm, then struct file *file = fd_file(fdget(some_random_file_descriptor_in_current)); file->f_op->read(..); Would VFS maintainers approve such usage ? More so, your LSM does file = get_task_exe_file(current); kernel_read_file(file, ...); This is even worse. You've corrupted the ELF binary with extra garbage at the end. objdump/elfutils will choke on it and you're lucky that binfmt_elf still loads it. The whole approach is a non-starter. > The program just got passed in to > bpf_prog_load() as a set of attributes which, for a light skeleton, > directly contain the code as a blob and have the various BTF > relocations as a blob in a single element array map. I think everyone > agrees that the integrity of the program would be compromised by > modifications to the relocations, so the security_bpf_prog_load() hook > can't make an integrity determination without examining both. If the > hook can't use the bpf_maps.. APIs directly is there some other API it > should be using to get the relocations, or are you saying that the > security_bpf_prog_load() hook isn't fit for purpose and it should be > called after the bpf core has loaded the relocations so they can be > provided to the hook as an argument? No. As I said twice already the only place to verify program signature is a bpf subsystem itself. Hacking into bpf internals from LSM, BPF-LSM program, or any other kernel subsystem is a no go. > The above, by the way, is independent of signing, because it applies to > any determination that might be made in the security_bpf_prog_load() > hook regardless of purpose. security_bpf_prog_load() should not access bpf internals. That LSM hook sees the following: security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr, struct bpf_token *token, bool kernel); LSM can look into uapi things there. Like prog->sleepable, prog->tag, prog->aux->name, but things like prog->aux->jit_data or prog->aux->used_maps are not ok to access. If in doubt, ask on the mailing list.
On Thu, 2025-04-24 at 16:41 -0700, Alexei Starovoitov wrote: > On Wed, Apr 23, 2025 at 7:12 AM James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > On Mon, 2025-04-21 at 13:12 -0700, Alexei Starovoitov wrote: > > [...] > > > Calling bpf_map_get() and > > > map->ops->map_lookup_elem() from a module is not ok either. > > > > I don't understand this objection. > > Consider an LSM that hooks into security_bprm_*(bprm), > parses something in linux_binprm, then > struct file *file = > fd_file(fdget(some_random_file_descriptor_in_current)); > file->f_op->read(..); > > Would VFS maintainers approve such usage ? This is a bit off topic from the request for clarification but: It's somewhat standard operating procedure for LSMs. Some do make decisions entirely within the data provided by the hook, but some need to take external readings, like selinux or IMA consulting the policy in the xattr or apparmor the one in the tree etc. Incidentally, none of them directly does a file->f_op->read(); they all use the kernel_read_file() API which is exported from the vfs for that purpose. > More so, your LSM does > file = get_task_exe_file(current); > kernel_read_file(file, ...); > > This is even worse. > You've corrupted the ELF binary with extra garbage at the end. > objdump/elfutils will choke on it and you're lucky that binfmt_elf > still loads it. > The whole approach is a non-starter. It's the same approach we use to create kernel modules: ELF with an appended signature. If you recall the kernel summit discussions about it, the reason that was chosen for modules is because it's easy and the ELF processor simply ignores any data in the file that's not described by the header (which means the ELF tools you refer to above are fine with this if you actually try them). But it you really want the signature to be part of the ELF, then the patch set can do what David Howells first suggested for modules: it can simply put the appended signature into an unloaded ELF section. > > The program just got passed in to bpf_prog_load() as a set of > > attributes which, for a light skeleton, directly contain the code > > as a blob and have the various BTF relocations as a blob in a > > single element array map. I think everyone agrees that the > > integrity of the program would be compromised by modifications to > > the relocations, so the security_bpf_prog_load() hook can't make an > > integrity determination without examining both. If the hook can't > > use the bpf_maps.. APIs directly is there some other API it should > > be using to get the relocations, or are you saying that the > > security_bpf_prog_load() hook isn't fit for purpose and it should > > be called after the bpf core has loaded the relocations so they can > > be provided to the hook as an argument? > > No. As I said twice already the only place to verify program > signature is a bpf subsystem itself. The above argument is actually independent of signing. However, although we have plenty of subsystems that verify their own signatures, it's perfectly valid for a LSM to do it as well: IMA is one of the oldest LSMs and it's been verifying signatures over binaries and text files since it was first created. > Hacking into bpf internals from LSM, BPF-LSM program, > or any other kernel subsystem is a no go. All LSMs depend to some extent on the internals of the subsystem where the hook is ... the very structures passed into them are often internal to that subsystem. The problem you didn't address was that some of the information necessary to determine the integrity properties in the bpf hook is in a map file descriptor. Since the map merely wraps a single blob of data, that could easily be passed in to the hook instead of having the LSM extract it from the map. How the hook gets the data is an internal implementation detail of the kernel that can be updated later. > > The above, by the way, is independent of signing, because it > > applies to any determination that might be made in the > > security_bpf_prog_load() hook regardless of purpose. > > security_bpf_prog_load() should not access bpf internals. > That LSM hook sees the following: > security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr, > struct bpf_token *token, bool kernel); > > LSM can look into uapi things there. Is that the misunderstanding? That's not how LSMs work: they are not bound by only the UAPI, they are in kernel and have full access to the kernel API so they can introspect stuff and make proper determinations. > Like prog->sleepable, prog->tag, prog->aux->name, > but things like prog->aux->jit_data or prog->aux->used_maps > are not ok to access. > If in doubt, ask on the mailing list. I am aren't I? At least the bpf is one of the lists cc'd on this. Regards, James
James Bottomley <James.Bottomley@HansenPartnership.com> writes: > On Thu, 2025-04-24 at 16:41 -0700, Alexei Starovoitov wrote: >> On Wed, Apr 23, 2025 at 7:12 AM James Bottomley >> <James.Bottomley@hansenpartnership.com> wrote: >> > On Mon, 2025-04-21 at 13:12 -0700, Alexei Starovoitov wrote: >> > [...] >> > > Calling bpf_map_get() and >> > > map->ops->map_lookup_elem() from a module is not ok either. >> > >> > I don't understand this objection. >> >> Consider an LSM that hooks into security_bprm_*(bprm), >> parses something in linux_binprm, then >> struct file *file = >> fd_file(fdget(some_random_file_descriptor_in_current)); >> file->f_op->read(..); >> >> Would VFS maintainers approve such usage ? > > This is a bit off topic from the request for clarification but: > > It's somewhat standard operating procedure for LSMs. Some do make > decisions entirely within the data provided by the hook, but some need > to take external readings, like selinux or IMA consulting the policy in > the xattr or apparmor the one in the tree etc. > > Incidentally, none of them directly does a file->f_op->read(); they all > use the kernel_read_file() API which is exported from the vfs for that > purpose. > >> More so, your LSM does >> file = get_task_exe_file(current); >> kernel_read_file(file, ...); >> >> This is even worse. >> You've corrupted the ELF binary with extra garbage at the end. >> objdump/elfutils will choke on it and you're lucky that binfmt_elf >> still loads it. >> The whole approach is a non-starter. > > It's the same approach we use to create kernel modules: ELF with an > appended signature. If you recall the kernel summit discussions about > it, the reason that was chosen for modules is because it's easy and the > ELF processor simply ignores any data in the file that's not described > by the header (which means the ELF tools you refer to above are fine > with this if you actually try them). > > But it you really want the signature to be part of the ELF, then the > patch set can do what David Howells first suggested for modules: it can > simply put the appended signature into an unloaded ELF section. > >> > The program just got passed in to bpf_prog_load() as a set of >> > attributes which, for a light skeleton, directly contain the code >> > as a blob and have the various BTF relocations as a blob in a >> > single element array map. I think everyone agrees that the >> > integrity of the program would be compromised by modifications to >> > the relocations, so the security_bpf_prog_load() hook can't make an >> > integrity determination without examining both. If the hook can't >> > use the bpf_maps.. APIs directly is there some other API it should >> > be using to get the relocations, or are you saying that the >> > security_bpf_prog_load() hook isn't fit for purpose and it should >> > be called after the bpf core has loaded the relocations so they can >> > be provided to the hook as an argument? >> >> No. As I said twice already the only place to verify program >> signature is a bpf subsystem itself. > > The above argument is actually independent of signing. However, > although we have plenty of subsystems that verify their own signatures, > it's perfectly valid for a LSM to do it as well: IMA is one of the > oldest LSMs and it's been verifying signatures over binaries and text > files since it was first created. > >> Hacking into bpf internals from LSM, BPF-LSM program, >> or any other kernel subsystem is a no go. > > All LSMs depend to some extent on the internals of the subsystem where > the hook is ... the very structures passed into them are often internal > to that subsystem. The problem you didn't address was that some of the > information necessary to determine the integrity properties in the bpf > hook is in a map file descriptor. Since the map merely wraps a single > blob of data, that could easily be passed in to the hook instead of > having the LSM extract it from the map. How the hook gets the data is > an internal implementation detail of the kernel that can be updated > later. > >> > The above, by the way, is independent of signing, because it >> > applies to any determination that might be made in the >> > security_bpf_prog_load() hook regardless of purpose. >> >> security_bpf_prog_load() should not access bpf internals. >> That LSM hook sees the following: >> security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr, >> struct bpf_token *token, bool kernel); >> >> LSM can look into uapi things there. > > Is that the misunderstanding? That's not how LSMs work: they are not > bound by only the UAPI, they are in kernel and have full access to the > kernel API so they can introspect stuff and make proper determinations. > >> Like prog->sleepable, prog->tag, prog->aux->name, >> but things like prog->aux->jit_data or prog->aux->used_maps >> are not ok to access. >> If in doubt, ask on the mailing list. > > I am aren't I? At least the bpf is one of the lists cc'd on this. > > Regards, > > James I think we may be in the weeds here a bit and starting to get a little off-topic. Let's try to back up some and take a different tack. We are going to rework this effort into a set of patches that target the bpf subsystem and it's tooling directly, performing optional signature verification of the inputs to bpf_prog_load, using signature data passed in via bpf_attr, which should enough provide metadata so that it can be consumed by interested parties to enforce policy decisions around code signing and data integrity. -blaise
diff --git a/Documentation/admin-guide/LSM/Hornet.rst b/Documentation/admin-guide/LSM/Hornet.rst new file mode 100644 index 000000000000..e0d2926c4041 --- /dev/null +++ b/Documentation/admin-guide/LSM/Hornet.rst @@ -0,0 +1,55 @@ +.. SPDX-License-Identifier: GPL-2.0 + +====== +Hornet +====== + +Hornet is a Linux Security Module that provides signature verification +for eBPF programs. This is selectable at build-time with +``CONFIG_SECURITY_HORNET``. + +Overview +======== + +Hornet provides signature verification for eBPF programs by utilizing +the existing PKCS#7 infrastructure that's used for module signature +verification. Hornet works by creating a buffer containing the eBPF +program instructions along with its associated maps and checking a +signature against that buffer. The signature is appended to the end of +the lskel executable file and is extracted at runtime via +get_task_exe_file. Hornet works by hooking into the +security_bpf_prog_load hook. Load invocations that originate from the +kernel (bpf preload, results of bpf_syscall programs, etc.) are +allowed to run unconditionally. Calls that originate from userspace +require signature verification. If signature verification fails, the +program will fail to load. Maps are frozen before the signature check +to prevent TOCTOU exploits where a sufficiently privileged user could +rewrite map data between the calls to BPF_PROG_LOAD and BPF_PROG_RUN. + +Instruction/Map Ordering +======================== + +Hornet supports both sparse-array based maps via map discovery along +with the newly added fd_array_cnt API for continuous map arrays. The +buffer used for signature verification is assumed to be the +instructions followed by all maps used, ordered by their index in +fd_array. + +Tooling +======= + +Some tooling is provided to aid with the development of signed eBPF +lskels. + +extract-skel.sh +--------------- + +This shell script extracts the instructions and map data used by the +light skeleton from the autogenerated header file created by bpftool. + +sign-ebpf +--------- + +sign-ebpf works similarly to the sign-file script with one key +difference: it takes a separate input binary used for signature +verification and will append the signature to a different output file. diff --git a/Documentation/admin-guide/LSM/index.rst b/Documentation/admin-guide/LSM/index.rst index ce63be6d64ad..0ecb5016ce1f 100644 --- a/Documentation/admin-guide/LSM/index.rst +++ b/Documentation/admin-guide/LSM/index.rst @@ -48,3 +48,4 @@ subdirectories. Yama SafeSetID ipe + Hornet diff --git a/MAINTAINERS b/MAINTAINERS index 3864d473f52f..a2355059cdf4 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -10588,6 +10588,15 @@ S: Maintained F: Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml F: drivers/iio/pressure/mprls0025pa* +HORNET SECURITY MODULE +M: Blaise Boscaccy <bboscaccy@linux.microsoft.com> +L: linux-security-module@vger.kernel.org +S: Supported +T: git https://github.com/blaiseboscaccy/hornet.git +F: Documentation/admin-guide/LSM/Hornet.rst +F: scripts/hornet/ +F: security/hornet/ + HP BIOSCFG DRIVER M: Jorge Lopez <jorge.lopez2@hp.com> L: platform-driver-x86@vger.kernel.org diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c index f0d4ff3c20a8..1a5fbb361218 100644 --- a/crypto/asymmetric_keys/pkcs7_verify.c +++ b/crypto/asymmetric_keys/pkcs7_verify.c @@ -428,6 +428,16 @@ int pkcs7_verify(struct pkcs7_message *pkcs7, } /* Authattr presence checked in parser */ break; + case VERIFYING_EBPF_SIGNATURE: + if (pkcs7->data_type != OID_data) { + pr_warn("Invalid ebpf sig (not pkcs7-data)\n"); + return -EKEYREJECTED; + } + if (pkcs7->have_authattrs) { + pr_warn("Invalid ebpf sig (has authattrs)\n"); + return -EKEYREJECTED; + } + break; case VERIFYING_UNSPECIFIED_SIGNATURE: if (pkcs7->data_type != OID_data) { pr_warn("Invalid unspecified sig (not pkcs7-data)\n"); diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h index 90451e2e12bd..7ed9337be542 100644 --- a/include/linux/kernel_read_file.h +++ b/include/linux/kernel_read_file.h @@ -14,6 +14,7 @@ id(KEXEC_INITRAMFS, kexec-initramfs) \ id(POLICY, security-policy) \ id(X509_CERTIFICATE, x509-certificate) \ + id(EBPF, ebpf) \ id(MAX_ID, ) #define __fid_enumify(ENUM, dummy) READING_ ## ENUM, diff --git a/include/linux/verification.h b/include/linux/verification.h index 4f3022d081c3..812be8ad5f74 100644 --- a/include/linux/verification.h +++ b/include/linux/verification.h @@ -35,6 +35,7 @@ enum key_being_used_for { VERIFYING_KEXEC_PE_SIGNATURE, VERIFYING_KEY_SIGNATURE, VERIFYING_KEY_SELF_SIGNATURE, + VERIFYING_EBPF_SIGNATURE, VERIFYING_UNSPECIFIED_SIGNATURE, NR__KEY_BEING_USED_FOR }; diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h index 938593dfd5da..2ff9bcdd551e 100644 --- a/include/uapi/linux/lsm.h +++ b/include/uapi/linux/lsm.h @@ -65,6 +65,7 @@ struct lsm_ctx { #define LSM_ID_IMA 111 #define LSM_ID_EVM 112 #define LSM_ID_IPE 113 +#define LSM_ID_HORNET 114 /* * LSM_ATTR_XXX definitions identify different LSM attributes diff --git a/security/Kconfig b/security/Kconfig index f10dbf15c294..0030f0224c7a 100644 --- a/security/Kconfig +++ b/security/Kconfig @@ -230,6 +230,7 @@ source "security/safesetid/Kconfig" source "security/lockdown/Kconfig" source "security/landlock/Kconfig" source "security/ipe/Kconfig" +source "security/hornet/Kconfig" source "security/integrity/Kconfig" @@ -273,7 +274,7 @@ config LSM default "landlock,lockdown,yama,loadpin,safesetid,apparmor,selinux,smack,tomoyo,ipe,bpf" if DEFAULT_SECURITY_APPARMOR default "landlock,lockdown,yama,loadpin,safesetid,tomoyo,ipe,bpf" if DEFAULT_SECURITY_TOMOYO default "landlock,lockdown,yama,loadpin,safesetid,ipe,bpf" if DEFAULT_SECURITY_DAC - default "landlock,lockdown,yama,loadpin,safesetid,selinux,smack,tomoyo,apparmor,ipe,bpf" + default "landlock,lockdown,yama,loadpin,safesetid,selinux,smack,tomoyo,apparmor,ipe,hornet,bpf" help A comma-separated list of LSMs, in initialization order. Any LSMs left off this list, except for those with order diff --git a/security/Makefile b/security/Makefile index 22ff4c8bd8ce..e24bccd951f8 100644 --- a/security/Makefile +++ b/security/Makefile @@ -26,6 +26,7 @@ obj-$(CONFIG_CGROUPS) += device_cgroup.o obj-$(CONFIG_BPF_LSM) += bpf/ obj-$(CONFIG_SECURITY_LANDLOCK) += landlock/ obj-$(CONFIG_SECURITY_IPE) += ipe/ +obj-$(CONFIG_SECURITY_HORNET) += hornet/ # Object integrity file lists obj-$(CONFIG_INTEGRITY) += integrity/ diff --git a/security/hornet/Kconfig b/security/hornet/Kconfig new file mode 100644 index 000000000000..19406aa237ac --- /dev/null +++ b/security/hornet/Kconfig @@ -0,0 +1,11 @@ +# SPDX-License-Identifier: GPL-2.0-only +config SECURITY_HORNET + bool "Hornet support" + depends on SECURITY + default n + help + This selects Hornet. + Further information can be found in + Documentation/admin-guide/LSM/Hornet.rst. + + If you are unsure how to answer this question, answer N. diff --git a/security/hornet/Makefile b/security/hornet/Makefile new file mode 100644 index 000000000000..79f4657b215f --- /dev/null +++ b/security/hornet/Makefile @@ -0,0 +1,4 @@ +# SPDX-License-Identifier: GPL-2.0-only +obj-$(CONFIG_SECURITY_HORNET) := hornet.o + +hornet-y := hornet_lsm.o diff --git a/security/hornet/hornet_lsm.c b/security/hornet/hornet_lsm.c new file mode 100644 index 000000000000..d9e36764f968 --- /dev/null +++ b/security/hornet/hornet_lsm.c @@ -0,0 +1,239 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Hornet Linux Security Module + * + * Author: Blaise Boscaccy <bboscaccy@linux.microsoft.com> + * + * Copyright (C) 2025 Microsoft Corporation + */ + +#include <linux/lsm_hooks.h> +#include <uapi/linux/lsm.h> +#include <linux/bpf.h> +#include <linux/verification.h> +#include <crypto/public_key.h> +#include <linux/module_signature.h> +#include <crypto/pkcs7.h> +#include <linux/bpf_verifier.h> +#include <linux/sort.h> + +#define EBPF_SIG_STRING "~eBPF signature appended~\n" + +struct hornet_maps { + u32 used_idx[MAX_USED_MAPS]; + u32 used_map_cnt; + bpfptr_t fd_array; +}; + +static int cmp_idx(const void *a, const void *b) +{ + return *(const u32 *)a - *(const u32 *)b; +} + +static int add_used_map(struct hornet_maps *maps, int idx) +{ + int i; + + for (i = 0; i < maps->used_map_cnt; i++) + if (maps->used_idx[i] == idx) + return i; + + if (maps->used_map_cnt >= MAX_USED_MAPS) + return -E2BIG; + + maps->used_idx[maps->used_map_cnt] = idx; + return maps->used_map_cnt++; +} + +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; + } + } + } + /* Sort the spare-array indices. This should match the map ordering used during + * signature generation + */ + sort(maps->used_idx, maps->used_map_cnt, sizeof(*maps->used_idx), + cmp_idx, NULL); + + return 0; +} + +static int hornet_populate_fd_array(struct hornet_maps *maps, u32 fd_array_cnt) +{ + int i; + + if (fd_array_cnt > MAX_USED_MAPS) + return -E2BIG; + + for (i = 0; i < fd_array_cnt; i++) + maps->used_idx[i] = i; + + maps->used_map_cnt = fd_array_cnt; + return 0; +} + +/* kern_sys_bpf is declared as an EXPORT_SYMBOL in kernel/bpf/syscall.c, however no definition is + * provided in any bpf header files. If/when this function has a proper definition provided + * somewhere this declaration should be removed + */ +int kern_sys_bpf(int cmd, union bpf_attr *attr, unsigned int size); + +static int hornet_verify_lskel(struct bpf_prog *prog, struct hornet_maps *maps, + void *sig, size_t sig_len) +{ + int fd; + u32 i; + void *buf; + void *new; + size_t buf_sz; + struct bpf_map *map; + int err = 0; + int key = 0; + union bpf_attr attr = {0}; + + buf = kmalloc_array(prog->len, sizeof(struct bpf_insn), GFP_KERNEL); + if (!buf) + return -ENOMEM; + buf_sz = prog->len * sizeof(struct bpf_insn); + memcpy(buf, prog->insnsi, buf_sz); + + for (i = 0; i < maps->used_map_cnt; i++) { + err = copy_from_bpfptr_offset(&fd, maps->fd_array, + maps->used_idx[i] * sizeof(fd), + sizeof(fd)); + if (err < 0) + continue; + if (fd < 1) + continue; + + map = bpf_map_get(fd); + if (IS_ERR(map)) + continue; + + /* don't allow userspace to change map data used for signature verification */ + if (!map->frozen) { + attr.map_fd = fd; + err = kern_sys_bpf(BPF_MAP_FREEZE, &attr, sizeof(attr)); + if (err < 0) + goto out; + } + + new = krealloc(buf, buf_sz + map->value_size, GFP_KERNEL); + if (!new) { + err = -ENOMEM; + goto out; + } + buf = new; + new = map->ops->map_lookup_elem(map, &key); + if (!new) { + err = -ENOENT; + goto out; + } + memcpy(buf + buf_sz, new, map->value_size); + buf_sz += map->value_size; + } + + err = verify_pkcs7_signature(buf, buf_sz, sig, sig_len, + VERIFY_USE_SECONDARY_KEYRING, + VERIFYING_EBPF_SIGNATURE, + NULL, NULL); +out: + kfree(buf); + return err; +} + +static int hornet_check_binary(struct bpf_prog *prog, union bpf_attr *attr, + struct hornet_maps *maps) +{ + struct file *file = get_task_exe_file(current); + const unsigned long markerlen = sizeof(EBPF_SIG_STRING) - 1; + void *buf = NULL; + size_t sz = 0, sig_len, prog_len, buf_sz; + int err = 0; + struct module_signature sig; + + buf_sz = kernel_read_file(file, 0, &buf, INT_MAX, &sz, READING_EBPF); + fput(file); + if (!buf_sz) + return -1; + + prog_len = buf_sz; + + if (prog_len > markerlen && + memcmp(buf + prog_len - markerlen, EBPF_SIG_STRING, markerlen) == 0) + prog_len -= markerlen; + + memcpy(&sig, buf + (prog_len - sizeof(sig)), sizeof(sig)); + sig_len = be32_to_cpu(sig.sig_len); + prog_len -= sig_len + sizeof(sig); + + err = mod_check_sig(&sig, prog->len * sizeof(struct bpf_insn), "ebpf"); + if (err) + return err; + return hornet_verify_lskel(prog, maps, buf + prog_len, sig_len); +} + +static int hornet_check_signature(struct bpf_prog *prog, union bpf_attr *attr, + struct bpf_token *token) +{ + struct hornet_maps maps = {0}; + int err; + + /* support both sparse arrays and explicit continuous arrays of map fds */ + if (attr->fd_array_cnt) + err = hornet_populate_fd_array(&maps, attr->fd_array_cnt); + else + err = hornet_find_maps(prog, &maps); + + if (err < 0) + return err; + + maps.fd_array = make_bpfptr(attr->fd_array, false); + return hornet_check_binary(prog, attr, &maps); +} + +static int hornet_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr, + struct bpf_token *token, bool is_kernel) +{ + if (is_kernel) + return 0; + return hornet_check_signature(prog, attr, token); +} + +static struct security_hook_list hornet_hooks[] __ro_after_init = { + LSM_HOOK_INIT(bpf_prog_load, hornet_bpf_prog_load), +}; + +static const struct lsm_id hornet_lsmid = { + .name = "hornet", + .id = LSM_ID_HORNET, +}; + +static int __init hornet_init(void) +{ + pr_info("Hornet: eBPF signature verification enabled\n"); + security_add_hooks(hornet_hooks, ARRAY_SIZE(hornet_hooks), &hornet_lsmid); + return 0; +} + +DEFINE_LSM(hornet) = { + .name = "hornet", + .init = hornet_init, +};
This adds the Hornet Linux Security Module which provides signature verification of eBPF programs. This allows users to continue to maintain an invariant that all code running inside of the kernel has been signed. The primary target for signature verification is light-skeleton based eBPF programs which was introduced here: https://lore.kernel.org/bpf/20220209054315.73833-1-alexei.starovoitov@gmail.com/ eBPF programs, before loading, undergo a complex set of operations which transform pseudo-values within the immediate operands of instructions into concrete values based on the running system. Typically, this is done by libbpf in userspace. Light-skeletons were introduced in order to support preloading of bpf programs and user-mode-drivers by removing the dependency on libbpf and userspace-based operations. Userpace modifications, which may change every time a program gets loaded or runs on a slightly different kernel, break known signature verification algorithms. A method is needed for passing unadulterated binary buffers into the kernel in-order to use existing signature verification algorithms. Light-skeleton loaders with their support of only in-kernel relocations fit that constraint. Hornet employs a signature verification scheme similar to that of kernel modules. A signature is appended to the end of an executable file. During an invocation of the BPF_PROG_LOAD subcommand, a signature is extracted from the current task's executable file. That signature is used to verify the integrity of the bpf instructions and maps which were passed into the kernel. Additionally, Hornet implicitly trusts any programs which were loaded from inside kernel rather than userspace, which allows BPF_PRELOAD programs along with outputs for BPF_SYSCALL programs to run. The validation check consists of checking a PKCS#7 formatted signature against a data buffer containing the raw instructions of an eBPF program, followed by the initial values of any maps used by the program. Maps are frozen before signature verification checking to stop TOCTOU attacks. Signed-off-by: Blaise Boscaccy <bboscaccy@linux.microsoft.com> --- Documentation/admin-guide/LSM/Hornet.rst | 55 ++++++ Documentation/admin-guide/LSM/index.rst | 1 + MAINTAINERS | 9 + crypto/asymmetric_keys/pkcs7_verify.c | 10 + include/linux/kernel_read_file.h | 1 + include/linux/verification.h | 1 + include/uapi/linux/lsm.h | 1 + security/Kconfig | 3 +- security/Makefile | 1 + security/hornet/Kconfig | 11 ++ security/hornet/Makefile | 4 + security/hornet/hornet_lsm.c | 239 +++++++++++++++++++++++ 12 files changed, 335 insertions(+), 1 deletion(-) create mode 100644 Documentation/admin-guide/LSM/Hornet.rst create mode 100644 security/hornet/Kconfig create mode 100644 security/hornet/Makefile create mode 100644 security/hornet/hornet_lsm.c