Re: [PATCH v2 security-next 1/4] security: Hornet LSM

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

James Bottomley <James.Bottomley@HansenPartnership.com> writes:

> On Thu, 2025-04-24 at 16:41 -0700, Alexei Starovoitov wrote:
>> On Wed, Apr 23, 2025 at 7:12 AM James Bottomley
>> <James.Bottomley@hansenpartnership.com> wrote:
>> > On Mon, 2025-04-21 at 13:12 -0700, Alexei Starovoitov wrote:
>> > [...]
>> > > Calling bpf_map_get() and
>> > > map->ops->map_lookup_elem() from a module is not ok either.
>> > 
>> > I don't understand this objection.
>> 
>> Consider an LSM that hooks into security_bprm_*(bprm),
>> parses something in linux_binprm, then
>> struct file *file =
>> fd_file(fdget(some_random_file_descriptor_in_current));
>> file->f_op->read(..);
>> 
>> Would VFS maintainers approve such usage ?
>
> This is a bit off topic from the request for clarification but:
>
> It's somewhat standard operating procedure for LSMs.  Some do make
> decisions entirely within the data provided by the hook, but some need
> to take external readings, like selinux or IMA consulting the policy in
> the xattr or apparmor the one in the tree etc.
>
> Incidentally, none of them directly does a file->f_op->read(); they all
> use the kernel_read_file() API which is exported from the vfs for that
> purpose.
>
>> More so, your LSM does
>> file = get_task_exe_file(current);
>> kernel_read_file(file, ...);
>> 
>> This is even worse.
>> You've corrupted the ELF binary with extra garbage at the end.
>> objdump/elfutils will choke on it and you're lucky that binfmt_elf
>> still loads it.
>> The whole approach is a non-starter.
>
> It's the same approach we use to create kernel modules: ELF with an
> appended signature.  If you recall the kernel summit discussions about
> it, the reason that was chosen for modules is because it's easy and the
> ELF processor simply ignores any data in the file that's not described
> by the header (which means the ELF tools you refer to above are fine
> with this if you actually try them).
>
> But it you really want the signature to be part of the ELF,  then the
> patch set can do what David Howells first suggested for modules: it can
> simply put the appended signature into an unloaded ELF section.
>
>> > The program just got passed in to bpf_prog_load() as a set of
>> > attributes which, for a light skeleton, directly contain the code
>> > as a blob and have the various BTF relocations as a blob in a
>> > single element array map.  I think everyone agrees that the
>> > integrity of the program would be compromised by modifications to
>> > the relocations, so the security_bpf_prog_load() hook can't make an
>> > integrity determination without examining both.  If the hook can't
>> > use the bpf_maps.. APIs directly is there some other API it should
>> > be using to get the relocations, or are you saying that the
>> > security_bpf_prog_load() hook isn't fit for purpose and it should
>> > be called after the bpf core has loaded the relocations so they can
>> > be provided to the hook as an argument?
>> 
>> No. As I said twice already the only place to verify program
>> signature is a bpf subsystem itself.
>
> The above argument is actually independent of signing.  However,
> although we have plenty of subsystems that verify their own signatures,
> it's perfectly valid for a LSM to do it as well: IMA is one of the
> oldest LSMs and it's been verifying signatures over binaries and text
> files since it was first created.
>
>> Hacking into bpf internals from LSM, BPF-LSM program,
>> or any other kernel subsystem is a no go.
>
> All LSMs depend to some extent on the internals of the subsystem where
> the hook is ... the very structures passed into them are often internal
> to that subsystem.  The problem you didn't address was that some of the
> information necessary to determine the integrity properties in the bpf
> hook is in a map file descriptor.  Since the map merely wraps a single
> blob of data, that could easily be passed in to the hook instead of
> having the LSM extract it from the map.  How the hook gets the data is
> an internal implementation detail of the kernel that can be updated
> later.
>
>> > The above, by the way, is independent of signing, because it
>> > applies to any determination that might be made in the
>> > security_bpf_prog_load() hook regardless of purpose.
>> 
>> security_bpf_prog_load() should not access bpf internals.
>> That LSM hook sees the following:
>> security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr,
>>                        struct bpf_token *token, bool kernel);
>> 
>> LSM can look into uapi things there.
>
> Is that the misunderstanding? That's not how LSMs work: they are not
> bound by only the UAPI, they are in kernel and have full access to the
> kernel API so they can introspect stuff and make proper determinations.
>
>> Like prog->sleepable, prog->tag, prog->aux->name,
>> but things like prog->aux->jit_data or prog->aux->used_maps
>> are not ok to access.
>> If in doubt, ask on the mailing list.
>
> I am aren't I? At least the bpf is one of the lists cc'd on this.
>
> Regards,
>
> James

I think we may be in the weeds here a bit and starting to get a little
off-topic. Let's try to back up some and take a different tack. We are
going to rework this effort into a set of patches that target the bpf
subsystem and it's tooling directly, performing optional signature
verification of the inputs to bpf_prog_load, using signature data
passed in via bpf_attr, which should enough provide metadata so that it
can be consumed by interested parties to enforce policy decisions around
code signing and data integrity.

-blaise