Re: [PATCH bpf-next v4 4/7] bpf: Attachment verification for BPF_MODIFY_RETURN
From: Stephen Smalley <stephen.smalley.work@gmail.com>
Date: 2020-03-05 13:42:00
Also in:
bpf, lkml
On Wed, Mar 4, 2020 at 2:20 PM KP Singh [off-list ref] wrote:
quoted hunk ↗ jump to hunk
From: KP Singh <redacted> - 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@chromium.org/ (local) - 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 <redacted> Acked-by: Andrii Nakryiko <redacted> ---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).