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@xxxxxxxxxx>
> ---
> 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
>
