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

[Blaise Boscaccy <bboscaccy@linux.microsoft.com>]

KP Singh <kpsingh@kernel.org> writes:

[...]

>>
>> The above code gets generated per-program and exists out-of-tree in a
>> very unreadable format in it's final form. I have general objections to
>> being forced to "trust" out-of-tree code, when it's demostrably trivial
>
> This is not out of tree. It's very much within the kernel tree.

No, it's not.

Running something like

bpftool gen skeleton -S -k <private_key> -i <identity_cert>
fentry_test.bpf.o

will yield a header file fentery_test.h or whatever. That header file
contains a customized and one-off version of the templated code in this
patch. That header file and the resultant loader it gets compiled into
exists out-of-tree.

>
>> to perform this check in-kernel, without impeding any of the other
>> stated use cases. There is no possible audit log nor LSM hook for these
>> operations. There is no way to know that this check was ever performed.
>>
>> Further, this check ends up happeing in an entirely different syscall,
>> the LSM layer and the end user may both see invalid programs successfully
>> being loaded into the kernel, that may fail mysteriously later.
>>
>> Also, this patch seems to rely on hacking into struct internals and
>> magic binary layouts.
>
> These magical binary layouts are BPF programs, as I mentioned, if you
> don't like this you (i.e an advanced user like Microsoft) can
> implement your own trusted loader in whatever format you like. We are
> not forcing you.
>
> If you really want to do it in the kernel, you can do it out of tree
> and maintain these patches (that's what "out of tree" actually means),
> this is not a direction the BPF maintainers are interested in as it
> does not meet the broader community's use-cases. We don’t want an
> unnecessary extension to the UAPI when some BPF programs do have
> stable instructions already (e.g. network) and some that can
> potentially have someday.
>

Yes, you are forcing us. Saying we are only allowed to use "trusted"
loaders, and that no one is allowed to have any in-kernel, in-tree code
that inspects user inputs or target programs directly is very
non-consentual on my end. This is a design mandate, being forced upon
other people, by you, with no concrete reasons, other than vague statements
around UAPI design, need or necessity.

-blaise

> RE The struct internals will be replaced by calling BPF_OBJ_GET_INFO
> directly from the loader program as I mentioned in the commit.”
>
>
> - KP
>
>
>>
>> -blaise
>>
>> >  void bpf_gen__record_attach_target(struct bpf_gen *gen, const char *attach_name,
>> >                                  enum bpf_attach_type type)
>> >  {
>> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>> > index b6ee9870523a..084372fa54f4 100644
>> > --- a/tools/lib/bpf/libbpf.h
>> > +++ b/tools/lib/bpf/libbpf.h
>> > @@ -1803,9 +1803,10 @@ struct gen_loader_opts {
>> >       const char *insns;
>> >       __u32 data_sz;
>> >       __u32 insns_sz;
>> > +     bool gen_hash;
>> >  };
>> >
>> > -#define gen_loader_opts__last_field insns_sz
>> > +#define gen_loader_opts__last_field gen_hash
>> >  LIBBPF_API int bpf_object__gen_loader(struct bpf_object *obj,
>> >                                     struct gen_loader_opts *opts);
>> >
>> > --
>> > 2.43.0