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@xxxxxx> 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@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'.
>
> 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.
