Message-ID: <6e086e29d258839e42ef7a83b38571d1882eb77d.camel@HansenPartnership.com>
Date: Fri, 25 Apr 2025 10:06:11 -0400
From: James Bottomley <James.Bottomley@...senPartnership.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Blaise Boscaccy <bboscaccy@...ux.microsoft.com>, 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

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

Powered by blists - more mailing lists