> From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com]
> Sent: Thursday, March 31, 2022 4:27 AM
> 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.

Hi Alexei

thanks for taking the time to provide such detailed
explanation.

> 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.

I reiterate the goal: enforce a mandatory policy with
an out-of-tree LSM (a kernel module is fine), with the
same guarantees of LSMs integrated in the security
subsystem.

The root user is not part of the TCB (i.e. is untrusted),
all the changes that user wants to make must be subject
of decision by the LSM enforcing the mandatory policy.

I thought about adding support for LSMs from kernel
modules via a new built-in LSM (called LoadLSM), but
to me it looks that the bpf LSM is closer to achieve the
same goal. And in addition, eBPF significantly simplifies
with its helpers writing an LSM.

> 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?

static int populate_bpffs(struct dentry *parent)
{
        struct bpf_preload_info objs[BPF_PRELOAD_LINKS] = {};

#define BPF_PRELOAD_LINKS 2

> > - only links can be pinned, other object types are not supported;
> 
> really? progs, maps can be pinned as well.

struct bpf_preload_info {
        char link_name[16];
        struct bpf_link *link;
};

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

I meant if you want to extend the bpf_preload kernel
module.

> > 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.

Ok. How can the goal I stated above be achieved properly?

> 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.

root can stop the LSM without consulting the security
policy. The goal of having root untrusted is not achieved.

Maybe there is another way to prevent unloading
the kernel module. I didn't find it yet. If there was an
LSM hook called when kernel modules are unloaded,
that would be sufficient, I guess.

My point was that pinning progs seems to be the
recommended way of keeping them running. Pinning
them to unreachable inodes intuitively looked the
way to go for achieving the stated goal. Or maybe I
should just increment the reference count of links
and don't decrement during an rmmod?

If there is something I'm missing, please let me know.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua

On Thu, Mar 31, 2022 at 08:25:22AM +0000, Roberto Sassu wrote:
> > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com]
> > Sent: Thursday, March 31, 2022 4:27 AM
> > 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.
> 
> Hi Alexei
> 
> thanks for taking the time to provide such detailed
> explanation.
> 
> > 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.
> 
> I reiterate the goal: enforce a mandatory policy with
> an out-of-tree LSM (a kernel module is fine), with the
> same guarantees of LSMs integrated in the security
> subsystem.

To make it 100% clear:
Any in-kernel feature that benefits out-of-tree module will be rejected.

> The root user is not part of the TCB (i.e. is untrusted),
> all the changes that user wants to make must be subject
> of decision by the LSM enforcing the mandatory policy.
> 
> I thought about adding support for LSMs from kernel
> modules via a new built-in LSM (called LoadLSM), but

Such approach will be rejected. See above.

> > 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.
> 
> root can stop the LSM without consulting the security
> policy. The goal of having root untrusted is not achieved.

Out-of-tree module can do any hack.
For example:
1. don't do detach skel in __fini
  rmmod will remove the module, but bpf progs will keep running.
2. do module_get(THIS_MODULE) in __init
  rmmod will return EBUSY
  and have some out-of-band way of dropping mod refcnt.
3. hack into sys_delete_module. if module_name==diglem return EBUSY.
4. add proper LSM hook to delete_module

> My point was that pinning progs seems to be the
> recommended way of keeping them running. 

Not quite. bpf_link refcnt is what keeps progs attached.
bpffs is mainly used for:
- to pass maps/links from one process to another
when passing fd is not possible.
- to solve the case of crashing user space.
The user space agent will restart and will pick up where
it's left by reading map, link, prog FDs from bpffs.
- pinning bpf iterators that are later used to 'cat' such files.
That is what bpf_preload is doing by creating two debug
files "maps.debug" and "progs.debug".

> Pinning
> them to unreachable inodes intuitively looked the
> way to go for achieving the stated goal. 

We can consider inodes in bpffs that are not unlinkable by root
in the future, but certainly not for this use case.

> Or maybe I
> should just increment the reference count of links
> and don't decrement during an rmmod?

I suggest to abandon out-of-tree goal.
Only then we can help and continue this discussion.

On Sat, Apr 2, 2022 at 1:55 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Mar 31, 2022 at 08:25:22AM +0000, Roberto Sassu wrote:
> > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com]
> > > Sent: Thursday, March 31, 2022 4:27 AM
> > > 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.
> >
> > Hi Alexei
> >
> > thanks for taking the time to provide such detailed
> > explanation.
> >
> > > 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.

+1 these are not security issues. They are limitations of your MAC policy.

> >
> > I reiterate the goal: enforce a mandatory policy with
> > an out-of-tree LSM (a kernel module is fine), with the
> > same guarantees of LSMs integrated in the security
> > subsystem.
>
> To make it 100% clear:
> Any in-kernel feature that benefits out-of-tree module will be rejected.
>
> > The root user is not part of the TCB (i.e. is untrusted),
> > all the changes that user wants to make must be subject
> > of decision by the LSM enforcing the mandatory policy.
> >
> > I thought about adding support for LSMs from kernel
> > modules via a new built-in LSM (called LoadLSM), but

Kernel modules cannot implement LSMs, this has already been
proposed on the lists and has been rejected.

>
> Such approach will be rejected. See above.
>
> > > 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.
> >
> > root can stop the LSM without consulting the security
> > policy. The goal of having root untrusted is not achieved.

Ofcourse, this is an issue, if you are using BPF to define a MAC
policy, the policy
needs to be comprehensive to prevent itself from being overridden. This is why
We have so many LSM hooks. If you think some are missing, let's add them.

This is why implementing a policy is not trivial, but we need to allow
users to build
such policies with the help from the kernel and not by using
out-of-tree modules.

I do think we can add some more helpers (e.g. for modifying xattrs
from BPF) that
would help us build complex policies.

>
> Out-of-tree module can do any hack.
> For example:
> 1. don't do detach skel in __fini
>   rmmod will remove the module, but bpf progs will keep running.
> 2. do module_get(THIS_MODULE) in __init
>   rmmod will return EBUSY
>   and have some out-of-band way of dropping mod refcnt.
> 3. hack into sys_delete_module. if module_name==diglem return EBUSY.
> 4. add proper LSM hook to delete_module

+1 I recommend this (but not from an out of tree module)

>
> > My point was that pinning progs seems to be the
> > recommended way of keeping them running.
>
> Not quite. bpf_link refcnt is what keeps progs attached.
> bpffs is mainly used for:
> - to pass maps/links from one process to another
> when passing fd is not possible.
> - to solve the case of crashing user space.
> The user space agent will restart and will pick up where
> it's left by reading map, link, prog FDs from bpffs.
> - pinning bpf iterators that are later used to 'cat' such files.
> That is what bpf_preload is doing by creating two debug
> files "maps.debug" and "progs.debug".
>
> > Pinning
> > them to unreachable inodes intuitively looked the
> > way to go for achieving the stated goal.
>
> We can consider inodes in bpffs that are not unlinkable by root
> in the future, but certainly not for this use case.

Can this not be already done by adding a BPF_LSM program to the
inode_unlink LSM hook?

>
> > Or maybe I
> > should just increment the reference count of links
> > and don't decrement during an rmmod?
>
> I suggest to abandon out-of-tree goal.
> Only then we can help and continue this discussion.

+1

On Sun, Apr 3, 2022 at 5:42 PM KP Singh <kpsingh@kernel.org> wrote:
>
> On Sat, Apr 2, 2022 at 1:55 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
...
> >
> > > Pinning
> > > them to unreachable inodes intuitively looked the
> > > way to go for achieving the stated goal.
> >
> > We can consider inodes in bpffs that are not unlinkable by root
> > in the future, but certainly not for this use case.
>
> Can this not be already done by adding a BPF_LSM program to the
> inode_unlink LSM hook?
>

Also, beside of the inode_unlink... and out of curiosity: making sysfs/bpffs/
readonly after pinning, then using bpf LSM hooks sb_mount|remount|unmount...
family combining bpf() LSM hook... isn't this enough to:
1. Restrict who can pin to bpffs without using a full MAC
2. Restrict who can delete or unmount bpf filesystem

?

-- 
https://djalal.opendz.org/

> From: Djalal Harouni [mailto:tixxdz@gmail.com]
> Sent: Monday, April 4, 2022 9:45 AM
> On Sun, Apr 3, 2022 at 5:42 PM KP Singh <kpsingh@kernel.org> wrote:
> >
> > On Sat, Apr 2, 2022 at 1:55 AM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> ...
> > >
> > > > Pinning
> > > > them to unreachable inodes intuitively looked the
> > > > way to go for achieving the stated goal.
> > >
> > > We can consider inodes in bpffs that are not unlinkable by root
> > > in the future, but certainly not for this use case.
> >
> > Can this not be already done by adding a BPF_LSM program to the
> > inode_unlink LSM hook?
> >
> 
> Also, beside of the inode_unlink... and out of curiosity: making sysfs/bpffs/
> readonly after pinning, then using bpf LSM hooks
> sb_mount|remount|unmount...
> family combining bpf() LSM hook... isn't this enough to:
> 1. Restrict who can pin to bpffs without using a full MAC
> 2. Restrict who can delete or unmount bpf filesystem
> 
> ?

I'm thinking to implement something like this.

First, I add a new program flag called
BPF_F_STOP_ONCONFIRM, which causes the ref count
of the link to increase twice at creation time. In this way,
user space cannot make the link disappear, unless a
confirmation is explicitly sent via the bpf() system call.

Another advantage is that other LSMs can decide
whether or not they allow a program with this flag
(in the bpf security hook).

This would work regardless of the method used to
load the eBPF program (user space or kernel space).

Second, I extend the bpf() system call with a new
subcommand, BPF_LINK_CONFIRM_STOP, which
decreases the ref count for the link of the programs
with the BPF_F_STOP_ONCONFIRM flag. I will also
introduce a new security hook (something like
security_link_confirm_stop), so that an LSM has the
opportunity to deny the stop (the bpf security hook
would not be sufficient to determine exactly for
which link the confirmation is given, an LSM should
be able to deny the stop for its own programs).

What do you think?

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua

On Mon, Apr 4, 2022 at 10:21 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> > From: Djalal Harouni [mailto:tixxdz@gmail.com]
> > Sent: Monday, April 4, 2022 9:45 AM
> > On Sun, Apr 3, 2022 at 5:42 PM KP Singh <kpsingh@kernel.org> wrote:
> > >
> > > On Sat, Apr 2, 2022 at 1:55 AM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > ...
> > > >
> > > > > Pinning
> > > > > them to unreachable inodes intuitively looked the
> > > > > way to go for achieving the stated goal.
> > > >
> > > > We can consider inodes in bpffs that are not unlinkable by root
> > > > in the future, but certainly not for this use case.
> > >
> > > Can this not be already done by adding a BPF_LSM program to the
> > > inode_unlink LSM hook?
> > >
> >
> > Also, beside of the inode_unlink... and out of curiosity: making sysfs/bpffs/
> > readonly after pinning, then using bpf LSM hooks
> > sb_mount|remount|unmount...
> > family combining bpf() LSM hook... isn't this enough to:
> > 1. Restrict who can pin to bpffs without using a full MAC
> > 2. Restrict who can delete or unmount bpf filesystem
> >
> > ?
>
> I'm thinking to implement something like this.
>
> First, I add a new program flag called
> BPF_F_STOP_ONCONFIRM, which causes the ref count
> of the link to increase twice at creation time. In this way,
> user space cannot make the link disappear, unless a
> confirmation is explicitly sent via the bpf() system call.
>
> Another advantage is that other LSMs can decide
> whether or not they allow a program with this flag
> (in the bpf security hook).
>
> This would work regardless of the method used to
> load the eBPF program (user space or kernel space).
>
> Second, I extend the bpf() system call with a new
> subcommand, BPF_LINK_CONFIRM_STOP, which
> decreases the ref count for the link of the programs
> with the BPF_F_STOP_ONCONFIRM flag. I will also
> introduce a new security hook (something like
> security_link_confirm_stop), so that an LSM has the
> opportunity to deny the stop (the bpf security hook
> would not be sufficient to determine exactly for
> which link the confirmation is given, an LSM should
> be able to deny the stop for its own programs).
>
> What do you think?

Hack upon a hack? Makes no sense.

On Tue, Apr 5, 2022 at 12:49 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Apr 4, 2022 at 10:21 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
> >
> > > From: Djalal Harouni [mailto:tixxdz@gmail.com]
> > > Sent: Monday, April 4, 2022 9:45 AM
> > > On Sun, Apr 3, 2022 at 5:42 PM KP Singh <kpsingh@kernel.org> wrote:
> > > >
> > > > On Sat, Apr 2, 2022 at 1:55 AM Alexei Starovoitov
> > > > <alexei.starovoitov@gmail.com> wrote:
> > > ...
> > > > >
> > > > > > Pinning
> > > > > > them to unreachable inodes intuitively looked the
> > > > > > way to go for achieving the stated goal.
> > > > >
> > > > > We can consider inodes in bpffs that are not unlinkable by root
> > > > > in the future, but certainly not for this use case.
> > > >
> > > > Can this not be already done by adding a BPF_LSM program to the
> > > > inode_unlink LSM hook?
> > > >
> > >
> > > Also, beside of the inode_unlink... and out of curiosity: making sysfs/bpffs/
> > > readonly after pinning, then using bpf LSM hooks
> > > sb_mount|remount|unmount...
> > > family combining bpf() LSM hook... isn't this enough to:
> > > 1. Restrict who can pin to bpffs without using a full MAC
> > > 2. Restrict who can delete or unmount bpf filesystem
> > >

I like this approach better, you will have to restrict the BPF, if you
want to implement MAC policy using BPF.

Can you please try implementing something using these hooks?

> > > ?
> >
> > I'm thinking to implement something like this.
> >
> > First, I add a new program flag called
> > BPF_F_STOP_ONCONFIRM, which causes the ref count
> > of the link to increase twice at creation time. In this way,
> > user space cannot make the link disappear, unless a
> > confirmation is explicitly sent via the bpf() system call.

I don't like this approach, this just sounds like an intentional
dangling reference, prone to refcounting errors and it does not
really solve the purpose you want to achieve.

And you will still need a policy around the BPF syscall,
so why not just use the LSM hooks as suggested above?

> >
> > Another advantage is that other LSMs can decide
> > whether or not they allow a program with this flag
> > (in the bpf security hook).
> >
> > This would work regardless of the method used to
> > load the eBPF program (user space or kernel space).
> >
> > Second, I extend the bpf() system call with a new
> > subcommand, BPF_LINK_CONFIRM_STOP, which
> > decreases the ref count for the link of the programs
> > with the BPF_F_STOP_ONCONFIRM flag. I will also
> > introduce a new security hook (something like
> > security_link_confirm_stop), so that an LSM has the
> > opportunity to deny the stop (the bpf security hook
> > would not be sufficient to determine exactly for
> > which link the confirmation is given, an LSM should
> > be able to deny the stop for its own programs).
> >
> > What do you think?
>
> Hack upon a hack? Makes no sense.

Introduce a new LSM to protect pinned objects in a bpf filesystem
instance. This is useful for example to ensure that an LSM will always
enforce its policy, even despite root tries to unload the corresponding
eBPF program.

Achieve the protection by denying inode unlink and unmount of the
protected bpf filesystem instance. Since protected inodes hold a
reference of the link of loaded programs (e.g. LSM hooks), denying
operations on them will prevent the ref count of the links from reaching
zero, ensuring that the programs remain always active.

Enable the protection only for the instance created by the user space
counterpart of the LSM, and don't interfere with other instances, so
that their behavior remains unchanged.

Suggested-by: Djalal Harouni <tixxdz@gmail.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 .gitignore       |  4 +++
 Makefile         | 18 ++++++++++++++
 bpffs_lsm_kern.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
 bpffs_lsm_user.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 145 insertions(+)
 create mode 100644 .gitignore
 create mode 100644 Makefile
 create mode 100644 bpffs_lsm_kern.c
 create mode 100644 bpffs_lsm_user.c

diff --git a/.gitignore b/.gitignore
new file mode 100644
index 000000000000..7fa02964f1dc
--- /dev/null
+++ b/.gitignore
@@ -0,0 +1,4 @@
+*.o
+vmlinux.h
+bpffs_lsm_kern.skel.h
+bpffs_lsm_user
diff --git a/Makefile b/Makefile
new file mode 100644
index 000000000000..c3d805759db3
--- /dev/null
+++ b/Makefile
@@ -0,0 +1,18 @@
+# SPDX-License-Identifier: GPL-2.0
+all: bpffs_lsm_user
+
+clean:
+	rm -rf bpffs_lsm.skel.h vmlinux.h bpffs_lsm_kern.o bpffs_lsm_user
+
+vmlinux.h:
+	/usr/sbin/bpftool btf dump file /sys/kernel/btf/vmlinux format c > \
+			  vmlinux.h
+
+bpffs_lsm_kern.skel.h: bpffs_lsm_kern.o
+	bpftool gen skeleton $< > $@
+
+bpffs_lsm_kern.o: bpffs_lsm_kern.c vmlinux.h
+	clang -Wall -Werror -g -O2 -target bpf -c $< -o $@
+
+bpffs_lsm_user: bpffs_lsm_user.c bpffs_lsm_kern.skel.h bpffs_lsm_kern.o
+	cc -Wall -Werror -g -o $@ $< -lbpf
diff --git a/bpffs_lsm_kern.c b/bpffs_lsm_kern.c
new file mode 100644
index 000000000000..b3ccb2a75c95
--- /dev/null
+++ b/bpffs_lsm_kern.c
@@ -0,0 +1,63 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH
+ *
+ * Authors:
+ * Roberto Sassu <roberto.sassu@huawei.com>
+ *
+ * Implement an LSM to protect a bpf filesystem instance.
+ */
+
+#include "vmlinux.h"
+#include <errno.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_core_read.h>
+
+char _license[] SEC("license") = "GPL";
+
+uint32_t monitored_pid = 0;
+
+struct {
+	__uint(type, BPF_MAP_TYPE_INODE_STORAGE);
+	__uint(map_flags, BPF_F_NO_PREALLOC);
+	__type(key, int);
+	__type(value, sizeof(uint8_t));
+} inode_storage_map SEC(".maps");
+
+SEC("lsm/sb_set_mnt_opts")
+int BPF_PROG(sb_set_mnt_opts, struct super_block *sb, void *mnt_opts,
+	     unsigned long kern_flags, unsigned long *set_kern_flags)
+{
+	u32 pid;
+
+	pid = bpf_get_current_pid_tgid() >> 32;
+	if (pid != monitored_pid)
+		return 0;
+
+	if (!bpf_inode_storage_get(&inode_storage_map, sb->s_root->d_inode, 0,
+				   BPF_LOCAL_STORAGE_GET_F_CREATE))
+		return -EPERM;
+
+	return 0;
+}
+
+SEC("lsm/inode_unlink")
+int BPF_PROG(inode_unlink, struct inode *dir, struct dentry *dentry)
+{
+	if (bpf_inode_storage_get(&inode_storage_map,
+				  dir->i_sb->s_root->d_inode, 0, 0))
+		return -EPERM;
+
+	return 0;
+}
+
+SEC("lsm/sb_umount")
+int BPF_PROG(sb_umount, struct vfsmount *mnt, int flags)
+{
+	if (bpf_inode_storage_get(&inode_storage_map,
+				  mnt->mnt_sb->s_root->d_inode, 0, 0))
+		return -EPERM;
+
+	return 0;
+}
diff --git a/bpffs_lsm_user.c b/bpffs_lsm_user.c
new file mode 100644
index 000000000000..e20180cc5db9
--- /dev/null
+++ b/bpffs_lsm_user.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH
+ *
+ * Author: Roberto Sassu <roberto.sassu@huawei.com>
+ *
+ * Implement the user space side of the LSM for bpffs.
+ */
+
+#include <fcntl.h>
+#include <unistd.h>
+#include <stdio.h>
+#include <errno.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <limits.h>
+#include <sys/mount.h>
+#include <sys/stat.h>
+
+#include "bpffs_lsm_kern.skel.h"
+
+#define MOUNT_FLAGS (MS_NOSUID | MS_NODEV | MS_NOEXEC | MS_RELATIME)
+
+int main(int argc, char *argv[])
+{
+	char mntpoint[] = "/tmp/bpf_private_mountXXXXXX";
+	char path[PATH_MAX];
+	struct bpffs_lsm_kern *skel;
+	int ret, i;
+
+	skel = bpffs_lsm_kern__open_and_load();
+	if (!skel)
+		return -EINVAL;
+
+	ret = bpffs_lsm_kern__attach(skel);
+	if (ret < 0)
+		goto out_destroy;
+
+	mkdtemp(mntpoint);
+
+	skel->bss->monitored_pid = getpid();
+	ret = mount(mntpoint, mntpoint, "bpf", MOUNT_FLAGS, NULL);
+	skel->bss->monitored_pid = 0;
+
+	if (ret < 0)
+		goto out_destroy;
+
+	for (i = 0; i < skel->skeleton->prog_cnt; i++) {
+		snprintf(path, sizeof(path), "%s/%s", mntpoint,
+			 skel->skeleton->progs[i].name);
+		ret = bpf_link__pin(*skel->skeleton->progs[i].link, path);
+		if (ret < 0)
+			goto out_destroy;
+	}
+
+	ret = 0;
+out_destroy:
+	bpffs_lsm_kern__destroy(skel);
+	return ret;
+}
-- 
2.32.0

On 4/5/2022 6:11 AM, Roberto Sassu wrote:
> Introduce a new LSM to protect pinned objects in a bpf filesystem

This is *not an LSM*. Do not call it an LSM. It is a set of
eBPF programs. We have all the opportunities for confusion
that we need. I suggested that you call this a BPF security
module (BSM) earlier today. You have any number of things
you can call this that won't be objectionable.

> instance. This is useful for example to ensure that an LSM will always
> enforce its policy, even despite root tries to unload the corresponding
> eBPF program.

How is this going to ensure that SELinux enforces its policy?
AppArmor has no eBPF program that corresponds to its policy,
neither does any other existing LSM, save BPF. Your claim is
nonsensical in the face of LSM behavior.

> Achieve the protection by denying inode unlink and unmount of the
> protected bpf filesystem instance. Since protected inodes hold a
> reference of the link of loaded programs (e.g. LSM hooks), denying
> operations on them will prevent the ref count of the links from reaching
> zero, ensuring that the programs remain always active.
>
> Enable the protection only for the instance created by the user space
> counterpart of the LSM, and don't interfere with other instances, so
> that their behavior remains unchanged.
>
> Suggested-by: Djalal Harouni <tixxdz@gmail.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>   .gitignore       |  4 +++
>   Makefile         | 18 ++++++++++++++
>   bpffs_lsm_kern.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
>   bpffs_lsm_user.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 145 insertions(+)
>   create mode 100644 .gitignore
>   create mode 100644 Makefile
>   create mode 100644 bpffs_lsm_kern.c
>   create mode 100644 bpffs_lsm_user.c
>
> diff --git a/.gitignore b/.gitignore
> new file mode 100644
> index 000000000000..7fa02964f1dc
> --- /dev/null
> +++ b/.gitignore
> @@ -0,0 +1,4 @@
> +*.o
> +vmlinux.h
> +bpffs_lsm_kern.skel.h
> +bpffs_lsm_user
> diff --git a/Makefile b/Makefile
> new file mode 100644
> index 000000000000..c3d805759db3
> --- /dev/null
> +++ b/Makefile
> @@ -0,0 +1,18 @@
> +# SPDX-License-Identifier: GPL-2.0
> +all: bpffs_lsm_user
> +
> +clean:
> +	rm -rf bpffs_lsm.skel.h vmlinux.h bpffs_lsm_kern.o bpffs_lsm_user
> +
> +vmlinux.h:
> +	/usr/sbin/bpftool btf dump file /sys/kernel/btf/vmlinux format c > \
> +			  vmlinux.h
> +
> +bpffs_lsm_kern.skel.h: bpffs_lsm_kern.o
> +	bpftool gen skeleton $< > $@
> +
> +bpffs_lsm_kern.o: bpffs_lsm_kern.c vmlinux.h
> +	clang -Wall -Werror -g -O2 -target bpf -c $< -o $@
> +
> +bpffs_lsm_user: bpffs_lsm_user.c bpffs_lsm_kern.skel.h bpffs_lsm_kern.o
> +	cc -Wall -Werror -g -o $@ $< -lbpf
> diff --git a/bpffs_lsm_kern.c b/bpffs_lsm_kern.c
> new file mode 100644
> index 000000000000..b3ccb2a75c95
> --- /dev/null
> +++ b/bpffs_lsm_kern.c
> @@ -0,0 +1,63 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH
> + *
> + * Authors:
> + * Roberto Sassu <roberto.sassu@huawei.com>
> + *
> + * Implement an LSM to protect a bpf filesystem instance.
> + */
> +
> +#include "vmlinux.h"
> +#include <errno.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +#include <bpf/bpf_core_read.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +uint32_t monitored_pid = 0;
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_INODE_STORAGE);
> +	__uint(map_flags, BPF_F_NO_PREALLOC);
> +	__type(key, int);
> +	__type(value, sizeof(uint8_t));
> +} inode_storage_map SEC(".maps");
> +
> +SEC("lsm/sb_set_mnt_opts")
> +int BPF_PROG(sb_set_mnt_opts, struct super_block *sb, void *mnt_opts,
> +	     unsigned long kern_flags, unsigned long *set_kern_flags)
> +{
> +	u32 pid;
> +
> +	pid = bpf_get_current_pid_tgid() >> 32;
> +	if (pid != monitored_pid)
> +		return 0;
> +
> +	if (!bpf_inode_storage_get(&inode_storage_map, sb->s_root->d_inode, 0,
> +				   BPF_LOCAL_STORAGE_GET_F_CREATE))
> +		return -EPERM;
> +
> +	return 0;
> +}
> +
> +SEC("lsm/inode_unlink")
> +int BPF_PROG(inode_unlink, struct inode *dir, struct dentry *dentry)
> +{
> +	if (bpf_inode_storage_get(&inode_storage_map,
> +				  dir->i_sb->s_root->d_inode, 0, 0))
> +		return -EPERM;
> +
> +	return 0;
> +}
> +
> +SEC("lsm/sb_umount")
> +int BPF_PROG(sb_umount, struct vfsmount *mnt, int flags)
> +{
> +	if (bpf_inode_storage_get(&inode_storage_map,
> +				  mnt->mnt_sb->s_root->d_inode, 0, 0))
> +		return -EPERM;
> +
> +	return 0;
> +}
> diff --git a/bpffs_lsm_user.c b/bpffs_lsm_user.c
> new file mode 100644
> index 000000000000..e20180cc5db9
> --- /dev/null
> +++ b/bpffs_lsm_user.c
> @@ -0,0 +1,60 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH
> + *
> + * Author: Roberto Sassu <roberto.sassu@huawei.com>
> + *
> + * Implement the user space side of the LSM for bpffs.
> + */
> +
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <stdio.h>
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <limits.h>
> +#include <sys/mount.h>
> +#include <sys/stat.h>
> +
> +#include "bpffs_lsm_kern.skel.h"
> +
> +#define MOUNT_FLAGS (MS_NOSUID | MS_NODEV | MS_NOEXEC | MS_RELATIME)
> +
> +int main(int argc, char *argv[])
> +{
> +	char mntpoint[] = "/tmp/bpf_private_mountXXXXXX";
> +	char path[PATH_MAX];
> +	struct bpffs_lsm_kern *skel;
> +	int ret, i;
> +
> +	skel = bpffs_lsm_kern__open_and_load();
> +	if (!skel)
> +		return -EINVAL;
> +
> +	ret = bpffs_lsm_kern__attach(skel);
> +	if (ret < 0)
> +		goto out_destroy;
> +
> +	mkdtemp(mntpoint);
> +
> +	skel->bss->monitored_pid = getpid();
> +	ret = mount(mntpoint, mntpoint, "bpf", MOUNT_FLAGS, NULL);
> +	skel->bss->monitored_pid = 0;
> +
> +	if (ret < 0)
> +		goto out_destroy;
> +
> +	for (i = 0; i < skel->skeleton->prog_cnt; i++) {
> +		snprintf(path, sizeof(path), "%s/%s", mntpoint,
> +			 skel->skeleton->progs[i].name);
> +		ret = bpf_link__pin(*skel->skeleton->progs[i].link, path);
> +		if (ret < 0)
> +			goto out_destroy;
> +	}
> +
> +	ret = 0;
> +out_destroy:
> +	bpffs_lsm_kern__destroy(skel);
> +	return ret;
> +}

> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
> Sent: Wednesday, April 6, 2022 12:48 AM
> On 4/5/2022 6:11 AM, Roberto Sassu wrote:
> > Introduce a new LSM to protect pinned objects in a bpf filesystem
> 
> This is *not an LSM*. Do not call it an LSM. It is a set of
> eBPF programs. We have all the opportunities for confusion
> that we need. I suggested that you call this a BPF security
> module (BSM) earlier today. You have any number of things
> you can call this that won't be objectionable.
> 
> > instance. This is useful for example to ensure that an LSM will always
> > enforce its policy, even despite root tries to unload the corresponding
> > eBPF program.
> 
> How is this going to ensure that SELinux enforces its policy?

I should have said above: that an LSM implemented with eBPF.
Built-in LSMs are not affected by this change.

Ok, next time I call it BSM.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua

> AppArmor has no eBPF program that corresponds to its policy,
> neither does any other existing LSM, save BPF. Your claim is
> nonsensical in the face of LSM behavior.
> 
> > Achieve the protection by denying inode unlink and unmount of the
> > protected bpf filesystem instance. Since protected inodes hold a
> > reference of the link of loaded programs (e.g. LSM hooks), denying
> > operations on them will prevent the ref count of the links from reaching
> > zero, ensuring that the programs remain always active.
> >
> > Enable the protection only for the instance created by the user space
> > counterpart of the LSM, and don't interfere with other instances, so
> > that their behavior remains unchanged.
> >
> > Suggested-by: Djalal Harouni <tixxdz@gmail.com>
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >   .gitignore       |  4 +++
> >   Makefile         | 18 ++++++++++++++
> >   bpffs_lsm_kern.c | 63
> ++++++++++++++++++++++++++++++++++++++++++++++++
> >   bpffs_lsm_user.c | 60
> +++++++++++++++++++++++++++++++++++++++++++++
> >   4 files changed, 145 insertions(+)
> >   create mode 100644 .gitignore
> >   create mode 100644 Makefile
> >   create mode 100644 bpffs_lsm_kern.c
> >   create mode 100644 bpffs_lsm_user.c
> >
> > diff --git a/.gitignore b/.gitignore
> > new file mode 100644
> > index 000000000000..7fa02964f1dc
> > --- /dev/null
> > +++ b/.gitignore
> > @@ -0,0 +1,4 @@
> > +*.o
> > +vmlinux.h
> > +bpffs_lsm_kern.skel.h
> > +bpffs_lsm_user
> > diff --git a/Makefile b/Makefile
> > new file mode 100644
> > index 000000000000..c3d805759db3
> > --- /dev/null
> > +++ b/Makefile
> > @@ -0,0 +1,18 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +all: bpffs_lsm_user
> > +
> > +clean:
> > +	rm -rf bpffs_lsm.skel.h vmlinux.h bpffs_lsm_kern.o bpffs_lsm_user
> > +
> > +vmlinux.h:
> > +	/usr/sbin/bpftool btf dump file /sys/kernel/btf/vmlinux format c > \
> > +			  vmlinux.h
> > +
> > +bpffs_lsm_kern.skel.h: bpffs_lsm_kern.o
> > +	bpftool gen skeleton $< > $@
> > +
> > +bpffs_lsm_kern.o: bpffs_lsm_kern.c vmlinux.h
> > +	clang -Wall -Werror -g -O2 -target bpf -c $< -o $@
> > +
> > +bpffs_lsm_user: bpffs_lsm_user.c bpffs_lsm_kern.skel.h
> bpffs_lsm_kern.o
> > +	cc -Wall -Werror -g -o $@ $< -lbpf
> > diff --git a/bpffs_lsm_kern.c b/bpffs_lsm_kern.c
> > new file mode 100644
> > index 000000000000..b3ccb2a75c95
> > --- /dev/null
> > +++ b/bpffs_lsm_kern.c
> > @@ -0,0 +1,63 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Authors:
> > + * Roberto Sassu <roberto.sassu@huawei.com>
> > + *
> > + * Implement an LSM to protect a bpf filesystem instance.
> > + */
> > +
> > +#include "vmlinux.h"
> > +#include <errno.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +#include <bpf/bpf_core_read.h>
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +uint32_t monitored_pid = 0;
> > +
> > +struct {
> > +	__uint(type, BPF_MAP_TYPE_INODE_STORAGE);
> > +	__uint(map_flags, BPF_F_NO_PREALLOC);
> > +	__type(key, int);
> > +	__type(value, sizeof(uint8_t));
> > +} inode_storage_map SEC(".maps");
> > +
> > +SEC("lsm/sb_set_mnt_opts")
> > +int BPF_PROG(sb_set_mnt_opts, struct super_block *sb, void
> *mnt_opts,
> > +	     unsigned long kern_flags, unsigned long *set_kern_flags)
> > +{
> > +	u32 pid;
> > +
> > +	pid = bpf_get_current_pid_tgid() >> 32;
> > +	if (pid != monitored_pid)
> > +		return 0;
> > +
> > +	if (!bpf_inode_storage_get(&inode_storage_map, sb->s_root-
> >d_inode, 0,
> > +				   BPF_LOCAL_STORAGE_GET_F_CREATE))
> > +		return -EPERM;
> > +
> > +	return 0;
> > +}
> > +
> > +SEC("lsm/inode_unlink")
> > +int BPF_PROG(inode_unlink, struct inode *dir, struct dentry *dentry)
> > +{
> > +	if (bpf_inode_storage_get(&inode_storage_map,
> > +				  dir->i_sb->s_root->d_inode, 0, 0))
> > +		return -EPERM;
> > +
> > +	return 0;
> > +}
> > +
> > +SEC("lsm/sb_umount")
> > +int BPF_PROG(sb_umount, struct vfsmount *mnt, int flags)
> > +{
> > +	if (bpf_inode_storage_get(&inode_storage_map,
> > +				  mnt->mnt_sb->s_root->d_inode, 0, 0))
> > +		return -EPERM;
> > +
> > +	return 0;
> > +}
> > diff --git a/bpffs_lsm_user.c b/bpffs_lsm_user.c
> > new file mode 100644
> > index 000000000000..e20180cc5db9
> > --- /dev/null
> > +++ b/bpffs_lsm_user.c
> > @@ -0,0 +1,60 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Author: Roberto Sassu <roberto.sassu@huawei.com>
> > + *
> > + * Implement the user space side of the LSM for bpffs.
> > + */
> > +
> > +#include <fcntl.h>
> > +#include <unistd.h>
> > +#include <stdio.h>
> > +#include <errno.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +#include <limits.h>
> > +#include <sys/mount.h>
> > +#include <sys/stat.h>
> > +
> > +#include "bpffs_lsm_kern.skel.h"
> > +
> > +#define MOUNT_FLAGS (MS_NOSUID | MS_NODEV | MS_NOEXEC |
> MS_RELATIME)
> > +
> > +int main(int argc, char *argv[])
> > +{
> > +	char mntpoint[] = "/tmp/bpf_private_mountXXXXXX";
> > +	char path[PATH_MAX];
> > +	struct bpffs_lsm_kern *skel;
> > +	int ret, i;
> > +
> > +	skel = bpffs_lsm_kern__open_and_load();
> > +	if (!skel)
> > +		return -EINVAL;
> > +
> > +	ret = bpffs_lsm_kern__attach(skel);
> > +	if (ret < 0)
> > +		goto out_destroy;
> > +
> > +	mkdtemp(mntpoint);
> > +
> > +	skel->bss->monitored_pid = getpid();
> > +	ret = mount(mntpoint, mntpoint, "bpf", MOUNT_FLAGS, NULL);
> > +	skel->bss->monitored_pid = 0;
> > +
> > +	if (ret < 0)
> > +		goto out_destroy;
> > +
> > +	for (i = 0; i < skel->skeleton->prog_cnt; i++) {
> > +		snprintf(path, sizeof(path), "%s/%s", mntpoint,
> > +			 skel->skeleton->progs[i].name);
> > +		ret = bpf_link__pin(*skel->skeleton->progs[i].link, path);
> > +		if (ret < 0)
> > +			goto out_destroy;
> > +	}
> > +
> > +	ret = 0;
> > +out_destroy:
> > +	bpffs_lsm_kern__destroy(skel);
> > +	return ret;
> > +}

On 4/4/2022 10:20 AM, Roberto Sassu wrote:
>> From: Djalal Harouni [mailto:tixxdz@gmail.com]
>> Sent: Monday, April 4, 2022 9:45 AM
>> On Sun, Apr 3, 2022 at 5:42 PM KP Singh <kpsingh@kernel.org> wrote:
>>> On Sat, Apr 2, 2022 at 1:55 AM Alexei Starovoitov
>>> <alexei.starovoitov@gmail.com> wrote:
>> ...
>>>>> Pinning
>>>>> them to unreachable inodes intuitively looked the
>>>>> way to go for achieving the stated goal.
>>>> We can consider inodes in bpffs that are not unlinkable by root
>>>> in the future, but certainly not for this use case.
>>> Can this not be already done by adding a BPF_LSM program to the
>>> inode_unlink LSM hook?
>>>
>> Also, beside of the inode_unlink... and out of curiosity: making sysfs/bpffs/
>> readonly after pinning, then using bpf LSM hooks
>> sb_mount|remount|unmount...
>> family combining bpf() LSM hook... isn't this enough to:
>> 1. Restrict who can pin to bpffs without using a full MAC
>> 2. Restrict who can delete or unmount bpf filesystem
>>
>> ?
> I'm thinking to implement something like this.
>
> First, I add a new program flag called
> BPF_F_STOP_ONCONFIRM, which causes the ref count
> of the link to increase twice at creation time. In this way,
> user space cannot make the link disappear, unless a
> confirmation is explicitly sent via the bpf() system call.
>
> Another advantage is that other LSMs can decide
> whether or not they allow a program with this flag
> (in the bpf security hook).
>
> This would work regardless of the method used to
> load the eBPF program (user space or kernel space).
>
> Second, I extend the bpf() system call with a new
> subcommand, BPF_LINK_CONFIRM_STOP, which
> decreasres the ref count for the link of the programs
> with the BPF_F_STOP_ONCONFIRM flag. I will also
> introduce a new security hook (something like
> security_link_confirm_stop), so that an LSM has the
> opportunity to deny the stop (the bpf security hook
> would not be sufficient to determine exactly for
> which link the confirmation is given, an LSM should
> be able to deny the stop for its own programs).

Would you please stop referring to a set of eBPF programs
loaded into the BPF LSM as an LSM? Call it a BPF security
module (BSM) if you must use an abbreviation. An LSM is a
provider of security_ hooks. In your case that is BPF. When
you call the set of eBPF programs an LSM it is like calling
an SELinux policy an LSM.

>
> What do you think?
>
> Thanks
>
> Roberto
>
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Li Peng, Zhong Ronghua

> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
> Sent: Tuesday, April 5, 2022 4:50 PM
> On 4/4/2022 10:20 AM, Roberto Sassu wrote:
> >> From: Djalal Harouni [mailto:tixxdz@gmail.com]
> >> Sent: Monday, April 4, 2022 9:45 AM
> >> On Sun, Apr 3, 2022 at 5:42 PM KP Singh <kpsingh@kernel.org> wrote:
> >>> On Sat, Apr 2, 2022 at 1:55 AM Alexei Starovoitov
> >>> <alexei.starovoitov@gmail.com> wrote:
> >> ...
> >>>>> Pinning
> >>>>> them to unreachable inodes intuitively looked the
> >>>>> way to go for achieving the stated goal.
> >>>> We can consider inodes in bpffs that are not unlinkable by root
> >>>> in the future, but certainly not for this use case.
> >>> Can this not be already done by adding a BPF_LSM program to the
> >>> inode_unlink LSM hook?
> >>>
> >> Also, beside of the inode_unlink... and out of curiosity: making
> sysfs/bpffs/
> >> readonly after pinning, then using bpf LSM hooks
> >> sb_mount|remount|unmount...
> >> family combining bpf() LSM hook... isn't this enough to:
> >> 1. Restrict who can pin to bpffs without using a full MAC
> >> 2. Restrict who can delete or unmount bpf filesystem
> >>
> >> ?
> > I'm thinking to implement something like this.
> >
> > First, I add a new program flag called
> > BPF_F_STOP_ONCONFIRM, which causes the ref count
> > of the link to increase twice at creation time. In this way,
> > user space cannot make the link disappear, unless a
> > confirmation is explicitly sent via the bpf() system call.
> >
> > Another advantage is that other LSMs can decide
> > whether or not they allow a program with this flag
> > (in the bpf security hook).
> >
> > This would work regardless of the method used to
> > load the eBPF program (user space or kernel space).
> >
> > Second, I extend the bpf() system call with a new
> > subcommand, BPF_LINK_CONFIRM_STOP, which
> > decreasres the ref count for the link of the programs
> > with the BPF_F_STOP_ONCONFIRM flag. I will also
> > introduce a new security hook (something like
> > security_link_confirm_stop), so that an LSM has the
> > opportunity to deny the stop (the bpf security hook
> > would not be sufficient to determine exactly for
> > which link the confirmation is given, an LSM should
> > be able to deny the stop for its own programs).
> 
> Would you please stop referring to a set of eBPF programs
> loaded into the BPF LSM as an LSM? Call it a BPF security
> module (BSM) if you must use an abbreviation. An LSM is a
> provider of security_ hooks. In your case that is BPF. When
> you call the set of eBPF programs an LSM it is like calling
> an SELinux policy an LSM.

An eBPF program could be a provider of security_ hooks
too. The bpf LSM is an aggregator, similarly to your
infrastructure to manage built-in LSMs. Maybe, calling
it second-level LSM or secondary LSM would better
represent this new class.

The only differences are the registration method, (SEC
directive instead of DEFINE_LSM), and what the hook
implementation can access.

The implementation of a security_ hook via eBPF can
follow the same structure of built-in LSMs, i.e. it can be
uniquely responsible for enforcing and be policy-agnostic,
and can retrieve the decisions based on a policy from a
component implemented somewhere else.

Hopefully, I understood the basic principles correctly.
I let the eBPF maintainers comment on this.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua

> > What do you think?
> >
> > Thanks
> >
> > Roberto
> >
> > HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> > Managing Director: Li Peng, Zhong Ronghua

On 4/5/2022 8:29 AM, Roberto Sassu wrote:
>> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
>> Sent: Tuesday, April 5, 2022 4:50 PM
>> On 4/4/2022 10:20 AM, Roberto Sassu wrote:
>>>> From: Djalal Harouni [mailto:tixxdz@gmail.com]
>>>> Sent: Monday, April 4, 2022 9:45 AM
>>>> On Sun, Apr 3, 2022 at 5:42 PM KP Singh <kpsingh@kernel.org> wrote:
>>>>> On Sat, Apr 2, 2022 at 1:55 AM Alexei Starovoitov
>>>>> <alexei.starovoitov@gmail.com> wrote:
>>>> ...
>>>>>>> Pinning
>>>>>>> them to unreachable inodes intuitively looked the
>>>>>>> way to go for achieving the stated goal.
>>>>>> We can consider inodes in bpffs that are not unlinkable by root
>>>>>> in the future, but certainly not for this use case.
>>>>> Can this not be already done by adding a BPF_LSM program to the
>>>>> inode_unlink LSM hook?
>>>>>
>>>> Also, beside of the inode_unlink... and out of curiosity: making
>> sysfs/bpffs/
>>>> readonly after pinning, then using bpf LSM hooks
>>>> sb_mount|remount|unmount...
>>>> family combining bpf() LSM hook... isn't this enough to:
>>>> 1. Restrict who can pin to bpffs without using a full MAC
>>>> 2. Restrict who can delete or unmount bpf filesystem
>>>>
>>>> ?
>>> I'm thinking to implement something like this.
>>>
>>> First, I add a new program flag called
>>> BPF_F_STOP_ONCONFIRM, which causes the ref count
>>> of the link to increase twice at creation time. In this way,
>>> user space cannot make the link disappear, unless a
>>> confirmation is explicitly sent via the bpf() system call.
>>>
>>> Another advantage is that other LSMs can decide
>>> whether or not they allow a program with this flag
>>> (in the bpf security hook).
>>>
>>> This would work regardless of the method used to
>>> load the eBPF program (user space or kernel space).
>>>
>>> Second, I extend the bpf() system call with a new
>>> subcommand, BPF_LINK_CONFIRM_STOP, which
>>> decreasres the ref count for the link of the programs
>>> with the BPF_F_STOP_ONCONFIRM flag. I will also
>>> introduce a new security hook (something like
>>> security_link_confirm_stop), so that an LSM has the
>>> opportunity to deny the stop (the bpf security hook
>>> would not be sufficient to determine exactly for
>>> which link the confirmation is given, an LSM should
>>> be able to deny the stop for its own programs).
>> Would you please stop referring to a set of eBPF programs
>> loaded into the BPF LSM as an LSM? Call it a BPF security
>> module (BSM) if you must use an abbreviation. An LSM is a
>> provider of security_ hooks. In your case that is BPF. When
>> you call the set of eBPF programs an LSM it is like calling
>> an SELinux policy an LSM.
> An eBPF program could be a provider of security_ hooks
> too.

No, it can't. If I look in /sys/kernel/security/lsm what
you see is "bpf". The LSM is BPF. What BPF does in its
hooks is up to it and its responsibility.

>   The bpf LSM is an aggregator, similarly to your
> infrastructure to manage built-in LSMs. Maybe, calling
> it second-level LSM or secondary LSM would better
> represent this new class.

It isn't an LSM, and adding a qualifier doesn't make it
one and only adds to the confusion.

> The only differences are the registration method, (SEC
> directive instead of DEFINE_LSM), and what the hook
> implementation can access.

Those two things pretty well define what an LSM is.

> The implementation of a security_ hook via eBPF can
> follow the same structure of built-in LSMs, i.e. it can be
> uniquely responsible for enforcing and be policy-agnostic,
> and can retrieve the decisions based on a policy from a
> component implemented somewhere else.

The BPF LSM provides mechanism. The eBPF programs provide policy.

>
> Hopefully, I understood the basic principles correctly.
> I let the eBPF maintainers comment on this.
>
> Thanks
>
> Roberto
>
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Li Peng, Zhong Ronghua
>
>>> What do you think?
>>>
>>> Thanks
>>>
>>> Roberto
>>>
>>> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
>>> Managing Director: Li Peng, Zhong Ronghua

On Tue, Apr 5, 2022 at 6:22 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 4/5/2022 8:29 AM, Roberto Sassu wrote:
> >> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
> >> Sent: Tuesday, April 5, 2022 4:50 PM
> >> On 4/4/2022 10:20 AM, Roberto Sassu wrote:
> >>>> From: Djalal Harouni [mailto:tixxdz@gmail.com]
> >>>> Sent: Monday, April 4, 2022 9:45 AM
> >>>> On Sun, Apr 3, 2022 at 5:42 PM KP Singh <kpsingh@kernel.org> wrote:
> >>>>> On Sat, Apr 2, 2022 at 1:55 AM Alexei Starovoitov
> >>>>> <alexei.starovoitov@gmail.com> wrote:
> >>>> ...
> >>>>>>> Pinning
> >>>>>>> them to unreachable inodes intuitively looked the
> >>>>>>> way to go for achieving the stated goal.
> >>>>>> We can consider inodes in bpffs that are not unlinkable by root
> >>>>>> in the future, but certainly not for this use case.
> >>>>> Can this not be already done by adding a BPF_LSM program to the
> >>>>> inode_unlink LSM hook?
> >>>>>
> >>>> Also, beside of the inode_unlink... and out of curiosity: making
> >> sysfs/bpffs/
> >>>> readonly after pinning, then using bpf LSM hooks
> >>>> sb_mount|remount|unmount...
> >>>> family combining bpf() LSM hook... isn't this enough to:
> >>>> 1. Restrict who can pin to bpffs without using a full MAC
> >>>> 2. Restrict who can delete or unmount bpf filesystem
> >>>>
> >>>> ?
> >>> I'm thinking to implement something like this.
> >>>
> >>> First, I add a new program flag called
> >>> BPF_F_STOP_ONCONFIRM, which causes the ref count
> >>> of the link to increase twice at creation time. In this way,
> >>> user space cannot make the link disappear, unless a
> >>> confirmation is explicitly sent via the bpf() system call.
> >>>
> >>> Another advantage is that other LSMs can decide
> >>> whether or not they allow a program with this flag
> >>> (in the bpf security hook).
> >>>
> >>> This would work regardless of the method used to
> >>> load the eBPF program (user space or kernel space).
> >>>
> >>> Second, I extend the bpf() system call with a new
> >>> subcommand, BPF_LINK_CONFIRM_STOP, which
> >>> decreasres the ref count for the link of the programs
> >>> with the BPF_F_STOP_ONCONFIRM flag. I will also
> >>> introduce a new security hook (something like
> >>> security_link_confirm_stop), so that an LSM has the
> >>> opportunity to deny the stop (the bpf security hook
> >>> would not be sufficient to determine exactly for
> >>> which link the confirmation is given, an LSM should
> >>> be able to deny the stop for its own programs).
> >> Would you please stop referring to a set of eBPF programs
> >> loaded into the BPF LSM as an LSM? Call it a BPF security
> >> module (BSM) if you must use an abbreviation. An LSM is a
> >> provider of security_ hooks. In your case that is BPF. When
> >> you call the set of eBPF programs an LSM it is like calling
> >> an SELinux policy an LSM.
> > An eBPF program could be a provider of security_ hooks
> > too.
>
> No, it can't. If I look in /sys/kernel/security/lsm what
> you see is "bpf". The LSM is BPF. What BPF does in its
> hooks is up to it and its responsibility.
>
> >   The bpf LSM is an aggregator, similarly to your
> > infrastructure to manage built-in LSMs. Maybe, calling
> > it second-level LSM or secondary LSM would better
> > represent this new class.
>
> It isn't an LSM, and adding a qualifier doesn't make it
> one and only adds to the confusion.
>
> > The only differences are the registration method, (SEC
> > directive instead of DEFINE_LSM), and what the hook
> > implementation can access.
>
> Those two things pretty well define what an LSM is.
>
> > The implementation of a security_ hook via eBPF can
> > follow the same structure of built-in LSMs, i.e. it can be
> > uniquely responsible for enforcing and be policy-agnostic,
> > and can retrieve the decisions based on a policy from a
> > component implemented somewhere else.
>
> The BPF LSM provides mechanism. The eBPF programs provide policy.

Yeah, let's stick what we call an LSM in the kernel, Here,
"bpf" is the LSM like selinux,apparmor and this is what you set in
CONFIG_LSM or pass on cmdline as lsm= and can be seen
in /sys/kernel/security/lsm

Calling your BPF programs an LSM will just confuse people.

>
> >
> > Hopefully, I understood the basic principles correctly.
> > I let the eBPF maintainers comment on this.
> >
> > Thanks
> >
> > Roberto
> >
> > HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> > Managing Director: Li Peng, Zhong Ronghua
> >
> >>> What do you think?
> >>>
> >>> Thanks
> >>>
> >>> Roberto
> >>>
> >>> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> >>> Managing Director: Li Peng, Zhong Ronghua

> From: KP Singh [mailto:kpsingh@kernel.org]
> Sent: Saturday, April 2, 2022 3:03 AM
> On Sat, Apr 2, 2022 at 1:55 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Thu, Mar 31, 2022 at 08:25:22AM +0000, Roberto Sassu wrote:
> > > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com]
> > > > Sent: Thursday, March 31, 2022 4:27 AM
> > > > 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.
> > >
> > > Hi Alexei
> > >
> > > thanks for taking the time to provide such detailed
> > > explanation.
> > >
> > > > 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.
> 
> +1 these are not security issues. They are limitations of your MAC policy.
> 
> > >
> > > I reiterate the goal: enforce a mandatory policy with
> > > an out-of-tree LSM (a kernel module is fine), with the
> > > same guarantees of LSMs integrated in the security
> > > subsystem.
> >
> > To make it 100% clear:
> > Any in-kernel feature that benefits out-of-tree module will be rejected.
> >
> > > The root user is not part of the TCB (i.e. is untrusted),
> > > all the changes that user wants to make must be subject
> > > of decision by the LSM enforcing the mandatory policy.
> > >
> > > I thought about adding support for LSMs from kernel
> > > modules via a new built-in LSM (called LoadLSM), but
> 
> Kernel modules cannot implement LSMs, this has already been
> proposed on the lists and has been rejected.

Looking at commit cb80ddc67152 ("bpf: Convert bpf_preload.ko
to use light skeleton."), I got that it is the most efficient way
to load an eBPF program (does not even require libbpf).

Another advantage was that we get integrity verification
from the module infrastructure. This would have been the
optimal solution in terms of dependencies. Enforcing
integrity could be turned on with the module.sig_enforce
kernel option.

If we switch to user space, the choice would be IMA.
However, in my use case (DIGLIM) it would be used just
for the purpose of doing integrity verifications pre-init.

Thinking which policy could be implemented for such purpose,
maybe something like appraise every process that is not linked
to an executable? And since there are no xattrs in the initial
ram disk, could I append a module signature to an ELF binary?

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua

> > Such approach will be rejected. See above.
> >
> > > > 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.
> > >
> > > root can stop the LSM without consulting the security
> > > policy. The goal of having root untrusted is not achieved.
> 
> Ofcourse, this is an issue, if you are using BPF to define a MAC
> policy, the policy
> needs to be comprehensive to prevent itself from being overridden. This is
> why
> We have so many LSM hooks. If you think some are missing, let's add them.
> 
> This is why implementing a policy is not trivial, but we need to allow
> users to build
> such policies with the help from the kernel and not by using
> out-of-tree modules.
> 
> I do think we can add some more helpers (e.g. for modifying xattrs
> from BPF) that
> would help us build complex policies.
> 
> >
> > Out-of-tree module can do any hack.
> > For example:
> > 1. don't do detach skel in __fini
> >   rmmod will remove the module, but bpf progs will keep running.
> > 2. do module_get(THIS_MODULE) in __init
> >   rmmod will return EBUSY
> >   and have some out-of-band way of dropping mod refcnt.
> > 3. hack into sys_delete_module. if module_name==diglem return EBUSY.
> > 4. add proper LSM hook to delete_module
> 
> +1 I recommend this (but not from an out of tree module)
> 
> >
> > > My point was that pinning progs seems to be the
> > > recommended way of keeping them running.
> >
> > Not quite. bpf_link refcnt is what keeps progs attached.
> > bpffs is mainly used for:
> > - to pass maps/links from one process to another
> > when passing fd is not possible.
> > - to solve the case of crashing user space.
> > The user space agent will restart and will pick up where
> > it's left by reading map, link, prog FDs from bpffs.
> > - pinning bpf iterators that are later used to 'cat' such files.
> > That is what bpf_preload is doing by creating two debug
> > files "maps.debug" and "progs.debug".
> >
> > > Pinning
> > > them to unreachable inodes intuitively looked the
> > > way to go for achieving the stated goal.
> >
> > We can consider inodes in bpffs that are not unlinkable by root
> > in the future, but certainly not for this use case.
> 
> Can this not be already done by adding a BPF_LSM program to the
> inode_unlink LSM hook?
> 
> >
> > > Or maybe I
> > > should just increment the reference count of links
> > > and don't decrement during an rmmod?
> >
> > I suggest to abandon out-of-tree goal.
> > Only then we can help and continue this discussion.
> 
> +1