| Message ID | 20250404215527.1563146-1-bboscaccy@linux.microsoft.com |
|---|---|
| Headers | show |
| Series | Introducing Hornet LSM | expand |
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 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.
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.
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); >> +}
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.
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
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 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.