Thread (19 messages) 19 messages, 4 authors, 2020-03-05

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).
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help