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 Mon, Jun 06, 2022 at 03:46:29PM -0700, Stanislav Fomichev wrote:
> On Sat, Jun 4, 2022 at 12:18 AM Martin KaFai Lau <kafai@fb.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@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'.
> 
> 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.
semi-related.
bpf_setsockopt will have more options (and more useful) in the future.
I am thinking to refactor the bpf_setsockopt() to avoid duplicate
codes between the syscall setsockopt().  With sockptr_t, that may be
easier to do now.  It will then be easier to allow most of the options
in bpf_setsockopt().

> 
> > 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)
Yeah, it seems a btf id list is needed.  Regardless of bpf_setsockopt()
or not, locking the sk is needed to make changes.