On Thu, Mar 05, 2020 at 08:43:11AM -0500, Stephen Smalley wrote: > On Wed, Mar 4, 2020 at 2:20 PM KP Singh <kpsingh@xxxxxxxxxxxx> wrote: > > > > From: KP Singh <kpsingh@xxxxxxxxxx> > > > > - Allow BPF_MODIFY_RETURN attachment only to functions that are: > > > > * Whitelisted for error injection by checking > > within_error_injection_list. Similar discussions happened for the > > bpf_override_return helper. > > > > * security hooks, this is expected to be cleaned up with the LSM > > changes after the KRSI patches introduce the LSM_HOOK macro: > > > > https://lore.kernel.org/bpf/20200220175250.10795-1-kpsingh@xxxxxxxxxxxx/ > > > > - The attachment is currently limited to functions that return an int. > > This can be extended later other types (e.g. PTR). > > > > Signed-off-by: KP Singh <kpsingh@xxxxxxxxxx> > > Acked-by: Andrii Nakryiko <andriin@xxxxxx> > > --- > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 2460c8e6b5be..ae32517d4ccd 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -9800,6 +9801,33 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env) > > > > return 0; > > } > > +#define SECURITY_PREFIX "security_" > > + > > +static int check_attach_modify_return(struct bpf_verifier_env *env) > > +{ > > + struct bpf_prog *prog = env->prog; > > + unsigned long addr = (unsigned long) prog->aux->trampoline->func.addr; > > + > > + if (within_error_injection_list(addr)) > > + return 0; > > + > > + /* This is expected to be cleaned up in the future with the KRSI effort > > + * introducing the LSM_HOOK macro for cleaning up lsm_hooks.h. > > + */ > > + if (!strncmp(SECURITY_PREFIX, prog->aux->attach_func_name, > > + sizeof(SECURITY_PREFIX) - 1)) { > > + > > + if (!capable(CAP_MAC_ADMIN)) > > + return -EPERM; > > CAP_MAC_ADMIN was originally introduced for Smack and is not > all-powerful wrt SELinux, so this is not a sufficient check for > SELinux. I think you're misunderstanding the intent here. This facility is just a faster version of kprobe based fault injection. It doesn't care about LSM. Security is not a focus here. It can fault inject in a lot of places in the kernel: syscalls, kmalloc, page_alloc, fs internals, etc I think above capable() check created this confusion and we should remove it.
