Message-ID: <CAHC9VhQT=ymqssa9ymXtvssHTdVH_64T8Mpb0Mh8oxRD0Guo_Q@mail.gmail.com>
Date: Mon, 2 Jun 2025 18:40:35 -0400
From: Paul Moore <paul@...l-moore.com>
To: Blaise Boscaccy <bboscaccy@...ux.microsoft.com>
Cc: jarkko@...nel.org, zeffron@...tgames.com, xiyou.wangcong@...il.com, 
	kysrinivasan@...il.com, code@...icks.com, 
	linux-security-module@...r.kernel.org, roberto.sassu@...wei.com, 
	James.Bottomley@...senpartnership.com, Alexei Starovoitov <ast@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, John Fastabend <john.fastabend@...il.com>, 
	Andrii Nakryiko <andrii@...nel.org>, Martin KaFai Lau <martin.lau@...ux.dev>, 
	Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>, 
	Yonghong Song <yonghong.song@...ux.dev>, KP Singh <kpsingh@...nel.org>, 
	Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, 
	David Howells <dhowells@...hat.com>, Lukas Wunner <lukas@...ner.de>, 
	Ignat Korchagin <ignat@...udflare.com>, Quentin Monnet <qmo@...nel.org>, 
	Jason Xing <kerneljasonxing@...il.com>, Willem de Bruijn <willemb@...gle.com>, 
	Anton Protopopov <aspsk@...valent.com>, Jordan Rome <linux@...danrome.com>, 
	Martin Kelly <martin.kelly@...wdstrike.com>, Alan Maguire <alan.maguire@...cle.com>, 
	Matteo Croce <teknoraver@...a.com>, bpf@...r.kernel.org, linux-kernel@...r.kernel.org, 
	keyrings@...r.kernel.org, linux-crypto@...r.kernel.org
Subject: Re: [PATCH 1/3] bpf: Add bpf_check_signature

On Wed, May 28, 2025 at 5:50 PM Blaise Boscaccy
<bboscaccy@...ux.microsoft.com> wrote:
>
> This introduces signature verification for eBPF programs inside of the
> bpf subsystem. Two signature validation schemes are included, one that
> only checks the instruction buffer, and another that checks over a
> hash chain constructed from the program and a list of maps. The
> alternative algorithm is designed to provide support to scenarios
> where having self-aborting light-skeletons or signature checking
> living outside the kernel-proper is insufficient or undesirable.
>
> An abstract hash method is introduced to allow calculating the hash of
> maps, only arrays are implemented at this time.
>
> A simple UAPI is introduced to provide passing signature information.
>
> The signature check is performed before the call to
> security_bpf_prog_load. This allows the LSM subsystem to be clued into
> the result of the signature check, whilst granting knowledge of the
> method and apparatus which was employed.
>
> Signed-off-by: Blaise Boscaccy <bboscaccy@...ux.microsoft.com>
> ---
>  include/linux/bpf.h            |   2 +
>  include/linux/verification.h   |   1 +
>  include/uapi/linux/bpf.h       |   4 ++
>  kernel/bpf/arraymap.c          |  11 ++-
>  kernel/bpf/syscall.c           | 123 ++++++++++++++++++++++++++++++++-
>  tools/include/uapi/linux/bpf.h |   4 ++
>  6 files changed, 143 insertions(+), 2 deletions(-)

...

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 64c3393e8270..7dc35681d3f8 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2753,8 +2764,113 @@ static bool is_perfmon_prog_type(enum bpf_prog_type prog_type)
>         }
>  }
>
> +static int bpf_check_signature(struct bpf_prog *prog, union bpf_attr *attr, bpfptr_t uattr,
> +                              __u32 uattr_size)
> +{
> +       u64 hash[4];
> +       u64 buffer[8];
> +       int err;
> +       char *signature;
> +       int *used_maps;
> +       int n;
> +       int map_fd;
> +       struct bpf_map *map;
> +
> +       if (!attr->signature)
> +               return 0;
> +
> +       signature = kmalloc(attr->signature_size, GFP_KERNEL);
> +       if (!signature) {
> +               err = -ENOMEM;
> +               goto out;
> +       }
> +
> +       if (copy_from_bpfptr(signature,
> +                            make_bpfptr(attr->signature, uattr.is_kernel),
> +                            attr->signature_size) != 0) {
> +               err = -EINVAL;
> +               goto free_sig;
> +       }
> +
> +       if (!attr->signature_maps_size) {
> +               sha256((u8 *)prog->insnsi, prog->len * sizeof(struct bpf_insn), (u8 *)&hash);
> +               err = verify_pkcs7_signature(hash, sizeof(hash), signature, attr->signature_size,
> +                                    VERIFY_USE_SECONDARY_KEYRING,
> +                                    VERIFYING_EBPF_SIGNATURE,
> +                                    NULL, NULL);
> +       } else {
> +               used_maps = kmalloc_array(attr->signature_maps_size,
> +                                         sizeof(*used_maps), GFP_KERNEL);
> +               if (!used_maps) {
> +                       err = -ENOMEM;
> +                       goto free_sig;
> +               }
> +               n = attr->signature_maps_size;
> +               n--;
> +
> +               err = copy_from_bpfptr_offset(&map_fd, make_bpfptr(attr->fd_array, uattr.is_kernel),
> +                                             used_maps[n] * sizeof(map_fd),
> +                                             sizeof(map_fd));
> +               if (err < 0)
> +                       goto free_maps;
> +
> +               /* calculate the terminal hash */
> +               CLASS(fd, f)(map_fd);
> +               map = __bpf_map_get(f);
> +               if (IS_ERR(map)) {
> +                       err = PTR_ERR(map);
> +                       goto free_maps;
> +               }
> +               if (__map_get_hash(map, (u8 *)hash)) {
> +                       err = -EINVAL;
> +                       goto free_maps;
> +               }
> +
> +               n--;
> +               /* calculate a link in the hash chain */
> +               while (n >= 0) {
> +                       memcpy(buffer, hash, sizeof(hash));
> +                       err = copy_from_bpfptr_offset(&map_fd,
> +                                                     make_bpfptr(attr->fd_array, uattr.is_kernel),
> +                                                     used_maps[n] * sizeof(map_fd),
> +                                                     sizeof(map_fd));
> +                       if (err < 0)
> +                               goto free_maps;
> +
> +                       CLASS(fd, f)(map_fd);
> +                       map = __bpf_map_get(f);
> +                       if (!map) {
> +                               err = -EINVAL;
> +                               goto free_maps;
> +                       }
> +                       if (__map_get_hash(map, (u8 *)buffer+4)) {
> +                               err = -EINVAL;
> +                               goto free_maps;
> +                       }
> +                       sha256((u8 *)buffer, sizeof(buffer), (u8 *)&hash);

James' comment about using the hash from the PKCS7 data makes a lot of
sense.  I'm not a kernel crypto expert, but if looks like if you call
pkcs7_parse_message() you should be able to get the hash algorithm
from pkcs7_message->signed_infos->sig->hash_algo.

I imagine there might be user/admin concerns over which algorithms
would be considered acceptable for a signature verification, but I
suppose one could make the argument that if you don't trust that
algorithm it shouldn't be enabled in the kernel.

> +                       n--;
> +               }
> +               /* calculate the root hash and verify it's signature */
> +               sha256((u8 *)prog->insnsi, prog->len * sizeof(struct bpf_insn), (u8 *)&buffer);
> +               memcpy(buffer+4, hash, sizeof(hash));
> +               sha256((u8 *)buffer, sizeof(buffer), (u8 *)&hash);
> +               err = verify_pkcs7_signature(hash, sizeof(hash), signature, attr->signature_size,
> +                                    VERIFY_USE_SECONDARY_KEYRING,
> +                                    VERIFYING_EBPF_SIGNATURE,
> +                                    NULL, NULL);
> +free_maps:
> +               kfree(used_maps);
> +       }
> +
> +free_sig:
> +       kfree(signature);
> +out:
> +       prog->aux->signature_verified = !err;

Considering this code supports two signature schemes, signed loader
(with implied loader verification of maps) and signed loader + maps,
it seems like it might be a good idea to have two flags to indicate
what has been verified in bpf_check_signature(); a "prog_sig_verified"
(or similar) flag to indicate the program has been verified and a
"maps_sig_verified" (or similar) to indicate the maps have been
verified.

Beyond that, I wanted to talk a bit about the two different signature
schemes and why I think there is value in supporting both.  The
discussion was happening in patch 0/3, but it looks like KP wanted to
move the discussion away from the cover letter and into that patch, so
I'm doing that here ...

The loader (+ implicit loader verification of maps w/original program)
signature verification scheme has been requested by Alexei/KP, and
that's fine, the code is trivial and if the user/admin is satisfied
with that as a solution, great.  However, the loader + map signature
verification scheme has some advantages and helps satisfy some
requirements that are not satisfied by only verifying the loader and
relying on the loader to verify the original program stored in the
maps.  One obvious advantage is that the lskel loader is much simpler
in this case as it doesn't need to worry about verification of the
program maps as that has already been done in bpf_check_signature().
I'm sure there are probably some other obvious reasons, but beyond the
one mentioned above, the other advantages that I'm interested in are a
little less obvious, or at least I haven't seen them brought up yet.
As I mentioned in an earlier thread, it's important to have the LSM
hook that handles authorization of a BPF program load *after* the BPF
program's signature has been verified.  This is not simply because the
LSM implementation might want to enforce and access control on a BPF
program load due to the signature state (signature verified vs no
signature), but also because the LSM might want to measure system
state and/or provide a record of the operation.  If we only verify the
lskel loader, at the point in time that the security_bpf_prog_load()
hook is called, we haven't properly verified both the loader and the
original BPF program stored in the map, that doesn't happen until much
later when the lskel loader executes.  Yes, I understand that may
sound very pedantic and fussy, but there are users who care very much
about those details, and if they see an event in the logs that
indicates that the BPF program signature has been verified as "good",
they need that log event to be fully, 100% true, and not have an
asterix of "only the lskel loader has been verified, the original BPF
program will potentially be verified later without any additional
events being logged to indicate the verification".

Considering that Blaise has proposed something which satisfies both
the loader-only and loader+maps signature requirements, I don't
understand the objections.  KP described two technical objections in
his replies to patch 0/3:

1. "It does not align with most BPF use-cases out there as most
use-cases need a trusted loader."
2. "Locks us into a UAPI, whereas a signed LOADER allows us to
incrementally build signing for all use-cases without compromising the
security properties."

In response to objection #1, the approach Blaise has described here
fully supports signing only the lskel loader and leaving it to the
loader to verify the original BPF program maps.  The "trusted loader"
use cases are fully supported, as the loader+maps scheme does not
prevent the loader-only signature scheme.

In response to objection #2, honestly this seems like an extension to
objection #1.  The trusted loader-only signature scheme is fully
supported.  Yes, adding support for either signature schemes is an
extension to the BPF program loading UAPI, but both schemes are
optional and supporting both is very much in line with BPF's stated
philosophy of "flexibility and not locking the users into a rigid
in-kernel implementation and UAPI."

> +       return err;
> +}
> +

-- 
paul-moore.com

Powered by blists - more mailing lists