* Re: [PATCH 11/12] bpftool: Add support for signing BPF programs [not found] ` <20250606232914.317094-12-kpsingh@kernel.org> @ 2025-06-08 14:03 ` James Bottomley 2025-06-10 8:50 ` KP Singh 0 siblings, 1 reply; 5+ messages in thread From: James Bottomley @ 2025-06-08 14:03 UTC (permalink / raw) To: KP Singh, bpf, linux-security-module Cc: bboscaccy, paul, kys, ast, daniel, andrii, keyrings [+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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 11/12] bpftool: Add support for signing BPF programs 2025-06-08 14:03 ` [PATCH 11/12] bpftool: Add support for signing BPF programs James Bottomley @ 2025-06-10 8:50 ` KP Singh 2025-06-10 15:56 ` James Bottomley 2025-06-10 16:34 ` Blaise Boscaccy 0 siblings, 2 replies; 5+ messages in thread From: KP Singh @ 2025-06-10 8:50 UTC (permalink / raw) To: James Bottomley Cc: bpf, linux-security-module, bboscaccy, paul, kys, ast, daniel, andrii, keyrings 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 11/12] bpftool: Add support for signing BPF programs 2025-06-10 8:50 ` KP Singh @ 2025-06-10 15:56 ` James Bottomley 2025-06-10 16:41 ` KP Singh 2025-06-10 16:34 ` Blaise Boscaccy 1 sibling, 1 reply; 5+ messages in thread From: James Bottomley @ 2025-06-10 15:56 UTC (permalink / raw) To: KP Singh Cc: bpf, linux-security-module, bboscaccy, paul, kys, ast, daniel, andrii, keyrings 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 11/12] bpftool: Add support for signing BPF programs 2025-06-10 15:56 ` James Bottomley @ 2025-06-10 16:41 ` KP Singh 0 siblings, 0 replies; 5+ messages in thread From: KP Singh @ 2025-06-10 16:41 UTC (permalink / raw) To: James Bottomley Cc: bpf, linux-security-module, bboscaccy, paul, kys, ast, daniel, andrii, keyrings 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 11/12] bpftool: Add support for signing BPF programs 2025-06-10 8:50 ` KP Singh 2025-06-10 15:56 ` James Bottomley @ 2025-06-10 16:34 ` Blaise Boscaccy 1 sibling, 0 replies; 5+ messages in thread From: Blaise Boscaccy @ 2025-06-10 16:34 UTC (permalink / raw) To: KP Singh, James Bottomley Cc: bpf, linux-security-module, paul, kys, ast, daniel, andrii, keyrings 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 >> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-06-10 16:41 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20250606232914.317094-1-kpsingh@kernel.org>
[not found] ` <20250606232914.317094-12-kpsingh@kernel.org>
2025-06-08 14:03 ` [PATCH 11/12] bpftool: Add support for signing BPF programs James Bottomley
2025-06-10 8:50 ` KP Singh
2025-06-10 15:56 ` James Bottomley
2025-06-10 16:41 ` KP Singh
2025-06-10 16:34 ` Blaise Boscaccy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox