netdev - Re: [PATCH bpf-next v6 05/10] bpf: implement BPF_PROG_QUERY for BPF_LSM

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKH8qBsGr+466B23ExBixwcriKxNrFa3v47st-S3qUggNP2Q5A@mail.gmail.com>
Date:   Mon, 9 May 2022 16:38:16 -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 v6 05/10] bpf: implement BPF_PROG_QUERY for BPF_LSM_CGROUP

On Fri, May 6, 2022 at 5:12 PM Martin KaFai Lau <kafai@...com> wrote:
>
> On Fri, Apr 29, 2022 at 02:15:35PM -0700, Stanislav Fomichev wrote:
> > We have two options:
> > 1. Treat all BPF_LSM_CGROUP as the same, regardless of attach_btf_id
> > 2. Treat BPF_LSM_CGROUP+attach_btf_id as a separate hook point
> >
> > I'm doing (2) here and adding attach_btf_id as a new BPF_PROG_QUERY
> > argument. The downside is that it requires iterating over all possible
> > bpf_lsm_ hook points in the userspace which might take some time.
> >
> > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > ---
> >  include/uapi/linux/bpf.h       |  1 +
> >  kernel/bpf/cgroup.c            | 43 ++++++++++++++++++++++++----------
> >  kernel/bpf/syscall.c           |  3 ++-
> >  tools/include/uapi/linux/bpf.h |  1 +
> >  tools/lib/bpf/bpf.c            | 42 ++++++++++++++++++++++++++-------
> >  tools/lib/bpf/bpf.h            | 15 ++++++++++++
> >  tools/lib/bpf/libbpf.map       |  1 +
> >  7 files changed, 85 insertions(+), 21 deletions(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 112e396bbe65..e38ea0b47b6a 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -1431,6 +1431,7 @@ union bpf_attr {
> >               __u32           attach_flags;
> >               __aligned_u64   prog_ids;
> >               __u32           prog_cnt;
> > +             __u32           attach_btf_id;  /* for BPF_LSM_CGROUP */
> If the downside/concern on (1) is the bpftool cannot show
> which bpf_lsm_* hook that a cgroup-lsm is attached to,
> how about adding this attach_btf_id to the bpf_prog_info instead.
> The bpftool side is getting the bpf_prog_info (e.g. for the name) anyway.
>
> Probably need to rename it to attach_func_btf_id (== prog->aux->attach_btf_id)
> and then also add the attach_btf_id (== prog->aux->attach_btf->id) to
> bpf_prog_info.
>
> The bpftool then will work mostly the same and no need to iterate btf_vmlinux
> to figure out the btf_id for all bpf_lsm_* hooks and no need to worry about
> the increasing total number of lsm hooks in the future while
> the latter bpftool patch has a static 1024.
>
> If you also agree on (1), for this patch on the kernel side concern,
> it needs to return all BPF_LSM_CGROUP progs to the userspace.

I was exploring this initially but with this scheme I'm not sure how
to export attach_flags. I'm assuming that one lsm hook can, say, be
attached as BPF_F_ALLOW_OVERRIDE and the other one can use
BPF_F_ALLOW_MULTI. Now, if we return all BPF_LSM_CGROUP programs (1),
we have a problem because there is only one attach_flags field per
attach_type.

I can extend BPF_PROG_QUERY with another user-provided pointer where
the kernel can put per-program attach_flags, doesn't seem like there
would be a problem, right?

> Feel free to put the bpf_prog_info modification and bpftool changes as a follow up
> patch.  In this same set is also fine.  Suggesting it because this set is
> getting long already.

SG. Let's discuss it first. I can do a follow up series to add this
query api, the series is getting long indeed :-(