On Tue, Jun 10, 2025 at 5:56 PM James Bottomley
<James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, 2025-06-10 at 10:50 +0200, KP Singh wrote:
> > On Sun, Jun 8, 2025 at 4:03 PM James Bottomley
> > <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> 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 problem is you're hard coding it at light skeleton creation time.
> Plus there doesn't seem to be any ability to use the system keyrings
> anyway as the kernel code only looks up the user keyrings. Since
> actual key ids are volatile handles which change from boot to boot (so
> can't be stored in anything durable) this can only be used for keyring
> specifiers, so it would also make sense to check this is actually a
> specifier (system keyring specifiers are positive and user specifiers
> negative, so it's easy to check for the range).
>
> > > 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.
>
> It's a classic supply chain attack (someone modifies the light skeleton
> between the creator and the consumer), even Google is claiming SLSA
> guarantees, so you can't just wave it away as "other issues".
>
> > So, this does not work in independence, signed BPF programs do not
> > really make sense without trusted execution).
>
> The other patch set provided this ability using signed hash chains, so
> absolutely there are signed bpf programmes that can work absent a
> trusted user execution environment. It may not be what you want for
> your use case (which is why the other patch set allowed for both), but
> there are lots of integrity use cases out there wanting precisely this.
>
> > > 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.
>
> You can't really have it both ways: either the keyring is part of the
> LSM supplied policy in which case it doesn't make much sense to have it
> in the durable attributes (and the LSM would have to set it before the
> signature is verified) or it's part of the durable attribute embedded
> security information and should be integrity protected.
>
> I suppose we could compromise and say it should not be part of the
> light skeleton durable attributes but should be set (or supplied by
> policy) at BPF_PROG_LOAD time.
Sure, this is expected, I added a default value there but this can be removed.
>
> I should also note that when other systems use derived keys in
> different keyrings, they usually have a specific named trusted keyring
> (like _ima and .ima) which has policy enforced rules for adding keys.
We can potentially add a bpf keyring but in general we don't want
every binary on the machine to use this derived key, but the binary
that's trusted to either load unsigned programs or use a derived key
for which the session keyring is more apt.
- KP
>
> Regards,
>
> James
>
>
> > This patch implements the signing support, not the security policy
> > for it.
> >
> > - KP
>
