Message-ID: <87bjsjlxw5.fsf@microsoft.com>
Date: Fri, 25 Apr 2025 14:44:10 -0700
From: Blaise Boscaccy <bboscaccy@...ux.microsoft.com>
To: James Bottomley <James.Bottomley@...senPartnership.com>, Alexei
 Starovoitov <alexei.starovoitov@...il.com>, KP Singh <kpsingh@...gle.com>,
 Paul Moore <paul@...l-moore.com>, Daniel Borkmann <daniel@...earbox.net>
Cc: Jonathan Corbet <corbet@....net>, David Howells <dhowells@...hat.com>,
 Herbert Xu <herbert@...dor.apana.org.au>, "David S. Miller"
 <davem@...emloft.net>, Paul Moore <paul@...l-moore.com>, James Morris
 <jmorris@...ei.org>, "Serge
 E. Hallyn" <serge@...lyn.com>, Masahiro Yamada <masahiroy@...nel.org>,
 Nathan Chancellor <nathan@...nel.org>, Nicolas Schier <nicolas@...sle.eu>,
 Shuah Khan <shuah@...nel.org>, Mickaël Salaün
 <mic@...ikod.net>, Günther
 Noack <gnoack@...gle.com>, Nick Desaulniers
 <nick.desaulniers+lkml@...il.com>, Bill Wendling <morbo@...gle.com>,
 Justin Stitt <justinstitt@...gle.com>, Jarkko Sakkinen
 <jarkko@...nel.org>, Jan Stancek <jstancek@...hat.com>, Neal Gompa
 <neal@...pa.dev>, "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
 LKML <linux-kernel@...r.kernel.org>, keyrings@...r.kernel.org, Linux
 Crypto Mailing List <linux-crypto@...r.kernel.org>, LSM List
 <linux-security-module@...r.kernel.org>, Linux Kbuild mailing list
 <linux-kbuild@...r.kernel.org>, "open list:KERNEL SELFTEST FRAMEWORK"
 <linux-kselftest@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
 clang-built-linux <llvm@...ts.linux.dev>, nkapron@...gle.com, Matteo Croce
 <teknoraver@...a.com>, Roberto Sassu <roberto.sassu@...wei.com>, Cong Wang
 <xiyou.wangcong@...il.com>
Subject: Re: [PATCH v2 security-next 1/4] security: Hornet LSM

James Bottomley <James.Bottomley@...senPartnership.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@...senpartnership.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

Powered by blists - more mailing lists