Re: [PATCH v7 6/18] bpf: add BPF token support to BPF_PROG_LOAD command

On Fri, Oct 13, 2023 at 2:15 PM Paul Moore [off-list ref] wrote:

On Oct 12, 2023 Andrii Nakryiko [off-list ref] wrote:

quoted

Add basic support of BPF token to BPF_PROG_LOAD. Wire through a set of
allowed BPF program types and attach types, derived from BPF FS at BPF
token creation time. Then make sure we perform bpf_token_capable()
checks everywhere where it's relevant.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 include/linux/bpf.h                           |  6 ++
 include/uapi/linux/bpf.h                      |  2 +
 kernel/bpf/core.c                             |  1 +
 kernel/bpf/inode.c                            |  6 +-
 kernel/bpf/syscall.c                          | 87 ++++++++++++++-----
 kernel/bpf/token.c                            | 27 ++++++
 tools/include/uapi/linux/bpf.h                |  2 +
 .../selftests/bpf/prog_tests/libbpf_probes.c  |  2 +
 .../selftests/bpf/prog_tests/libbpf_str.c     |  3 +
 9 files changed, 110 insertions(+), 26 deletions(-)

...

quoted

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index a2c9edcbcd77..c6b00aee3b62 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c

@@ -2584,13 +2584,15 @@ static bool is_perfmon_prog_type(enum bpf_prog_type prog_type)
 }

 /* last field in 'union bpf_attr' used by this command */
-#define      BPF_PROG_LOAD_LAST_FIELD log_true_size
+#define BPF_PROG_LOAD_LAST_FIELD prog_token_fd

 static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
 {
      enum bpf_prog_type type = attr->prog_type;
      struct bpf_prog *prog, *dst_prog = NULL;
      struct btf *attach_btf = NULL;
+     struct bpf_token *token = NULL;
+     bool bpf_cap;
      int err;
      char license[128];

@@ -2606,10 +2608,31 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
                               BPF_F_XDP_DEV_BOUND_ONLY))
              return -EINVAL;

+     bpf_prog_load_fixup_attach_type(attr);
+
+     if (attr->prog_token_fd) {
+             token = bpf_token_get_from_fd(attr->prog_token_fd);
+             if (IS_ERR(token))
+                     return PTR_ERR(token);
+             /* if current token doesn't grant prog loading permissions,
+              * then we can't use this token, so ignore it and rely on
+              * system-wide capabilities checks
+              */
+             if (!bpf_token_allow_cmd(token, BPF_PROG_LOAD) ||
+                 !bpf_token_allow_prog_type(token, attr->prog_type,
+                                            attr->expected_attach_type)) {
+                     bpf_token_put(token);
+                     token = NULL;
+             }

At the start of this effort I mentioned how we wanted to have LSM
control points when the token is created and when it is used.  It is
for this reason that we still want a hook inside the
bpf_token_allow_cmd() function as it allows us to enable/disable use
of the token when its use is first attempted.  If the LSM decides to
disallow use of the token in this particular case then the token is
disabled (set to NULL) while the operation is still allowed to move
forward, simply without the token.  It's a much cleaner and well
behaved approach as it allows the normal BPF access controls to do
their work.

I see, ok, so you want to be able to say "no BPF token for you", but
not just error out the entire operation. Makes sense.

quoted

+     }
+
+     bpf_cap = bpf_token_capable(token, CAP_BPF);

Similar to the above comment, we want to a LSM control point in
bpf_token_capable() so that the LSM can control the token's
ability to delegate capability privileges when they are used.  Having
to delay this access control point to security_bpf_prog_load() is not
only awkward but it requires either manual synchronization between
all of the different LSMs and the the capability checks in the
bpf_prog_load() function or a completely different set of LSM
permissions for a token-based BPF program load over a normal BPF
program load.

We really need these hooks Andrii, I wouldn't have suggested them if
I didn't believe they were important.

No problem, I'll add both of them. I really didn't want to add hooks
for allow_{maps,progs,attachs} (which you agreed shouldn't be added,
so we are good), but I think allow_cmds and capable checks are fine.
Will add in the next revision.

quoted

+     err = -EPERM;
+
      if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) &&
          (attr->prog_flags & BPF_F_ANY_ALIGNMENT) &&
-         !bpf_capable())
-             return -EPERM;
+         !bpf_cap)
+             goto put_token;

[...]