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. We would want an actual security hook called here so we can implement a specific check over userspace being able to attach BPF progs to LSM hooks. CAP_MAC_ADMIN has other connotations to SELinux (presently the ability to set/get file security labels that are not known to the currently loaded policy).
