From: Stanislav Fomichev <sdf@google.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>
Subject: Re: [PATCH bpf-next v7 11/11] selftests/bpf: verify lsm_cgroup struct sock access
Date: Mon, 23 May 2022 19:15:37 -0700 [thread overview]
Message-ID: <CAKH8qBsQH2fwxa6B6LOqfw1ru_qk9wyypXnAzy4u+uBYBmQq8w@mail.gmail.com> (raw)
In-Reply-To: <CAEf4BzaG2bOcyVfGZxcNU1p8i0Xipez7v-789bq8qYDE1Ce-sQ@mail.gmail.com>
On Mon, May 23, 2022 at 4:33 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, May 18, 2022 at 3:56 PM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > sk_priority & sk_mark are writable, the rest is readonly.
> >
> > One interesting thing here is that the verifier doesn't
> > really force me to add NULL checks anywhere :-/
> >
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> > .../selftests/bpf/prog_tests/lsm_cgroup.c | 69 +++++++++++++++++++
> > 1 file changed, 69 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> > index 29292ec40343..64b6830e03f5 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> > @@ -270,8 +270,77 @@ static void test_lsm_cgroup_functional(void)
> > lsm_cgroup__destroy(skel);
> > }
> >
> > +static int field_offset(const char *type, const char *field)
> > +{
> > + const struct btf_member *memb;
> > + const struct btf_type *tp;
> > + const char *name;
> > + struct btf *btf;
> > + int btf_id;
> > + int i;
> > +
> > + btf = btf__load_vmlinux_btf();
> > + if (!btf)
> > + return -1;
> > +
> > + btf_id = btf__find_by_name_kind(btf, type, BTF_KIND_STRUCT);
> > + if (btf_id < 0)
> > + return -1;
> > +
> > + tp = btf__type_by_id(btf, btf_id);
> > + memb = btf_members(tp);
> > +
> > + for (i = 0; i < btf_vlen(tp); i++) {
> > + name = btf__name_by_offset(btf,
> > + memb->name_off);
> > + if (strcmp(field, name) == 0)
> > + return memb->offset / 8;
> > + memb++;
> > + }
> > +
> > + return -1;
> > +}
> > +
> > +static bool sk_writable_field(const char *type, const char *field, int size)
> > +{
> > + LIBBPF_OPTS(bpf_prog_load_opts, opts,
> > + .expected_attach_type = BPF_LSM_CGROUP);
> > + struct bpf_insn insns[] = {
> > + /* r1 = *(u64 *)(r1 + 0) */
> > + BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, 0),
> > + /* r1 = *(u64 *)(r1 + offsetof(struct socket, sk)) */
> > + BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, field_offset("socket", "sk")),
> > + /* r2 = *(u64 *)(r1 + offsetof(struct sock, <field>)) */
> > + BPF_LDX_MEM(size, BPF_REG_2, BPF_REG_1, field_offset(type, field)),
> > + /* *(u64 *)(r1 + offsetof(struct sock, <field>)) = r2 */
> > + BPF_STX_MEM(size, BPF_REG_1, BPF_REG_2, field_offset(type, field)),
> > + BPF_MOV64_IMM(BPF_REG_0, 1),
> > + BPF_EXIT_INSN(),
> > + };
> > + int fd;
>
> This is really not much better than test_verifier assembly. What I had
> in mind when I was suggesting to use test_progs was that you'd have a
> normal C source code for BPF part, something like this:
>
> __u64 tmp;
>
> SEC("?lsm_cgroup/socket_bind")
> int BPF_PROG(access1_bad, struct socket *sock, struct sockaddr
> *address, int addrlen)
> {
> *(volatile u16 *)(sock->sk.skc_family) = *(volatile u16
> *)sock->sk.skc_family;
> return 0;
> }
>
>
> SEC("?lsm_cgroup/socket_bind")
> int BPF_PROG(access2_bad, struct socket *sock, struct sockaddr
> *address, int addrlen)
> {
> *(volatile u64 *)(sock->sk.sk_sndtimeo) = *(volatile u64
> *)sock->sk.sk_sndtimeo;
> return 0;
> }
>
> and so on. From user-space you'd be loading just one of those
> accessX_bad programs at a time (note SEC("?"))
>
>
> But having said that, what you did is pretty self-contained, so not
> too bad. It's just not what I was suggesting :)
Yeah, that's what I suggested I was gonna try in:
https://lore.kernel.org/bpf/CAKH8qBuHU7OAjTMk-6GU08Nmwnn6J7Cw1TzP6GwCEq0x1Wwd9w@mail.gmail.com/
I don't really want to separate the program from the test, it seems
like keeping everything in one file is easier to read.
So unless you strongly dislike this new self-contained version, I'd
keep it as is.
> > +
> > + opts.attach_btf_id = libbpf_find_vmlinux_btf_id("socket_post_create",
> > + opts.expected_attach_type);
> > +
> > + fd = bpf_prog_load(BPF_PROG_TYPE_LSM, NULL, "GPL", insns, ARRAY_SIZE(insns), &opts);
> > + if (fd >= 0)
> > + close(fd);
> > + return fd >= 0;
> > +}
> > +
> > +static void test_lsm_cgroup_access(void)
> > +{
> > + ASSERT_FALSE(sk_writable_field("sock_common", "skc_family", BPF_H), "skc_family");
> > + ASSERT_FALSE(sk_writable_field("sock", "sk_sndtimeo", BPF_DW), "sk_sndtimeo");
> > + ASSERT_TRUE(sk_writable_field("sock", "sk_priority", BPF_W), "sk_priority");
> > + ASSERT_TRUE(sk_writable_field("sock", "sk_mark", BPF_W), "sk_mark");
> > + ASSERT_FALSE(sk_writable_field("sock", "sk_pacing_rate", BPF_DW), "sk_pacing_rate");
> > +}
> > +
> > void test_lsm_cgroup(void)
> > {
> > if (test__start_subtest("functional"))
> > test_lsm_cgroup_functional();
> > + if (test__start_subtest("access"))
> > + test_lsm_cgroup_access();
> > }
> > --
> > 2.36.1.124.g0e6072fb45-goog
> >
next prev parent reply other threads:[~2022-05-24 2:15 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-18 22:55 [PATCH bpf-next v7 00/11] bpf: cgroup_sock lsm flavor Stanislav Fomichev
2022-05-18 22:55 ` [PATCH bpf-next v7 01/11] bpf: add bpf_func_t and trampoline helpers Stanislav Fomichev
2022-05-20 0:45 ` Yonghong Song
2022-05-21 0:03 ` Stanislav Fomichev
2022-05-18 22:55 ` [PATCH bpf-next v7 02/11] bpf: convert cgroup_bpf.progs to hlist Stanislav Fomichev
2022-05-18 22:55 ` [PATCH bpf-next v7 03/11] bpf: per-cgroup lsm flavor Stanislav Fomichev
2022-05-20 1:00 ` Yonghong Song
2022-05-21 0:03 ` Stanislav Fomichev
2022-05-23 15:41 ` Yonghong Song
2022-05-21 0:53 ` Martin KaFai Lau
2022-05-24 2:15 ` Stanislav Fomichev
2022-05-24 5:40 ` Martin KaFai Lau
2022-05-24 15:56 ` Stanislav Fomichev
2022-05-24 5:57 ` Martin KaFai Lau
2022-05-18 22:55 ` [PATCH bpf-next v7 04/11] bpf: minimize number of allocated lsm slots per program Stanislav Fomichev
2022-05-21 6:56 ` Martin KaFai Lau
2022-05-24 2:14 ` Stanislav Fomichev
2022-05-24 5:53 ` Martin KaFai Lau
2022-05-18 22:55 ` [PATCH bpf-next v7 05/11] bpf: implement BPF_PROG_QUERY for BPF_LSM_CGROUP Stanislav Fomichev
2022-05-19 2:31 ` kernel test robot
2022-05-19 14:57 ` kernel test robot
2022-05-23 23:23 ` Andrii Nakryiko
2022-05-24 2:15 ` Stanislav Fomichev
2022-05-24 3:48 ` Martin KaFai Lau
2022-05-24 15:55 ` Stanislav Fomichev
2022-05-24 17:50 ` Martin KaFai Lau
2022-05-24 23:45 ` Andrii Nakryiko
2022-05-25 4:03 ` Stanislav Fomichev
2022-05-25 4:39 ` Andrii Nakryiko
2022-05-25 16:01 ` Stanislav Fomichev
2022-05-25 17:02 ` Stanislav Fomichev
2022-05-25 20:39 ` Martin KaFai Lau
2022-05-25 21:25 ` sdf
2022-05-26 0:03 ` Martin KaFai Lau
2022-05-26 1:23 ` Martin KaFai Lau
2022-05-26 2:50 ` Stanislav Fomichev
2022-05-31 23:08 ` Andrii Nakryiko
2022-05-18 22:55 ` [PATCH bpf-next v7 06/11] bpf: allow writing to a subset of sock fields from lsm progtype Stanislav Fomichev
2022-05-18 22:55 ` [PATCH bpf-next v7 07/11] libbpf: implement bpf_prog_query_opts Stanislav Fomichev
2022-05-23 23:22 ` Andrii Nakryiko
2022-05-24 2:15 ` Stanislav Fomichev
2022-05-24 3:45 ` Andrii Nakryiko
2022-05-24 4:01 ` Martin KaFai Lau
2022-05-18 22:55 ` [PATCH bpf-next v7 08/11] libbpf: add lsm_cgoup_sock type Stanislav Fomichev
2022-05-23 23:26 ` Andrii Nakryiko
2022-05-24 2:15 ` Stanislav Fomichev
2022-05-18 22:55 ` [PATCH bpf-next v7 09/11] bpftool: implement cgroup tree for BPF_LSM_CGROUP Stanislav Fomichev
2022-05-18 22:55 ` [PATCH bpf-next v7 10/11] selftests/bpf: lsm_cgroup functional test Stanislav Fomichev
2022-05-18 22:55 ` [PATCH bpf-next v7 11/11] selftests/bpf: verify lsm_cgroup struct sock access Stanislav Fomichev
2022-05-23 23:33 ` Andrii Nakryiko
2022-05-24 2:15 ` Stanislav Fomichev [this message]
2022-05-24 3:46 ` Andrii Nakryiko
2022-05-19 23:34 ` [PATCH bpf-next v7 00/11] bpf: cgroup_sock lsm flavor Yonghong Song
2022-05-19 23:39 ` Stanislav Fomichev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKH8qBsQH2fwxa6B6LOqfw1ru_qk9wyypXnAzy4u+uBYBmQq8w@mail.gmail.com \
--to=sdf@google.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).