Re: [PATCH 11/12] bpftool: Add support for signing BPF programs

[Blaise Boscaccy <bboscaccy@linux.microsoft.com>]

KP Singh <kpsingh@kernel.org> writes:

> On Sun, Jun 8, 2025 at 4:03 PM James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
>>
>> [+keyrings]
>> On Sat, 2025-06-07 at 01:29 +0200, KP Singh wrote:
>> [...]
>> > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
>> > index f010295350be..e1dbbca91e34 100644
>> > --- a/tools/bpf/bpftool/prog.c
>> > +++ b/tools/bpf/bpftool/prog.c
>> > @@ -23,6 +23,7 @@
>> >  #include <linux/err.h>
>> >  #include <linux/perf_event.h>
>> >  #include <linux/sizes.h>
>> > +#include <linux/keyctl.h>
>> >
>> >  #include <bpf/bpf.h>
>> >  #include <bpf/btf.h>
>> > @@ -1875,6 +1876,8 @@ static int try_loader(struct gen_loader_opts
>> > *gen)
>> >  {
>> >       struct bpf_load_and_run_opts opts = {};
>> >       struct bpf_loader_ctx *ctx;
>> > +     char sig_buf[MAX_SIG_SIZE];
>> > +     __u8 prog_sha[SHA256_DIGEST_LENGTH];
>> >       int ctx_sz = sizeof(*ctx) + 64 * max(sizeof(struct
>> > bpf_map_desc),
>> >                                            sizeof(struct
>> > bpf_prog_desc));
>> >       int log_buf_sz = (1u << 24) - 1;
>> > @@ -1898,6 +1901,24 @@ static int try_loader(struct gen_loader_opts
>> > *gen)
>> >       opts.insns = gen->insns;
>> >       opts.insns_sz = gen->insns_sz;
>> >       fds_before = count_open_fds();
>> > +
>> > +     if (sign_progs) {
>> > +             opts.excl_prog_hash = prog_sha;
>> > +             opts.excl_prog_hash_sz = sizeof(prog_sha);
>> > +             opts.signature = sig_buf;
>> > +             opts.signature_sz = MAX_SIG_SIZE;
>> > +             opts.keyring_id = KEY_SPEC_SESSION_KEYRING;
>> > +
>>
>> This looks wrong on a couple of levels.  Firstly, if you want system
>> level integrity you can't search the session keyring because any
>> process can join (subject to keyring permissions) and the owner, who is
>> presumably the one inserting the bpf program, can add any key they
>> like.
>>
>
> Wanting system level integrity is a security policy question, so this
> is something that needs to be implemented at the security layer, the
> LSM can deny the keys / keyring IDs they don't trust.  Session
> keyrings are for sure useful for delegated signing of BPF programs
> when dynamically generated.
>
>> The other problem with this scheme is that the keyring_id itself has no
>> checked integrity, which means that even if a script was marked as
>
> If an attacker can modify a binary that has permissions to load BPF
> programs and update the keyring ID then we have other issues. So, this
> does not work in independence, signed BPF programs do not really make
> sense without trusted execution).
>

Untrusted userspace/root is precisely the issue I solved with previous
patchsets for this effort. Signed BPF programs absolutely work without
trusted execution.

-blaise

>> system keyring only anyone can binary edit the user space program to
>> change it to their preferred keyring and it will still work.  If you
>> want variable keyrings, they should surely be part of the validated
>> policy.
>
> The policy is what I expect to be implemented in the LSM layer. A
> variable keyring ID is a critical part of the UAPI to create different
> "rings of trust" e.g. LSM can enforce that network programs can be
> loaded with a derived key, and have a different keyring for
> unprivileged BPF programs.
>
> This patch implements the signing support, not the security policy for it.
>
> - KP
>
>>
>> Regards,
>>
>> James
>>