Re: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM

[Alexei Starovoitov]

On Tue, Feb 11, 2020 at 01:43:34PM +0100, KP Singh wrote:
> 
> > Pros:
> > - no changes to security/ directory
> 
> * We do need to initialize the BPF LSM as a proper LSM (i.e. in
>   security/bpf) because it needs access to security blobs. This is
>   only possible statically for now as they should be set after boot
>   time to provide guarantees to any helper that uses information in
>   security blobs. I have proposed how we could make this dynamic as a
>   discussion topic for the BPF conference:
> 
>     https://lore.kernel.org/bpf/20200127171516.GA2710@xxxxxxxxxxxx
> 
>   As you can see from some of the prototype use cases e.g:
> 
>     https://github.com/sinkap/linux-krsi/blob/patch/v1/examples/samples/bpf/lsm_detect_exec_unlink.c
> 
>   that they do rely on security blobs and that they are key in doing
>   meaningful security analysis.

above example doesn't use security blob from bpf prog.
Are you referring to
https://github.com/sinkap/linux-krsi/blob/patch/v1/examples/security/bpf/ops.c#L455
Then it's a bpf helper that is using it. And that helper could have been
implemented differently. I think it should be a separate discussion on merits
of such helper, its api, and its implementation.

At the same time I agree that additional scratch space accessible by lsm in
inode->i_security, cred->security and other kernel structs is certainly
necessary, but it's a nack to piggy back on legacy lsm way of doing it. The
implementation of bpf_lsm_blob_sizes.lbs_inode fits one single purpose. It's
fine for builtin LSM where blob sizes and code that uses it lives in one place
in the kernel and being modified at once when need for more space arises. For
bpf progs such approach is a non starter. Progs need to have flexible amount
scratch space. Thankfully this problem is not new. It was solved already.
Please see how bpf_sk_storage is implemented. It's a flexible amount of scratch
spaces available to bpf progs that is available in every socket. It's done on
demand. No space is wasted when progs are not using it. Not all sockets has to
have it either. I strongly believe that the same approach should be used for
scratch space in inode, cred, etc. It can be a union of existing 'void
*security' pointer or a new pointer. net/core/bpf_sk_storage.c implementation
is not socket specific. It can be generalized for inode, cred, task, etc.

> * When using the semantic provided by fexit, the BPF LSM program will
>   always be executed and will be able to override / clobber the
>   decision of LSMs which appear before it in the ordered list. This
>   semantic is very different from what we currently have (i.e. the BPF
>   LSM hook is only called if all the other LSMs allow the action) and
>   seems to be bypassing the LSM framework.

It that's a concern it's trivial to add 'if (RC == 0)' check to fexit
trampoline generator specific to lsm progs.

> * Not all security_* wrappers simply call the attached hooks and return
>   their exit code and not all of them pass the same arguments to the
>   hook e.g. security_bprm_check, security_file_open,
>   security_task_alloc to just name a few. Illustrating this further
>   using security_task_alloc as an example:
> 
> 	rc = call_int_hook(task_alloc, 0, task, clone_flags);
> 	if (unlikely(rc))
> 		security_task_free(task);
> 	return rc;
> 
> Which means we would leak task_structs in this case. While
> call_int_hook is sort of equivalent to the fexit trampoline for most
> hooks, it's not really the case for some (quite important) LSM hooks.

let's look at them one by one.
1.
security_bprm_check() calling ima_bprm_check.
I think it's layering violation.
Was it that hard to convince vfs folks to add it in fs/exec.c where
it belongs?

2.
security_file_open() calling fsnotify_perm().
Same layering violation and it's clearly broken.
When CONFIG_SECURITY is not defined:
static inline int security_file_open(struct file *file)
{
        return 0;
}
There is no call to fsnotify_perm().
So fsnotify_open/mkdir/etc() work fine with and without CONFIG_SECURITY,
but fsnotify_perm() events can be lost depending on kconfig.
fsnotify_perm() should be moved in fs/open.c.

3.
security_task_alloc(). hmm.
when CONFIG_SECURITY is enabled and corresponding LSM with
non zero blob_sizes.lbs_task is registered that hook allocates
memory even if task_alloc is empty.
Doing security_task_free() in that hook also looks wrong.
It should have been:
diff --git a/kernel/fork.c b/kernel/fork.c
index ef82feb4bddc..a0d31e781341 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2062,7 +2062,7 @@ static __latent_entropy struct task_struct *copy_process(
        shm_init_task(p);
        retval = security_task_alloc(p, clone_flags);
        if (retval)
-               goto bad_fork_cleanup_audit;
+               goto bad_fork_cleanup_security;
Same issue with security_file_alloc().

I think this layering issues should be fixed, but it's not a blocker for
lsm-bpf to proceed. Using fexit mechanism and bpf_sk_storage generalization is
all that is needed. None of it should touch security/*.