Re: [PATCH 10/12] libbpf: Embed and verify the metadata hash in the loader

[KP Singh <kpsingh@kernel.org>]

On Wed, Jun 11, 2025 at 1:59 PM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Wed, 2025-06-11 at 00:35 +0200, KP Singh wrote:
> > On Tue, Jun 10, 2025 at 11:24 PM James Bottomley
> > <James.Bottomley@hansenpartnership.com> wrote:
> > >
> > > On Tue, 2025-06-10 at 21:47 +0200, KP Singh wrote:
> > > > It's been repeatedly mentioned that trusted loaders (whether
> > > > kernel or BPF programs) are the only way because a large number
> > > > of BPF use-cases dynamically generate BPF programs.
> > >
> > > You keep asserting this, but it isn't supported by patches already
> >
> > This is supported for sure. But it's not what the patches are
> > providing a reference implementation for. The patches provide a stand
> > alone reference implementation using in-kernel / BPF loaders but you
> > can surely implement this (see below):
> >
> > > proposed.  Specifically, there already exists a patch set:
> > >
> > > https://lore.kernel.org/all/20250528215037.2081066-1-bboscaccy@linux.microsoft.com/
> >
> > The patch-set takes a very narrow view by adding additional UAPI and
> > ties us into an implementation.
>
> What do you mean by this?  When kernel people say UAPI, they think of
> the contract between the kernel and userspace.  So for both patch sets
> the additional attr. entries which user space adds and the kernel
> parses for the signature would conventionally be thought to extend the
> UAPI.
>
> Additionally, the content of the signature (what it's over) is a UAPI
> contract.  When adding to the kernel UAPI we don't look not to change
> it, we look to change it in a way that is extensible.  It strikes me
> that actually only the linked patch does this because the UAPI addition
> for your signature scheme doesn't seem to be that extensible.

James, I am adding less attributes, it's always extensible, adding
more UAPI than strictly needed is what's not flexible.

The attributes I proposed remain valid in a world where the BPF
instruction set is stable at compile time, for trusted user space
loaders (applications like Cilium) that can already have a stable
instruction buffer, the attributes Blaise proposed do not.

I believe we have discussed this enough. Let's have the BPF maintainers decide.

>
> >  Whereas the current approach keeps the UAPI clean while still
> > meeting all the use-cases and keeps the implementation flexible
> > should it need to change. (no tie into the hash chain approach, if we
> > are able to move to stable BPF instruction buffers in the future).
> >
> > Blaise's patches also do not handle the trusted user-space loader
> > space and the "signature_maps" are not relevant to dynamic generation
> > or simple BPF programs like networking, see below.
>
> OK, is this just a technical misreading?  I missed the fact that it
> supported both schemes on first reading as well.  If you look in this
> patch:
>
> https://lore.kernel.org/all/20250528215037.2081066-2-bboscaccy@linux.microsoft.com/
>
> It's this addition in bpf_check_signature():
>
> > +     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);
> > [...]
>
> The first leg of the if is your use case: a zero map size means the
> signature is a single hash of the loader only.  The else clause
> encompasses a hash chain over the maps as well.  This means the signer
> can choose which scheme they want.

I have read and understood the code, there is no technical misalignment.

I am talking about a trusted user space loader. You seem to confuse
the trusted BPF loader program as userspace, no this is not userspace,
it runs in the kernel context.

- KP

>
> I'll skip responding to the rest since it seems to be assuming that
> Blaise's patch excludes your use case (which the above should
> demonstrate it doesn't) and we'd be talking past each other.
>
> Regards,
>
> James
>