Message-ID: <CAKH8qBv-cKrqvYjPh3P1JaWGLCTpBq3JtOEg+Py=a7BN_dVrPw@mail.gmail.com>
Date:   Mon, 6 Jun 2022 15:46:29 -0700
From:   Stanislav Fomichev <sdf@...gle.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org, ast@...nel.org,
        daniel@...earbox.net, andrii@...nel.org
Subject: Re: [PATCH bpf-next v8 06/11] bpf: allow writing to a subset of sock
 fields from lsm progtype

On Sat, Jun 4, 2022 at 12:18 AM Martin KaFai Lau <kafai@...com> wrote:
>
> 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@...gle.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'.

SG, I'll change to offsetof() and will enfoce u32 size.

> > +             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 ?

For my use-case I only need sk_priority, but I was thinking that we
can later extend that list as needed. But you suggestion to use
bpf_setsockopt sounds good, let me try to use that. That might be
better than poking directly into the fields.

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

It seems that we might need to more strictly control it (regardless of
helper or direct field access). Not all lsm hooks lock sk argument, so
we so maybe start with some allowlist of attach_btf_ids that can do
bpf_setsockopt? (I'll add existing hooks that work on new/unreferenced
or locked sockets to the list)


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

Powered by blists - more mailing lists