Re: [PATCH bpf-next v8 06/11] bpf: allow writing to a subset of sock fields from lsm progtype

[Martin KaFai Lau <kafai@fb.com>]

On Wed, Jun 01, 2022 at 12:02:13PM -0700, Stanislav Fomichev wrote:
> For now, allow only the obvious ones, like sk_priority and sk_mark.
> 
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>  kernel/bpf/bpf_lsm.c  | 58 +++++++++++++++++++++++++++++++++++++++++++
>  kernel/bpf/verifier.c |  3 ++-
>  2 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index 83aa431dd52e..feba8e96f58d 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
> @@ -303,7 +303,65 @@ bool bpf_lsm_is_sleepable_hook(u32 btf_id)
>  const struct bpf_prog_ops lsm_prog_ops = {
>  };
>  
> +static int lsm_btf_struct_access(struct bpf_verifier_log *log,
> +					const struct btf *btf,
> +					const struct btf_type *t, int off,
> +					int size, enum bpf_access_type atype,
> +					u32 *next_btf_id,
> +					enum bpf_type_flag *flag)
> +{
> +	const struct btf_type *sock_type;
> +	struct btf *btf_vmlinux;
> +	s32 type_id;
> +	size_t end;
> +
> +	if (atype == BPF_READ)
> +		return btf_struct_access(log, btf, t, off, size, atype, next_btf_id,
> +					 flag);
> +
> +	btf_vmlinux = bpf_get_btf_vmlinux();
> +	if (!btf_vmlinux) {
> +		bpf_log(log, "no vmlinux btf\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	type_id = btf_find_by_name_kind(btf_vmlinux, "sock", BTF_KIND_STRUCT);
> +	if (type_id < 0) {
> +		bpf_log(log, "'struct sock' not found in vmlinux btf\n");
> +		return -EINVAL;
> +	}
> +
> +	sock_type = btf_type_by_id(btf_vmlinux, type_id);
> +
> +	if (t != sock_type) {
> +		bpf_log(log, "only 'struct sock' writes are supported\n");
> +		return -EACCES;
> +	}
> +
> +	switch (off) {
> +	case bpf_ctx_range(struct sock, sk_priority):
This looks wrong.  It should not allow to write at
any bytes of the '__u32 sk_priority'.

> +		end = offsetofend(struct sock, sk_priority);
> +		break;
> +	case bpf_ctx_range(struct sock, sk_mark):
Same here.

Just came to my mind,
if the current need is only sk_priority and sk_mark,
do you think allowing bpf_setsockopt will be more useful instead ?
It currently has SO_MARK, SO_PRIORITY, and other options.
Also, changing SO_MARK requires to clear the sk->sk_dst_cache.
In general, is it safe to do bpf_setsockopt in all bpf_lsm hooks ?

> +		end = offsetofend(struct sock, sk_mark);
> +		break;
> +	default:
> +		bpf_log(log, "no write support to 'struct sock' at off %d\n", off);
> +		return -EACCES;
> +	}
> +
> +	if (off + size > end) {
> +		bpf_log(log,
> +			"write access at off %d with size %d beyond the member of 'struct sock' ended at %zu\n",
> +			off, size, end);
> +		return -EACCES;
> +	}
> +
> +	return NOT_INIT;
> +}
> +
>  const struct bpf_verifier_ops lsm_verifier_ops = {
>  	.get_func_proto = bpf_lsm_func_proto,
>  	.is_valid_access = btf_ctx_access,
> +	.btf_struct_access = lsm_btf_struct_access,
>  };
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index caa5740b39b3..c54e171d9c23 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -13413,7 +13413,8 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
>  				insn->code = BPF_LDX | BPF_PROBE_MEM |
>  					BPF_SIZE((insn)->code);
>  				env->prog->aux->num_exentries++;
> -			} else if (resolve_prog_type(env->prog) != BPF_PROG_TYPE_STRUCT_OPS) {
> +			} else if (resolve_prog_type(env->prog) != BPF_PROG_TYPE_STRUCT_OPS &&
> +				   resolve_prog_type(env->prog) != BPF_PROG_TYPE_LSM) {
>  				verbose(env, "Writes through BTF pointers are not allowed\n");
>  				return -EINVAL;
>  			}
> -- 
> 2.36.1.255.ge46751e96f-goog
>