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

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

[James Bottomley]

[+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.

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
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.

Regards,

James

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

[KP Singh]

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).

> 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
>

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

[James Bottomley]

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@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 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.

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.

Regards,

James


> This patch implements the signing support, not the security policy
> for it.
> 
> - KP

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

[KP Singh]

On Tue, Jun 10, 2025 at 5:56 PM James Bottomley
<James.Bottomley@hansenpartnership.com> 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@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 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
>

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

[Blaise Boscaccy]

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
>>