Message-ID: <f739928b1db9a9e45da89249c0389e85.paul@paul-moore.com>
Date: Fri, 13 Oct 2023 17:15:36 -0400
From: Paul Moore <paul@...l-moore.com>
To: Andrii Nakryiko <andrii@...nel.org>, <bpf@...r.kernel.org>, <netdev@...r.kernel.org>
Cc: <linux-fsdevel@...r.kernel.org>, <linux-security-module@...r.kernel.org>, <keescook@...omium.org>, <brauner@...nel.org>, <lennart@...ttering.net>, <kernel-team@...a.com>, <sargun@...gun.me>
Subject: Re: [PATCH v7 6/18] bpf: add BPF token support to BPF_PROG_LOAD  command

On Oct 12, 2023 Andrii Nakryiko <andrii@...nel.org> wrote:
> 
> 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@...nel.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(-)

...

> 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.

> +	}
> +
> +	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.

> +	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;
>  
>  	/* Intent here is for unprivileged_bpf_disabled to block BPF program
>  	 * creation for unprivileged users; other actions depend
> @@ -2618,21 +2641,23 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
>  	 * capability checks are still carried out for these
>  	 * and other operations.
>  	 */
> -	if (sysctl_unprivileged_bpf_disabled && !bpf_capable())
> -		return -EPERM;
> +	if (sysctl_unprivileged_bpf_disabled && !bpf_cap)
> +		goto put_token;
>  
>  	if (attr->insn_cnt == 0 ||
> -	    attr->insn_cnt > (bpf_capable() ? BPF_COMPLEXITY_LIMIT_INSNS : BPF_MAXINSNS))
> -		return -E2BIG;
> +	    attr->insn_cnt > (bpf_cap ? BPF_COMPLEXITY_LIMIT_INSNS : BPF_MAXINSNS)) {
> +		err = -E2BIG;
> +		goto put_token;
> +	}
>  	if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
>  	    type != BPF_PROG_TYPE_CGROUP_SKB &&
> -	    !bpf_capable())
> -		return -EPERM;
> +	    !bpf_cap)
> +		goto put_token;
>  
> -	if (is_net_admin_prog_type(type) && !bpf_net_capable())
> -		return -EPERM;
> -	if (is_perfmon_prog_type(type) && !perfmon_capable())
> -		return -EPERM;
> +	if (is_net_admin_prog_type(type) && !bpf_token_capable(token, CAP_NET_ADMIN))
> +		goto put_token;
> +	if (is_perfmon_prog_type(type) && !bpf_token_capable(token, CAP_PERFMON))
> +		goto put_token;
>  
>  	/* attach_prog_fd/attach_btf_obj_fd can specify fd of either bpf_prog
>  	 * or btf, we need to check which one it is
> @@ -2642,27 +2667,33 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
>  		if (IS_ERR(dst_prog)) {
>  			dst_prog = NULL;
>  			attach_btf = btf_get_by_fd(attr->attach_btf_obj_fd);
> -			if (IS_ERR(attach_btf))
> -				return -EINVAL;
> +			if (IS_ERR(attach_btf)) {
> +				err = -EINVAL;
> +				goto put_token;
> +			}
>  			if (!btf_is_kernel(attach_btf)) {
>  				/* attaching through specifying bpf_prog's BTF
>  				 * objects directly might be supported eventually
>  				 */
>  				btf_put(attach_btf);
> -				return -ENOTSUPP;
> +				err = -ENOTSUPP;
> +				goto put_token;
>  			}
>  		}
>  	} else if (attr->attach_btf_id) {
>  		/* fall back to vmlinux BTF, if BTF type ID is specified */
>  		attach_btf = bpf_get_btf_vmlinux();
> -		if (IS_ERR(attach_btf))
> -			return PTR_ERR(attach_btf);
> -		if (!attach_btf)
> -			return -EINVAL;
> +		if (IS_ERR(attach_btf)) {
> +			err = PTR_ERR(attach_btf);
> +			goto put_token;
> +		}
> +		if (!attach_btf) {
> +			err = -EINVAL;
> +			goto put_token;
> +		}
>  		btf_get(attach_btf);
>  	}
>  
> -	bpf_prog_load_fixup_attach_type(attr);
>  	if (bpf_prog_load_check_attach(type, attr->expected_attach_type,
>  				       attach_btf, attr->attach_btf_id,
>  				       dst_prog)) {
> @@ -2670,7 +2701,8 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
>  			bpf_prog_put(dst_prog);
>  		if (attach_btf)
>  			btf_put(attach_btf);
> -		return -EINVAL;
> +		err = -EINVAL;
> +		goto put_token;
>  	}
>  
>  	/* plain bpf_prog allocation */
> @@ -2680,7 +2712,8 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
>  			bpf_prog_put(dst_prog);
>  		if (attach_btf)
>  			btf_put(attach_btf);
> -		return -ENOMEM;
> +		err = -EINVAL;
> +		goto put_token;
>  	}
>  
>  	prog->expected_attach_type = attr->expected_attach_type;
> @@ -2691,6 +2724,10 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
>  	prog->aux->sleepable = attr->prog_flags & BPF_F_SLEEPABLE;
>  	prog->aux->xdp_has_frags = attr->prog_flags & BPF_F_XDP_HAS_FRAGS;
>  
> +	/* move token into prog->aux, reuse taken refcnt */
> +	prog->aux->token = token;
> +	token = NULL;
> +
>  	err = security_bpf_prog_alloc(prog->aux);
>  	if (err)
>  		goto free_prog;
> @@ -2792,6 +2829,8 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
>  	if (prog->aux->attach_btf)
>  		btf_put(prog->aux->attach_btf);
>  	bpf_prog_free(prog);
> +put_token:
> +	bpf_token_put(token);
>  	return err;
>  }

--
paul-moore.com

Powered by blists - more mailing lists