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

[James Bottomley]

On Wed, 2025-06-11 at 14:33 +0200, KP Singh wrote:
> On Wed, Jun 11, 2025 at 1:59 PM James Bottomley
> <James.Bottomley at 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 at 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.

To repeat: the object should be extensibility not minimization.  If an
API is extensible it doesn't tie you to a specific implementation
regardless of how many arguments it adds.  The attr structure uses the
standard kernel way of doing this: it can grow but may never lose
elements and  features added at the end are always optional so an older
kernel that doesn't see them can still process everything it does
understand.

> 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 don't follow.  For stable compilation (I'm more familiar with the way
systemd does this but I presume cilium does the same: by constructing
ebpf byte code on the fly that doesn't require relocation and then
inserting it directly) you simply program the loader to do the
restrictions (about insertion point and the like) and sign it, correct?
That's covered in the linked patch in the !attr->signature_maps_size
case, so what Blaise proposed most definitely does do this.

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

But this is obviously an important point otherwise you wouldn't be
arguing about it.  If pure minimization were all that's required then
it's easy to do since we're using pkcs7 signatures, the signature can
contain a data structure with authenticatedAttributes that are
validated by the signature, so I could do the Blaise patch with fewer
attr elements than you simply by moving the maps and their count into
the athenticatedAttributes element of the pkcs7 signature.  I could
also do the same with your keyring_id and, bonus, it would be integrity
validated.  Then each of you adds the same number of UAPI attr's so
there's no argument about who adds fewer attributes.

Regards,

James