Re: [PATCH v9 11/17] bpf,lsm: add BPF token LSM hooks

On Mon, Nov 6, 2023 at 2:17 PM Andrii Nakryiko
[off-list ref] wrote:

On Sun, Nov 5, 2023 at 9:01 PM Paul Moore [off-list ref] wrote:

quoted

On Nov  3, 2023 Andrii Nakryiko [off-list ref] wrote:

quoted

Wire up bpf_token_create and bpf_token_free LSM hooks, which allow to
allocate LSM security blob (we add `void *security` field to struct
bpf_token for that), but also control who can instantiate BPF token.
This follows existing pattern for BPF map and BPF prog.

Also add security_bpf_token_allow_cmd() and security_bpf_token_capable()
LSM hooks that allow LSM implementation to control and negate (if
necessary) BPF token's delegation of a specific bpf_cmd and capability,
respectively.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 include/linux/bpf.h           |  3 ++
 include/linux/lsm_hook_defs.h |  5 +++
 include/linux/security.h      | 25 +++++++++++++++
 kernel/bpf/bpf_lsm.c          |  4 +++
 kernel/bpf/token.c            | 13 ++++++--
 security/security.c           | 60 +++++++++++++++++++++++++++++++++++
 6 files changed, 107 insertions(+), 3 deletions(-)

...

quoted

diff --git a/include/linux/security.h b/include/linux/security.h
index 08fd777cbe94..1d6edbf45d1c 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h

@@ -60,6 +60,7 @@ struct fs_parameter;
 enum fs_value_type;
 struct watch;
 struct watch_notification;
+enum bpf_cmd;

Yes, I think it's fine to include bpf.h in security.h instead of the
forward declaration.

quoted

 /* Default (no) options for the capable function */
 #define CAP_OPT_NONE 0x0

@@ -2031,6 +2032,11 @@ extern void security_bpf_map_free(struct bpf_map *map);
 extern int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr,
                                struct bpf_token *token);
 extern void security_bpf_prog_free(struct bpf_prog *prog);
+extern int security_bpf_token_create(struct bpf_token *token, union bpf_attr *attr,
+                                  struct path *path);
+extern void security_bpf_token_free(struct bpf_token *token);
+extern int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd);
+extern int security_bpf_token_capable(const struct bpf_token *token, int cap);
 #else
 static inline int security_bpf(int cmd, union bpf_attr *attr,
                                           unsigned int size)

@@ -2065,6 +2071,25 @@ static inline int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *

 static inline void security_bpf_prog_free(struct bpf_prog *prog)
 { }
+
+static inline int security_bpf_token_create(struct bpf_token *token, union bpf_attr *attr,
+                                  struct path *path)
+{
+     return 0;
+}
+
+static inline void security_bpf_token_free(struct bpf_token *token)
+{ }
+
+static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd)
+{
+     return 0;
+}
+
+static inline int security_bpf_token_capable(const struct bpf_token *token, int cap)
+{
+     return 0;
+}

Another nitpick, but I would prefer to shorten
security_bpf_token_allow_cmd() renamed to security_bpf_token_cmd() both
to shorten the name and to better fit convention.  I realize the caller
is named bpf_token_allow_cmd() but I'd still rather see the LSM hook
with the shorter name.

Makes sense, renamed to security_bpf_token_cmd() and updated hook name as well

Thanks.

quoted

quoted

diff --git a/kernel/bpf/token.c b/kernel/bpf/token.c
index 35e6f55c2a41..5d04da54faea 100644
--- a/kernel/bpf/token.c
+++ b/kernel/bpf/token.c

@@ -7,11 +7,12 @@
 #include <linux/idr.h>
 #include <linux/namei.h>
 #include <linux/user_namespace.h>
+#include <linux/security.h>

 bool bpf_token_capable(const struct bpf_token *token, int cap)
 {
      /* BPF token allows ns_capable() level of capabilities */
-     if (token) {
+     if (token && security_bpf_token_capable(token, cap) == 0) {
              if (ns_capable(token->userns, cap))
                      return true;
              if (cap != CAP_SYS_ADMIN && ns_capable(token->userns, CAP_SYS_ADMIN))

We typically perform the capability based access controls prior to the
LSM controls, meaning if we want to the token controls to work in a
similar way we should do something like this:

  bool bpf_token_capable(...)
  {
    if (token) {
      if (ns_capable(token, cap) ||
          (cap != ADMIN && ns_capable(token, ADMIN)))
        return security_bpf_token_capable(token, cap);
    }
    return capable(cap) || (cap != ADMIN && capable(...))
  }

yep, makes sense, I changed it as you suggested above

Thanks again.

quoted

quoted

@@ -28,6 +29,7 @@ void bpf_token_inc(struct bpf_token *token)

 static void bpf_token_free(struct bpf_token *token)
 {
+     security_bpf_token_free(token);
      put_user_ns(token->userns);
      kvfree(token);
 }

@@ -172,6 +174,10 @@ int bpf_token_create(union bpf_attr *attr)
      token->allowed_progs = mnt_opts->delegate_progs;
      token->allowed_attachs = mnt_opts->delegate_attachs;

+     err = security_bpf_token_create(token, attr, &path);
+     if (err)
+             goto out_token;
+
      fd = get_unused_fd_flags(O_CLOEXEC);
      if (fd < 0) {
              err = fd;

@@ -216,8 +222,9 @@ bool bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd)
 {
      if (!token)
              return false;
-
-     return token->allowed_cmds & (1ULL << cmd);
+     if (!(token->allowed_cmds & (1ULL << cmd)))
+             return false;
+     return security_bpf_token_allow_cmd(token, cmd) == 0;

I'm not sure how much it really matters, but someone might prefer
the '!!' approach/style over '== 0'.

it would have to be !security_bpf_token_cmd(), right?

Yeah :P

In most, although definitely not all, kernel functions when something
returns 0 we consider that the positive/success case, with non-zero
values being some sort of failure.  I must have defaulted to that
logic here, but you are correct that just a single negation would be
needed here.

And that single
negation is just very confusing when dealing with int-returning
function. I find it much easier to make sure the logic is correct when
we have explicit `== 0`.

That's fine, it's something I've seen mentioned over the years and
thought I might offer it as a comment.  I can read either approach
just fine :)

Anyway, with the other changes mentioned above, e.g. naming and
permission ordering, feel free to add my ACK.

Acked-by: Paul Moore <paul@paul-moore.com>

-- 
paul-moore.com