mbox series

[v2,security-next,0/4] Introducing Hornet LSM

Message ID 20250404215527.1563146-1-bboscaccy@linux.microsoft.com
Headers show
Series Introducing Hornet LSM | expand

Message

Blaise Boscaccy April 4, 2025, 9:54 p.m. UTC
This patch series introduces the Hornet LSM. The goal of Hornet is to
provide a signature verification mechanism for eBPF programs.

eBPF has similar requirements to that of modules when it comes to
loading: find symbol addresses, fix up ELF relocations, some struct
field offset handling stuff called CO-RE (compile-once run-anywhere),
and some other miscellaneous bookkeeping. During eBPF program
compilation, pseudo-values get written to the immediate operands of
instructions. During loading, those pseudo-values get rewritten with
concrete addresses or data applicable to the currently running system,
e.g., a kallsyms address or an fd for a map. This needs to happen
before the instructions for a bpf program are loaded into the kernel
via the bpf() syscall. Unlike modules, an in-kernel loader
unfortunately doesn't exist. Typically, the instruction rewriting is
done dynamically in userspace via libbpf. Since the relocations and
instruction modifications are happening in userspace, and their values
may change depending upon the running system, this breaks known
signature verification mechanisms.

Light skeleton programs were introduced in order to support early
loading of eBPF programs along with user-mode drivers. They utilize a
separate eBPF program that can load a target eBPF program and perform
all necessary relocations in-kernel without needing a working
userspace. Light skeletons were mentioned as a possible path forward
for signature verification.

Hornet takes a simple approach to light-skeleton-based eBPF signature
verification. A PKCS#7 signature of a data buffer containing the raw
instructions of an eBPF program, followed by the initial values of any
maps used by the program is used. A utility script is provided to
parse and extract the contents of autogenerated header files created
via bpftool. That payload can then be signed and appended to the light
skeleton executable.

Maps are frozen to prevent TOCTOU bugs where a sufficiently privileged
user could rewrite map data between the calls to BPF_PROG_LOAD and
BPF_PROG_RUN. Additionally, both sparse-array-based and
fd_array_cnt-based map fd arrays are supported for signature
verification.


References:
  [1] https://lore.kernel.org/bpf/20220209054315.73833-1-alexei.starovoitov@gmail.com/
  [2] https://lore.kernel.org/bpf/CAADnVQ+wPK1KKZhCgb-Nnf0Xfjk8M1UpX5fnXC=cBzdEYbv_kg@mail.gmail.com/

Change list:
- v1 -> v2
  - Jargon clarification, maintainer entry and a few cosmetic fixes

Revisions:
- v1
  https://lore.kernel.org/bpf/20250321164537.16719-1-bboscaccy@linux.microsoft.com


Blaise Boscaccy (4):
  security: Hornet LSM
  hornet: Introduce sign-ebpf
  hornet: Add a light-skeleton data extactor script
  selftests/hornet: Add a selftest for the Hornet LSM

 Documentation/admin-guide/LSM/Hornet.rst     |  53 +++
 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 +
 scripts/Makefile                             |   1 +
 scripts/hornet/Makefile                      |   5 +
 scripts/hornet/extract-skel.sh               |  29 ++
 scripts/hornet/sign-ebpf.c                   | 411 +++++++++++++++++++
 security/Kconfig                             |   3 +-
 security/Makefile                            |   1 +
 security/hornet/Kconfig                      |  11 +
 security/hornet/Makefile                     |   4 +
 security/hornet/hornet_lsm.c                 | 239 +++++++++++
 tools/testing/selftests/Makefile             |   1 +
 tools/testing/selftests/hornet/Makefile      |  51 +++
 tools/testing/selftests/hornet/loader.c      |  21 +
 tools/testing/selftests/hornet/trivial.bpf.c |  33 ++
 20 files changed, 885 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/admin-guide/LSM/Hornet.rst
 create mode 100644 scripts/hornet/Makefile
 create mode 100755 scripts/hornet/extract-skel.sh
 create mode 100644 scripts/hornet/sign-ebpf.c
 create mode 100644 security/hornet/Kconfig
 create mode 100644 security/hornet/Makefile
 create mode 100644 security/hornet/hornet_lsm.c
 create mode 100644 tools/testing/selftests/hornet/Makefile
 create mode 100644 tools/testing/selftests/hornet/loader.c
 create mode 100644 tools/testing/selftests/hornet/trivial.bpf.c

Comments

Tyler Hicks April 11, 2025, 7:09 p.m. UTC | #1
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);
> +}
Alexei Starovoitov April 12, 2025, 12:09 a.m. UTC | #2
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.
Alexei Starovoitov April 12, 2025, 12:57 a.m. UTC | #3
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.
Blaise Boscaccy April 12, 2025, 2:11 p.m. UTC | #4
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.
Paul Moore April 14, 2025, 4:08 p.m. UTC | #5
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.
Blaise Boscaccy April 14, 2025, 8:11 p.m. UTC | #6
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);
>> +}
Alexei Starovoitov April 14, 2025, 8:56 p.m. UTC | #7
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.
Blaise Boscaccy April 15, 2025, 12:32 a.m. UTC | #8
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.
Alexei Starovoitov April 15, 2025, 1:38 a.m. UTC | #9
On Mon, Apr 14, 2025 at 5:32 PM Blaise Boscaccy
<bboscaccy@linux.microsoft.com> wrote:
>
> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>
> > On Sat, Apr 12, 2025 at 6:58 AM Blaise Boscaccy
> > <bboscaccy@linux.microsoft.com> wrote:
> >>
> >> TAlexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> >>
> >> > On Fri, Apr 4, 2025 at 2:56 PM Blaise Boscaccy
> >> > <bboscaccy@linux.microsoft.com> wrote:
> >> >> +
> >> >> +static int hornet_find_maps(struct bpf_prog *prog, struct hornet_maps *maps)
> >> >> +{
> >> >> +       struct bpf_insn *insn = prog->insnsi;
> >> >> +       int insn_cnt = prog->len;
> >> >> +       int i;
> >> >> +       int err;
> >> >> +
> >> >> +       for (i = 0; i < insn_cnt; i++, insn++) {
> >> >> +               if (insn[0].code == (BPF_LD | BPF_IMM | BPF_DW)) {
> >> >> +                       switch (insn[0].src_reg) {
> >> >> +                       case BPF_PSEUDO_MAP_IDX_VALUE:
> >> >> +                       case BPF_PSEUDO_MAP_IDX:
> >> >> +                               err = add_used_map(maps, insn[0].imm);
> >> >> +                               if (err < 0)
> >> >> +                                       return err;
> >> >> +                               break;
> >> >> +                       default:
> >> >> +                               break;
> >> >> +                       }
> >> >> +               }
> >> >> +       }
> >> >
> >> > ...
> >> >
> >> >> +               if (!map->frozen) {
> >> >> +                       attr.map_fd = fd;
> >> >> +                       err = kern_sys_bpf(BPF_MAP_FREEZE, &attr, sizeof(attr));
> >> >
> >> > Sorry for the delay. Still swamped after conferences and the merge window.
> >> >
> >>
> >> No worries.
> >>
> >> > Above are serious layering violations.
> >> > LSMs should not be looking that deep into bpf instructions.
> >>
> >> These aren't BPF internals; this is data passed in from
> >> userspace. Inspecting userspace function inputs is definitely within the
> >> purview of an LSM.
> >>
> >> Lskel signature verification doesn't actually need a full disassembly,
> >> but it does need all the maps used by the lskel. Due to API design
> >> choices, this unfortunately requires disassembling the program to see
> >> which array indexes are being used.
> >>
> >> > Calling into sys_bpf from LSM is plain nack.
> >> >
> >>
> >> kern_sys_bpf is an EXPORT_SYMBOL, which means that it should be callable
> >> from a module.
> >
> > It's a leftover.
> > kern_sys_bpf() is not something that arbitrary kernel
> > modules should call.
> > It was added to work for cases where kernel modules
> > carry their own lskels.
> > That use case is gone, so EXPORT_SYMBOL will be removed.
> >
>
> I'm not following that at all. You recommended using module-based lskels
> to get around code signing requirements at lsfmmbpf and now you want to
> nuke that entire feature? And further, skel_internal will no longer be
> usable from within the kernel and bpf_preload is no longer going to be
> supported?

It was exported to modules to run lskel-s from modules.
It's bpf internal api, but seeing how you want to abuse it
the feature has to go. Sadly.

> >> Lskels without frozen maps are vulnerable to a TOCTOU
> >> attack from a sufficiently privileged user. Lskels currently pass
> >> unfrozen maps into the kernel, and there is nothing stopping someone
> >> from modifying them between BPF_PROG_LOAD and BPF_PROG_RUN.
> >>
> >> > The verification of module signatures is a job of the module loading process.
> >> > The same thing should be done by the bpf system.
> >> > The signature needs to be passed into sys_bpf syscall
> >> > as a part of BPF_PROG_LOAD command.
> >> > It probably should be two new fields in union bpf_attr
> >> > (signature and length),
> >> > and the whole thing should be processed as part of the loading
> >> > with human readable error reported back through the verifier log
> >> > in case of signature mismatch, etc.
> >> >
> >>
> >> I don't necessarily disagree, but my main concern with this is that
> >> previous code signing patchsets seem to get gaslit or have the goalposts
> >> moved until they die or are abandoned.
> >
> > Previous attempts to add signing failed because
> > 1. It's a difficult problem to solve
> > 2. people only cared about their own narrow use case and not
> > considering the needs of bpf ecosystem as a whole.
> >
> >> Are you saying that at this point, you would be amenable to an in-tree
> >> set of patches that enforce signature verification of lskels during
> >> BPF_PROG_LOAD that live in syscall.c,
> >
> > that's the only way to do it.
> >
>
> So the notion of forcing people into writing bpf-based gatekeeper programs
> is being abandoned? e.g.
>
> https://lore.kernel.org/bpf/bqxgv2tqk3hp3q3lcdqsw27btmlwqfkhyg6kohsw7lwdgbeol7@nkbxnrhpn7qr/#t
> https://lore.kernel.org/bpf/61aae2da8c7b0_68de0208dd@john.notmuch/

Not abandoned.
bpf-based tuning of load conditions is still necessary.
The bpf_prog_load command will check the signature only.
It won't start rejecting progs that don't have a signature.
For that a one liner bpf-lsm or C-based lsm would be needed
to address your dont-trust-root use case.

>
> >> without adding extra non-code
> >> signing requirements like attachment point verification, completely
> >> eBPF-based solutions, or rich eBPF-based program run-time policy
> >> enforcement?
> >
> > Those are secondary considerations that should also be discussed.
> > Not necessarily a blocker.
>
> Again, I'm confused here since you recently stated this whole thing
> was "questionable" without attachment point verification.

Correct.
For fentry prog type the attachment point is checked during the load,
but for tracepoints it's not, and anyone who is claiming that
their system is secure because the tracepoint prog was signed
is simply clueless in how bpf works.
Blaise Boscaccy April 15, 2025, 3:45 p.m. UTC | #10
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 April 15, 2025, 7:08 p.m. UTC | #11
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.
Alexei Starovoitov April 15, 2025, 9:48 p.m. UTC | #12
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.
Blaise Boscaccy April 16, 2025, 5:31 p.m. UTC | #13
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
James Bottomley April 19, 2025, 6:43 p.m. UTC | #14
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",
James Bottomley April 21, 2025, 7:03 p.m. UTC | #15
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
Paul Moore April 21, 2025, 10:03 p.m. UTC | #16
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.
Alexei Starovoitov April 21, 2025, 11:48 p.m. UTC | #17
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.