| 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
| ||
|
Date: Fri, 26 Aug 2022 04:26:30 +0200
From: Kumar Kartikeya Dwivedi <memxor@...il.com>
To: Benjamin Tissoires <benjamin.tissoires@...hat.com>
Cc: Greg KH <gregkh@...uxfoundation.org>,
Jiri Kosina <jikos@...nel.org>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau <kafai@...com>,
Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...nel.org>, Shuah Khan <shuah@...nel.org>,
Dave Marchevsky <davemarchevsky@...com>,
Joe Stringer <joe@...ium.io>, Jonathan Corbet <corbet@....net>,
Tero Kristo <tero.kristo@...ux.intel.com>,
linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
netdev@...r.kernel.org, bpf@...r.kernel.org,
linux-kselftest@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH bpf-next v9 05/23] selftests/bpf: Add tests for kfunc
returning a memory pointer
On Wed, 24 Aug 2022 at 15:41, Benjamin Tissoires
<benjamin.tissoires@...hat.com> wrote:
>
> We add 2 new kfuncs that are following the RET_PTR_TO_MEM
> capability from the previous commit.
> Then we test them in selftests:
> the first tests are testing valid case, and are not failing,
> and the later ones are actually preventing the program to be loaded
> because they are wrong.
>
> To work around that, we mark the failing ones as not autoloaded
> (with SEC("?tc")), and we manually enable them one by one, ensuring
> the verifier rejects them.
>
> To be able to use bpf_program__set_autoload() from libbpf, we need
> to use a plain skeleton, not a light-skeleton, and this is why we
> also change the Makefile to generate both for kfunc_call_test.c
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@...hat.com>
>
> ---
>
> changes in v9:
> - updated to match upstream (net/bpf/test_run.c id sets is now using
> flags)
>
> no changes in v8
>
> changes in v7:
> - removed stray include/linux/btf.h change
>
> new in v6
> ---
> net/bpf/test_run.c | 20 +++++
> tools/testing/selftests/bpf/Makefile | 5 +-
> .../selftests/bpf/prog_tests/kfunc_call.c | 48 ++++++++++
> .../selftests/bpf/progs/kfunc_call_test.c | 89 +++++++++++++++++++
> 4 files changed, 160 insertions(+), 2 deletions(-)
>
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index f16baf977a21..6accd57d4ded 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -606,6 +606,24 @@ noinline void bpf_kfunc_call_memb1_release(struct prog_test_member1 *p)
> WARN_ON_ONCE(1);
> }
>
> +static int *__bpf_kfunc_call_test_get_mem(struct prog_test_ref_kfunc *p, const int size)
> +{
> + if (size > 2 * sizeof(int))
> + return NULL;
> +
> + return (int *)p;
> +}
> +
> +noinline int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size)
> +{
> + return __bpf_kfunc_call_test_get_mem(p, rdwr_buf_size);
> +}
> +
> +noinline int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size)
> +{
> + return __bpf_kfunc_call_test_get_mem(p, rdonly_buf_size);
> +}
> +
> noinline struct prog_test_ref_kfunc *
> bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **pp, int a, int b)
> {
> @@ -712,6 +730,8 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_memb_acquire, KF_ACQUIRE | KF_RET_NULL)
> BTF_ID_FLAGS(func, bpf_kfunc_call_test_release, KF_RELEASE)
> BTF_ID_FLAGS(func, bpf_kfunc_call_memb_release, KF_RELEASE)
> BTF_ID_FLAGS(func, bpf_kfunc_call_memb1_release, KF_RELEASE)
> +BTF_ID_FLAGS(func, bpf_kfunc_call_test_get_rdwr_mem, KF_RET_NULL)
> +BTF_ID_FLAGS(func, bpf_kfunc_call_test_get_rdonly_mem, KF_RET_NULL)
> BTF_ID_FLAGS(func, bpf_kfunc_call_test_kptr_get, KF_ACQUIRE | KF_RET_NULL | KF_KPTR_GET)
> BTF_ID_FLAGS(func, bpf_kfunc_call_test_pass_ctx)
> BTF_ID_FLAGS(func, bpf_kfunc_call_test_pass1)
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 8d59ec7f4c2d..0905315ff86d 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -350,11 +350,12 @@ LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h \
> test_subskeleton.skel.h test_subskeleton_lib.skel.h \
> test_usdt.skel.h
>
> -LSKELS := kfunc_call_test.c fentry_test.c fexit_test.c fexit_sleep.c \
> +LSKELS := fentry_test.c fexit_test.c fexit_sleep.c \
> test_ringbuf.c atomics.c trace_printk.c trace_vprintk.c \
> map_ptr_kern.c core_kern.c core_kern_overflow.c
> # Generate both light skeleton and libbpf skeleton for these
> -LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c kfunc_call_test_subprog.c
> +LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c kfunc_call_test.c \
> + kfunc_call_test_subprog.c
> SKEL_BLACKLIST += $$(LSKELS)
>
> test_static_linked.skel.h-deps := test_static_linked1.o test_static_linked2.o
> diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
> index 1edad012fe01..590417d48962 100644
> --- a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
> +++ b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
> @@ -2,6 +2,7 @@
> /* Copyright (c) 2021 Facebook */
> #include <test_progs.h>
> #include <network_helpers.h>
> +#include "kfunc_call_test.skel.h"
> #include "kfunc_call_test.lskel.h"
> #include "kfunc_call_test_subprog.skel.h"
> #include "kfunc_call_test_subprog.lskel.h"
> @@ -53,10 +54,12 @@ static void test_main(void)
> prog_fd = skel->progs.kfunc_syscall_test.prog_fd;
> err = bpf_prog_test_run_opts(prog_fd, &syscall_topts);
> ASSERT_OK(err, "bpf_prog_test_run(syscall_test)");
> + ASSERT_EQ(syscall_topts.retval, 0, "syscall_test-retval");
>
> prog_fd = skel->progs.kfunc_syscall_test_fail.prog_fd;
> err = bpf_prog_test_run_opts(prog_fd, &syscall_topts);
> ASSERT_ERR(err, "bpf_prog_test_run(syscall_test_fail)");
> + ASSERT_EQ(syscall_topts.retval, 0, "syscall_test_fail-retval");
>
> syscall_topts.ctx_in = NULL;
> syscall_topts.ctx_size_in = 0;
> @@ -147,6 +150,48 @@ static void test_destructive(void)
> cap_enable_effective(save_caps, NULL);
> }
>
> +static void test_get_mem(void)
> +{
> + struct kfunc_call_test *skel;
> + int prog_fd, err;
> + LIBBPF_OPTS(bpf_test_run_opts, topts,
> + .data_in = &pkt_v4,
> + .data_size_in = sizeof(pkt_v4),
> + .repeat = 1,
> + );
> +
> + skel = kfunc_call_test__open_and_load();
> + if (!ASSERT_OK_PTR(skel, "skel"))
> + return;
> +
> + prog_fd = bpf_program__fd(skel->progs.kfunc_call_test_get_mem);
> + err = bpf_prog_test_run_opts(prog_fd, &topts);
> + ASSERT_OK(err, "bpf_prog_test_run(test_get_mem)");
> + ASSERT_EQ(topts.retval, 42, "test_get_mem-retval");
> +
> + kfunc_call_test__destroy(skel);
> +
> + /* start the various failing tests */
> + skel = kfunc_call_test__open();
> + if (!ASSERT_OK_PTR(skel, "skel"))
> + return;
> +
> + bpf_program__set_autoload(skel->progs.kfunc_call_test_get_mem_fail1, true);
> + err = kfunc_call_test__load(skel);
> + ASSERT_ERR(err, "load(kfunc_call_test_get_mem_fail1)");
> + kfunc_call_test__destroy(skel);
> +
> + skel = kfunc_call_test__open();
> + if (!ASSERT_OK_PTR(skel, "skel"))
> + return;
> +
> + bpf_program__set_autoload(skel->progs.kfunc_call_test_get_mem_fail2, true);
> + err = kfunc_call_test__load(skel);
> + ASSERT_ERR(err, "load(kfunc_call_test_get_mem_fail2)");
We should match the verifier error string. See e.g. how dynptr tests
work. Also it would be better to split failure and success tests into
separate objects.
> +
> + kfunc_call_test__destroy(skel);
> +}
> +
> void test_kfunc_call(void)
> {
> if (test__start_subtest("main"))
> @@ -160,4 +205,7 @@ void test_kfunc_call(void)
>
> if (test__start_subtest("destructive"))
> test_destructive();
> +
> + if (test__start_subtest("get_mem"))
> + test_get_mem();
> }
> diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_test.c b/tools/testing/selftests/bpf/progs/kfunc_call_test.c
> index da7ae0ef9100..b4a98d17c2b6 100644
> --- a/tools/testing/selftests/bpf/progs/kfunc_call_test.c
> +++ b/tools/testing/selftests/bpf/progs/kfunc_call_test.c
> @@ -14,6 +14,8 @@ extern void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p) __ksym;
> extern void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p) __ksym;
> extern void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len) __ksym;
> extern void bpf_kfunc_call_test_mem_len_fail2(__u64 *mem, int len) __ksym;
> +extern int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) __ksym;
> +extern int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
>
> SEC("tc")
> int kfunc_call_test2(struct __sk_buff *skb)
> @@ -128,4 +130,91 @@ int kfunc_syscall_test_fail(struct syscall_test_args *args)
> return 0;
> }
>
> +SEC("tc")
> +int kfunc_call_test_get_mem(struct __sk_buff *skb)
> +{
> + struct prog_test_ref_kfunc *pt;
> + unsigned long s = 0;
> + int *p = NULL;
> + int ret = 0;
> +
> + pt = bpf_kfunc_call_test_acquire(&s);
> + if (pt) {
> + if (pt->a != 42 || pt->b != 108)
> + ret = -1;
No need to test this I think.
> +
> + p = bpf_kfunc_call_test_get_rdwr_mem(pt, 2 * sizeof(int));
> + if (p) {
> + p[0] = 42;
> + ret = p[1]; /* 108 */
> + } else {
> + ret = -1;
> + }
> +
> + if (ret >= 0) {
> + p = bpf_kfunc_call_test_get_rdonly_mem(pt, 2 * sizeof(int));
> + if (p)
> + ret = p[0]; /* 42 */
> + else
> + ret = -1;
> + }
> +
> + bpf_kfunc_call_test_release(pt);
> + }
> + return ret;
> +}
> +
> +SEC("?tc")
> +int kfunc_call_test_get_mem_fail1(struct __sk_buff *skb)
> +{
> + struct prog_test_ref_kfunc *pt;
> + unsigned long s = 0;
> + int *p = NULL;
> + int ret = 0;
> +
> + pt = bpf_kfunc_call_test_acquire(&s);
> + if (pt) {
> + if (pt->a != 42 || pt->b != 108)
> + ret = -1;
> +
> + p = bpf_kfunc_call_test_get_rdonly_mem(pt, 2 * sizeof(int));
> + if (p)
> + p[0] = 42; /* this is a read-only buffer, so -EACCES */
> + else
> + ret = -1;
> +
> + bpf_kfunc_call_test_release(pt);
> + }
> + return ret;
> +}
> +
> +SEC("?tc")
> +int kfunc_call_test_get_mem_fail2(struct __sk_buff *skb)
> +{
> + struct prog_test_ref_kfunc *pt;
> + unsigned long s = 0;
> + int *p = NULL;
> + int ret = 0;
> +
> + pt = bpf_kfunc_call_test_acquire(&s);
> + if (pt) {
> + if (pt->a != 42 || pt->b != 108)
> + ret = -1;
> +
> + p = bpf_kfunc_call_test_get_rdwr_mem(pt, 2 * sizeof(int));
> + if (p) {
> + p[0] = 42;
> + ret = p[1]; /* 108 */
> + } else {
> + ret = -1;
> + }
> +
> + bpf_kfunc_call_test_release(pt);
> + }
> + if (p)
> + ret = p[0]; /* p is not valid anymore */
Great that this ref_obj_id transfer is tested. A few more small test
cases that come to mind:
. oob access to returned ptr_to_mem to ensure size is set correctly.
. failure when size is not 'const', since this is not going through
check_mem_size_reg.
. incorrect acq kfunc type inside kernel so that on use its ret type
is not struct ptr, so verifier complains about it.
> +
> + return ret;
> +}
> +
> char _license[] SEC("license") = "GPL";
> --
> 2.36.1
>
Powered by blists - more mailing lists