Message ID | 20250404215527.1563146-2-bboscaccy@linux.microsoft.com (mailing list archive) |
---|---|
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,
On Fri, Apr 11, 2025 at 5:30 PM Matteo Croce <technoboy85@gmail.com> wrote: > > 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/ ... > @@ -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 */ Well, yeah, two fields are obvious. But not like that link from 2021. KP proposed them a year later in 2022 on top of lskel which was much closer to be acceptable. We need to think it through and complete the work, since there are various ways to do it. For example, lskel has a map and a prog. A signature in a prog may cover both, but not necessary it's a good design. A signature for the map plus a signature for the prog that is tied to a map might be a better option. At map creation time the contents can be checked, the map is frozen, and then the verifier can proceed with prog's signature checking. lskel doesn't support all the bpf feature yet, so we need to make sure that the signature verification process is extensible when lskel gains new features. Attaching was also brought up at lsfmm. Without checking the attach point the whole thing is quite questionable from security pov.
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.
Alexei Starovoitov <alexei.starovoitov@gmail.com> writes: > On Fri, Apr 11, 2025 at 5:30 PM Matteo Croce <technoboy85@gmail.com> wrote: >> >> 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/ > ... >> @@ -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 */ > > Well, yeah, two fields are obvious. > But not like that link from 2021. > KP proposed them a year later in 2022 on top of lskel > which was much closer to be acceptable. > We need to think it through and complete the work, > since there are various ways to do it. > For example, lskel has a map and a prog. > A signature in a prog may cover both, but > not necessary it's a good design. > A signature for the map plus a signature for the prog > that is tied to a map might be a better option. > At map creation time the contents can be checked, > the map is frozen, and then the verifier can proceed > with prog's signature checking. > lskel doesn't support all the bpf feature yet, so we need > to make sure that the signature verification process > is extensible when lskel gains new features. > > Attaching was also brought up at lsfmm. > Without checking the attach point the whole thing is quite > questionable from security pov. That statement is quite questionable. Yes, IIRC you brought that up. And again, runtime policy enforcement has nothing to do with proving code provenance. They are completely independent concerns. That would be akin to saying that having locks on a door is questionable without having surveillance cameras installed.
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 4:46 PM Blaise Boscaccy <bboscaccy@linux.microsoft.com> wrote: > Paul Moore <paul@paul-moore.com> writes: > > On Apr 4, 2025 Blaise Boscaccy <bboscaccy@linux.microsoft.com> wrote: ... > >> +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 ... My mistake, I spaced out a bit when looking at this and for some reason thought it was reading the BPF program/lskel itself and not the signature, meaning that the BPF that was being checked by Hornet was not necessarily the same BPF that was actually loaded. However, that's not the case here, as you rightly point out it is just the signature that is being read by the kernel_read_file() which shouldn't be an issue. Thanks for the correction and sorry for the noise :)
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.
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. > Are we in preschool again? You don't like how others are playing with your toys so you want to take them away from everyone. Forever. >> >> 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.
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.
Alexei Starovoitov <alexei.starovoitov@gmail.com> writes: > 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. > Well, I suppose anything worth doing is going to be hard :) The end-to-end design for this is the same end-to-end design that exists for signing kernel modules today. We envisioned it working for others the same way module signing works for others. > 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/ -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