Re: [PATCH 00/18] bpf: Secure and authenticated preloading of eBPF programs

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Roberto Sassu <roberto.sassu@huawei.com>
Cc: corbet@lwn.net, viro@zeniv.linux.org.uk, ast@kernel.org,
	daniel@iogearbox.net, andrii@kernel.org, kpsingh@kernel.org,
	shuah@kernel.org, mcoquelin.stm32@gmail.com,
	alexandre.torgue@foss.st.com, zohar@linux.ibm.com,
	linux-doc@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	netdev@vger.kernel.org, bpf@vger.kernel.org,
	linux-kselftest@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org,
	linux-integrity@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 00/18] bpf: Secure and authenticated preloading of eBPF programs
Date: Wed, 30 Mar 2022 19:27:27 -0700	[thread overview]
Message-ID: <20220331022727.ybj4rui4raxmsdpu@MBP-98dd607d3435.dhcp.thefacebook.com> (raw)
In-Reply-To: <20220328175033.2437312-1-roberto.sassu@huawei.com>

On Mon, Mar 28, 2022 at 07:50:15PM +0200, Roberto Sassu wrote:
> eBPF already allows programs to be preloaded and kept running without
> intervention from user space. There is a dedicated kernel module called
> bpf_preload, which contains the light skeleton of the iterators_bpf eBPF
> program. If this module is enabled in the kernel configuration, its loading
> will be triggered when the bpf filesystem is mounted (unless the module is
> built-in), and the links of iterators_bpf are pinned in that filesystem
> (they will appear as the progs.debug and maps.debug files).
> 
> However, the current mechanism, if used to preload an LSM, would not offer
> the same security guarantees of LSMs integrated in the security subsystem.
> Also, it is not generic enough to be used for preloading arbitrary eBPF
> programs, unless the bpf_preload code is heavily modified.
> 
> More specifically, the security problems are:
> - any program can be pinned to the bpf filesystem without limitations
>   (unless a MAC mechanism enforces some restrictions);
> - programs being executed can be terminated at any time by deleting the
>   pinned objects or unmounting the bpf filesystem.

So many things to untangle here.

The above paragraphs are misleading and incorrect.
The commit log sounds like there are security issues that this
patch set is fixing.
This is not true.
Looks like there is a massive misunderstanding on what bpffs is.
It's a file system to pin and get bpf objects with normal
file access permissions. Nothing else.
Do NOT use it to pin LSM or any other security sensitive bpf programs
and then complain that root can unpin them.
Yes. Root can and should be able to 'rm -rf' anything in bpffs instance.

> The usability problems are:
> - only a fixed amount of links can be pinned;

where do you see this limit?

> - only links can be pinned, other object types are not supported;

really? progs, maps can be pinned as well.

> - code to pin objects has to be written manually;

huh?

> Solve the security problems by mounting the bpf filesystem from the kernel,
> by preloading authenticated kernel modules (e.g. with module.sig_enforce)
> and by pinning objects to that filesystem. This particular filesystem
> instance guarantees that desired eBPF programs run until the very end of
> the kernel lifecycle, since even root cannot interfere with it.

No.

I suspect there is huge confusion on what these two "progs.debug"
and "maps.debug" files are in a bpffs instance.
They are debug files to pretty pring loaded maps and progs for folks who
like to use 'cat' to examine the state of the system instead of 'bpftool'.
The root can remove these files from bpffs.

There is no reason for kernel module to pin its bpf progs.
If you want to develop DIGLIM as a kernel module that uses light skeleton
just do:
#include <linux/init.h>
#include <linux/module.h>
#include "diglim.lskel.h"

static struct diglim_bpf *skel;

static int __init load(void)
{
        skel = diglim_bpf__open_and_load();
        err = diglim_bpf__attach(skel);
}
/* detach skel in __fini */

It's really that short.

Then you will be able to
- insmod diglim.ko -> will load and attach bpf progs.
- rmmod diglim -> will detach them.

Independantly from these two mistunderstandings of bpffs and light skel
we've been talking about auto exposing loaded bpf progs, maps, links
in a bpffs without incrementing refcnt of them.
When progs get unloaded the files will disappear.
Some folks believe that doing 'ls' in a directory and see one file
for each bpf prog loaded and then doing 'cat' on that file would be
useful for debugging. That idea wasn't rejected. We're still thinking
what would be the best way to auto-expose all bpf objects for debugging
and whether it's actually makes sense to do considering that
bpftool already has commands to list all progs, maps, links, etc
with great detail.
It's pretty much an argument between 'cat+ls' believers and
'bpftool' cmdline believers.
That discussion is orthogonal and should not be mixed with bpffs, lsm,
security or anything else.