netdev - Re: [RFC bpf-next 1/4] bpf: cgroup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAADnVQLHHf=WT2iZ3j7ja6nvy4MzzLM9u7=orVSw9SBj-amRXg@mail.gmail.com>
Date:   Thu, 17 Feb 2022 08:58:01 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Stanislav Fomichev <sdf@...gle.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>
Subject: Re: [RFC bpf-next 1/4] bpf: cgroup_sock lsm flavor

On Thu, Feb 17, 2022 at 8:21 AM <sdf@...gle.com> wrote:
> > As far as api the attach should probably be to a cgroup+lsm_hook pair.
> > link_create.target_fd will be cgroup_fd.
> > At prog load time attach_btf_id should probably be one
> > of existing bpf_lsm_* hooks.
> > Feels wrong to duplicate the whole set into lsm_cgroup_sock set.
>
> lsm_cgroup_sock is there to further limit which particular lsm
> hooks BPF_LSM_CGROUP_SOCK can use. I guess I can maybe look at
> BTF's first argument to verify that it's 'struct socket'? Let
> me try to see whether it's a good alternative..

That's a great idea.
We probably would need 2 flavors of __cgroup_bpf_run_lsm_sock wrapper.
One for 'struct socket *' and another for 'struct sock *'.
In lsm hooks they come as the first argument and BTF will tell us what it is.
There are exceptions like socket_create hook
which would have to use current->cgroup.
So ideally we can have a single attach_type BPF_LSM_CGROUP
and use proper wrapper socket/sock/current depending on BTF of the lsm hook.

> > Would it be fast enough?
> > We went through a bunch of optimization for normal cgroup and ended with:
> >          if (cgroup_bpf_enabled(CGROUP_INET_INGRESS) &&
> >              cgroup_bpf_sock_enabled(sk, CGROUP_INET_INGRESS))
> > Here the trampoline code plus call into __cgroup_bpf_run_lsm_sock
> > will be there for all cgroups.
> > Since cgroup specific check will be inside BPF_PROG_RUN_ARRAY_CG.
> > I suspect it's ok, since the link_create will be for few specific lsm
> > hooks
> > which are typically not in the fast path.
> > Unlike traditional cgroup hook like ingress that is hot.
>
> Right, cgroup_bpf_enabled() is not needed because lsm is by definition
> off/unattached by default. Seems like we can add cgroup_bpf_sock_enabled()
> to
> __cgroup_bpf_run_lsm_sock.

I guess we can, but that feels redundant.
If we attach the wrapper to a particular lsm hook then cgroup_bpf_sock
is surely enabled.

>
> > For BPF_LSM_CGROUP_TASK it will take cgroup from current instead of sock,
> > right?
>
> Right. Seems like the only difference is where we get the cgroup pointer
> from: current vs sock->cgroup. Although, I'm a bit unsure whether to
> allow hooks that are clearly sock-cgroup-based to use
> BPF_LSM_CGROUP_TASK. For example, should we allow
> BPF_LSM_CGROUP_TASK to attach to that socket_post_create? I'd prohibit that
> at
> least initially to avoid some subtle 'why sometimes my
> programs trigger on the wrong cgroup' types of issues.

Agree. With a single BPF_LSM_CGROUP attach type will get correct
behavior automatically.

Looking forward to non rfc patches. Exciting feature!