James Bottomley <james.bottom...@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.bottom...@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
