On Tue, 6 Sept 2022 at 17:13, Benjamin Tissoires benjamin.tissoires@redhat.com wrote:

Similar to tools/testing/selftests/bpf/prog_tests/dynptr.c: we declare an array of tests that we run one by one in a for loop.

Followup patches will add more similar-ish tests, so avoid a lot of copy paste by grouping the declaration in an array.

For light skeletons, we have to rely on the offsetof() macro so we can statically declare which program we are using. In the libbpf case, we can rely on bpf_object__find_program_by_name(). So also change the Makefile to generate both light skeletons and normal ones.

Signed-off-by: Benjamin Tissoires benjamin.tissoires@redhat.com

---

Acked-by: Kumar Kartikeya Dwivedi memxor@gmail.com

changes in v11:

• use of both light skeletons and normal libbpf

new in v10

tools/testing/selftests/bpf/Makefile | 5 +- .../selftests/bpf/prog_tests/kfunc_call.c | 81 +++++++++++++++---- 2 files changed, 68 insertions(+), 18 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index eecad99f1735..314216d77b8d 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -351,11 +351,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 eede7c304f86..9dfbe5355a2d 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" @@ -9,9 +10,31 @@

#include "cap_helpers.h"

-static void test_main(void) +struct kfunc_test_params {

•   const char *prog_name;
  

•   unsigned long lskel_prog_desc_offset;
  

•   int retval;
  

+};

+#define TC_TEST(name, __retval) \

•   { \
  

•     .prog_name = #name, \
  

•     .lskel_prog_desc_offset = offsetof(struct kfunc_call_test_lskel, progs.name), \
  

•     .retval = __retval, \
  

•   }
  

+static struct kfunc_test_params kfunc_tests[] = {

•   TC_TEST(kfunc_call_test1, 12),
  

•   TC_TEST(kfunc_call_test2, 3),
  

•   TC_TEST(kfunc_call_test_ref_btf_id, 0),
  

+};

+static void verify_success(struct kfunc_test_params *param) {

•   struct kfunc_call_test_lskel *skel;
  

•   struct kfunc_call_test_lskel *lskel = NULL;
  

•   struct bpf_prog_desc *lskel_prog;
  

•   struct kfunc_call_test *skel;
  

•   struct bpf_program *prog;
    int prog_fd, err;
    LIBBPF_OPTS(bpf_test_run_opts, topts,
            .data_in = &pkt_v4,
  

@@ -19,26 +42,53 @@ static void test_main(void) .repeat = 1, );

•   skel = kfunc_call_test_lskel__open_and_load();
  

•   /* first test with normal libbpf */
  

•   skel = kfunc_call_test__open_and_load();
    if (!ASSERT_OK_PTR(skel, "skel"))
            return;
  
  

•   prog_fd = skel->progs.kfunc_call_test1.prog_fd;
  

•   err = bpf_prog_test_run_opts(prog_fd, &topts);
  

•   ASSERT_OK(err, "bpf_prog_test_run(test1)");
  

•   ASSERT_EQ(topts.retval, 12, "test1-retval");
  

•   prog = bpf_object__find_program_by_name(skel->obj, param->prog_name);
  

•   if (!ASSERT_OK_PTR(prog, "bpf_object__find_program_by_name"))
  

•           goto cleanup;
  
  

•   prog_fd = skel->progs.kfunc_call_test2.prog_fd;
  

•   prog_fd = bpf_program__fd(prog);
    err = bpf_prog_test_run_opts(prog_fd, &topts);
  

•   ASSERT_OK(err, "bpf_prog_test_run(test2)");
  

•   ASSERT_EQ(topts.retval, 3, "test2-retval");
  

•   if (!ASSERT_OK(err, param->prog_name))
  

•           goto cleanup;
  

•   if (!ASSERT_EQ(topts.retval, param->retval, "retval"))
  

•           goto cleanup;
  

•   /* second test with light skeletons */
  

•   lskel = kfunc_call_test_lskel__open_and_load();
  

•   if (!ASSERT_OK_PTR(lskel, "lskel"))
  

•           goto cleanup;
  

•   lskel_prog = (struct bpf_prog_desc *)((char *)lskel + param->lskel_prog_desc_offset);
  
  

•   prog_fd = skel->progs.kfunc_call_test_ref_btf_id.prog_fd;
  

•   prog_fd = lskel_prog->prog_fd;
    err = bpf_prog_test_run_opts(prog_fd, &topts);
  

•   ASSERT_OK(err, "bpf_prog_test_run(test_ref_btf_id)");
  

•   ASSERT_EQ(topts.retval, 0, "test_ref_btf_id-retval");
  

•   if (!ASSERT_OK(err, param->prog_name))
  

•           goto cleanup;
  

•   ASSERT_EQ(topts.retval, param->retval, "retval");
  

+cleanup:

•   kfunc_call_test__destroy(skel);
  

•   if (lskel)
  

•           kfunc_call_test_lskel__destroy(lskel);
  

+}

+static void test_main(void) +{

•   int i;
  

•   for (i = 0; i < ARRAY_SIZE(kfunc_tests); i++) {
  

•           if (!test__start_subtest(kfunc_tests[i].prog_name))
  

•                   continue;
  
  

•   kfunc_call_test_lskel__destroy(skel);
  

•           verify_success(&kfunc_tests[i]);
  

•   }
  

}

static void test_subprog(void) @@ -121,8 +171,7 @@ static void test_destructive(void)

void test_kfunc_call(void) {

•   if (test__start_subtest("main"))
  

•           test_main();
  

•   test_main();
  
    if (test__start_subtest("subprog"))
            test_subprog();
  

-- 2.36.1

On Tue, 6 Sept 2022 at 17:13, Benjamin Tissoires benjamin.tissoires@redhat.com wrote:

btf_check_subprog_arg_match() was used twice in verifier.c:

• when checking for the type mismatches between a (sub)prog declaration and BTF
• when checking the call of a subprog to see if the provided arguments are correct and valid

This is problematic when we check if the first argument of a program (pointer to ctx) is correctly accessed: To be able to ensure we access a valid memory in the ctx, the verifier assumes the pointer to context is not null. This has the side effect of marking the program accessing the entire context, even if the context is never dereferenced.

For example, by checking the context access with the current code, the following eBPF program would fail with -EINVAL if the ctx is set to null from the userspace:

SEC("syscall")
int prog(struct my_ctx *args) {
  return 0;
}

In that particular case, we do not want to actually check that the memory is correct while checking for the BTF validity, but we just want to ensure that the (sub)prog definition matches the BTF we have.

So split btf_check_subprog_arg_match() in two so we can actually check for the memory used when in a call, and ignore that part when not.

Note that a further patch is in preparation to disentangled btf_check_func_arg_match() from these two purposes, and so right now we just add a new hack around that by adding a boolean to this function.

Signed-off-by: Benjamin Tissoires benjamin.tissoires@redhat.com

---

Given I'll fix it properly in my kfunc rework, LGTM otherwise: Acked-by: Kumar Kartikeya Dwivedi memxor@gmail.com

no changes in v11

new in v10

include/linux/bpf.h | 2 ++ kernel/bpf/btf.c | 54 +++++++++++++++++++++++++++++++++++++++---- kernel/bpf/verifier.c | 2 +- 3 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 9c1674973e03..c9c72a089579 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1943,6 +1943,8 @@ int btf_distill_func_proto(struct bpf_verifier_log *log, struct bpf_reg_state; int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, struct bpf_reg_state *regs); +int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,

•                      struct bpf_reg_state *regs);
  

int btf_check_kfunc_arg_match(struct bpf_verifier_env *env, const struct btf *btf, u32 func_id, struct bpf_reg_state *regs, diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 903719b89238..eca9ea78ee5f 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -6170,7 +6170,8 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, const struct btf *btf, u32 func_id, struct bpf_reg_state *regs, bool ptr_to_mem_ok,

•                               u32 kfunc_flags)
  

•                               u32 kfunc_flags,
  

•                               bool processing_call)
  

{ enum bpf_prog_type prog_type = resolve_prog_type(env->prog); bool rel = false, kptr_get = false, trusted_arg = false; @@ -6356,7 +6357,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, reg_ref_tname); return -EINVAL; }

•           } else if (ptr_to_mem_ok) {
  

•           } else if (ptr_to_mem_ok && processing_call) {
                    const struct btf_type *resolve_ret;
                    u32 type_size;
  
  

@@ -6431,7 +6432,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, return rel ? ref_regno : 0; }

-/* Compare BTF of a function with given bpf_reg_state. +/* Compare BTF of a function declaration with given bpf_reg_state.

• Returns:
• EFAULT - there is a verifier bug. Abort verification.
• EINVAL - there is a type mismatch or BTF is not available.

@@ -6458,7 +6459,50 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, return -EINVAL;

    is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL;

•   err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, 0);
  

•   err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, 0, false);
  

•   /* Compiler optimizations can remove arguments from static functions
  

•    * or mismatched type can be passed into a global function.
  

•    * In such cases mark the function as unreliable from BTF point of view.
  

•    */
  

•   if (err)
  

•           prog->aux->func_info_aux[subprog].unreliable = true;
  

•   return err;
  

+}

+/* Compare BTF of a function call with given bpf_reg_state.

• • Returns:
• • EFAULT - there is a verifier bug. Abort verification.
• • EINVAL - there is a type mismatch or BTF is not available.
• • 0 - BTF matches with what bpf_reg_state expects.
• • Only PTR_TO_CTX and SCALAR_VALUE states are recognized.
• • NOTE: the code is duplicated from btf_check_subprog_arg_match()
• • because btf_check_func_arg_match() is still doing both. Once that
• • function is split in 2, we can call from here btf_check_subprog_arg_match()
• • first, and then treat the calling part in a new code path.
• */

+int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,

•                      struct bpf_reg_state *regs)
  

+{

•   struct bpf_prog *prog = env->prog;
  

•   struct btf *btf = prog->aux->btf;
  

•   bool is_global;
  

•   u32 btf_id;
  

•   int err;
  

•   if (!prog->aux->func_info)
  

•           return -EINVAL;
  

•   btf_id = prog->aux->func_info[subprog].type_id;
  

•   if (!btf_id)
  

•           return -EFAULT;
  

•   if (prog->aux->func_info_aux[subprog].unreliable)
  

•           return -EINVAL;
  

•   is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL;
  

•   err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, 0, true);
  
    /* Compiler optimizations can remove arguments from static functions
     * or mismatched type can be passed into a global function.
  

@@ -6474,7 +6518,7 @@ int btf_check_kfunc_arg_match(struct bpf_verifier_env *env, struct bpf_reg_state *regs, u32 kfunc_flags) {

•   return btf_check_func_arg_match(env, btf, func_id, regs, true, kfunc_flags);
  

•   return btf_check_func_arg_match(env, btf, func_id, regs, true, kfunc_flags, true);
  

}

/* Convert BTF of a function into bpf_reg_state if possible diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 0194a36d0b36..d27fae3ce949 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -6626,7 +6626,7 @@ static int __check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn func_info_aux = env->prog->aux->func_info_aux; if (func_info_aux) is_global = func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL;

•   err = btf_check_subprog_arg_match(env, subprog, caller->regs);
  

•   err = btf_check_subprog_call(env, subprog, caller->regs);
    if (err == -EFAULT)
            return err;
    if (is_global) {
  

-- 2.36.1

On Wed, Sep 7, 2022 at 10:46 AM Kumar Kartikeya Dwivedi memxor@gmail.com wrote:

On Tue, 6 Sept 2022 at 17:13, Benjamin Tissoires benjamin.tissoires@redhat.com wrote:

...

We need to also export the kfunc set to the syscall program type, and then add a couple of eBPF programs that are testing those calls.

The first one checks for valid access, and the second one is OK from a static analysis point of view but fails at run time because we are trying to access outside of the allocated memory.

Signed-off-by: Benjamin Tissoires benjamin.tissoires@redhat.com

---

CI is failing for test_progs-no_alu32: https://github.com/kernel-patches/bpf/runs/8220916615?check_suite_focus=true

...

changes in v11:

• use new way of declaring tests

changes in v10:

• use new definitions for tests in an array
• add a new kfunc syscall_test_null_fail test

no changes in v9

no changes in v8

changes in v7:

• add 1 more case to ensure we can read the entire sizeof(ctx)
• add a test case for when the context is NULL

new in v6

net/bpf/test_run.c | 1 + .../selftests/bpf/prog_tests/kfunc_call.c | 143 +++++++++++++++++- .../selftests/bpf/progs/kfunc_call_fail.c | 39 +++++ .../selftests/bpf/progs/kfunc_call_test.c | 38 +++++ 4 files changed, 214 insertions(+), 7 deletions(-) create mode 100644 tools/testing/selftests/bpf/progs/kfunc_call_fail.c

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index 25d8ecf105aa..f16baf977a21 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -1634,6 +1634,7 @@ static int __init bpf_prog_test_run_init(void)

    ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_prog_test_kfunc_set);
    ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_prog_test_kfunc_set);

•   ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &bpf_prog_test_kfunc_set);
    return ret ?: register_btf_id_dtor_kfuncs(bpf_prog_test_dtor_kfunc,
                                              ARRAY_SIZE(bpf_prog_test_dtor_kfunc),
                                              THIS_MODULE);
  

diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c index 9dfbe5355a2d..d5881c3331a8 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_fail.skel.h" #include "kfunc_call_test.skel.h" #include "kfunc_call_test.lskel.h" #include "kfunc_call_test_subprog.skel.h" @@ -10,37 +11,96 @@

#include "cap_helpers.h"

+static size_t log_buf_sz = 1048576; /* 1 MB */ +static char obj_log_buf[1048576];

+enum kfunc_test_type {

•   tc_test = 0,
  

•   syscall_test,
  

•   syscall_null_ctx_test,
  

+};

struct kfunc_test_params { const char *prog_name; unsigned long lskel_prog_desc_offset; int retval;

•   enum kfunc_test_type test_type;
  

•   const char *expected_err_msg;
  

};

-#define TC_TEST(name, __retval) \ +#define __BPF_TEST_SUCCESS(name, __retval, type) \ { \ .prog_name = #name, \ .lskel_prog_desc_offset = offsetof(struct kfunc_call_test_lskel, progs.name), \ .retval = __retval, \

•     .test_type = type, \
  

•     .expected_err_msg = NULL, \
  

•   }
  

+#define __BPF_TEST_FAIL(name, __retval, type, error_msg) \

•   { \
  

•     .prog_name = #name, \
  

•     .lskel_prog_desc_offset = 0 /* unused when test is failing */, \
  

•     .retval = __retval, \
  

•     .test_type = type, \
  

•     .expected_err_msg = error_msg, \
    }
  
  

+#define TC_TEST(name, retval) __BPF_TEST_SUCCESS(name, retval, tc_test) +#define SYSCALL_TEST(name, retval) __BPF_TEST_SUCCESS(name, retval, syscall_test) +#define SYSCALL_NULL_CTX_TEST(name, retval) __BPF_TEST_SUCCESS(name, retval, syscall_null_ctx_test)

+#define SYSCALL_NULL_CTX_FAIL(name, retval, error_msg) \

•   __BPF_TEST_FAIL(name, retval, syscall_null_ctx_test, error_msg)
  

static struct kfunc_test_params kfunc_tests[] = {

•   /* failure cases:
  

•    * if retval is 0 -> the program will fail to load and the error message is an error
  

•    * if retval is not 0 -> the program can be loaded but running it will gives the
  

•    *                       provided return value. The error message is thus the one
  

•    *                       from a successful load
  

•    */
  

•   SYSCALL_NULL_CTX_FAIL(kfunc_syscall_test_fail, -EINVAL, "processed 4 insns"),
  

•   SYSCALL_NULL_CTX_FAIL(kfunc_syscall_test_null_fail, -EINVAL, "processed 4 insns"),
  

•   /* success cases */
    TC_TEST(kfunc_call_test1, 12),
    TC_TEST(kfunc_call_test2, 3),
    TC_TEST(kfunc_call_test_ref_btf_id, 0),
  

•   SYSCALL_TEST(kfunc_syscall_test, 0),
  

•   SYSCALL_NULL_CTX_TEST(kfunc_syscall_test_null, 0),
  

+};

+struct syscall_test_args {

•   __u8 data[16];
  

•   size_t size;
  

};

static void verify_success(struct kfunc_test_params *param) { struct kfunc_call_test_lskel *lskel = NULL;

•   LIBBPF_OPTS(bpf_test_run_opts, topts);
    struct bpf_prog_desc *lskel_prog;
    struct kfunc_call_test *skel;
    struct bpf_program *prog;
    int prog_fd, err;
  

•   LIBBPF_OPTS(bpf_test_run_opts, topts,
  

•           .data_in = &pkt_v4,
  

•           .data_size_in = sizeof(pkt_v4),
  

•           .repeat = 1,
  

•   );
  

•   struct syscall_test_args args = {
  

•           .size = 10,
  

•   };
  

•   switch (param->test_type) {
  

•   case syscall_test:
  

•           topts.ctx_in = &args;
  

•           topts.ctx_size_in = sizeof(args);
  

•           /* fallthrough */
  

•   case syscall_null_ctx_test:
  

•           break;
  

•   case tc_test:
  

•           topts.data_in = &pkt_v4;
  

•           topts.data_size_in = sizeof(pkt_v4);
  

•           topts.repeat = 1;
  

•           break;
  

•   }
  
    /* first test with normal libbpf */
    skel = kfunc_call_test__open_and_load();
  

@@ -79,6 +139,72 @@ static void verify_success(struct kfunc_test_params *param) kfunc_call_test_lskel__destroy(lskel); }

+static void verify_fail(struct kfunc_test_params *param) +{

•   LIBBPF_OPTS(bpf_object_open_opts, opts);
  

•   LIBBPF_OPTS(bpf_test_run_opts, topts);
  

•   struct bpf_program *prog;
  

•   struct kfunc_call_fail *skel;
  

•   int prog_fd, err;
  

•   struct syscall_test_args args = {
  

•           .size = 10,
  

•   };
  

•   opts.kernel_log_buf = obj_log_buf;
  

•   opts.kernel_log_size = log_buf_sz;
  

•   opts.kernel_log_level = 1;
  

•   switch (param->test_type) {
  

•   case syscall_test:
  

•           topts.ctx_in = &args;
  

•           topts.ctx_size_in = sizeof(args);
  

•           /* fallthrough */
  

•   case syscall_null_ctx_test:
  

•           break;
  

•   case tc_test:
  

•           topts.data_in = &pkt_v4;
  

•           topts.data_size_in = sizeof(pkt_v4);
  

•           break;
  

•           topts.repeat = 1;
  

•   }
  

•   skel = kfunc_call_fail__open_opts(&opts);
  

•   if (!ASSERT_OK_PTR(skel, "kfunc_call_fail__open_opts"))
  

•           goto cleanup;
  

•   prog = bpf_object__find_program_by_name(skel->obj, param->prog_name);
  

•   if (!ASSERT_OK_PTR(prog, "bpf_object__find_program_by_name"))
  

•           goto cleanup;
  

•   bpf_program__set_autoload(prog, true);
  

•   err = kfunc_call_fail__load(skel);
  

•   if (!param->retval) {
  

•           /* the verifier is supposed to complain and refuses to load */
  

•           if (!ASSERT_ERR(err, "unexpected load success"))
  

•                   goto out_err;
  

•   } else {
  

•           /* the program is loaded but must dynamically fail */
  

•           if (!ASSERT_OK(err, "unexpected load error"))
  

•                   goto out_err;
  

•           prog_fd = bpf_program__fd(prog);
  

•           err = bpf_prog_test_run_opts(prog_fd, &topts);
  

•           if (!ASSERT_EQ(err, param->retval, param->prog_name))
  

•                   goto out_err;
  

•   }
  

+out_err:

•   if (!ASSERT_OK_PTR(strstr(obj_log_buf, param->expected_err_msg), "expected_err_msg")) {
  

•           fprintf(stderr, "Expected err_msg: %s\n", param->expected_err_msg);
  

•           fprintf(stderr, "Verifier output: %s\n", obj_log_buf);
  

•   }
  

+cleanup:

•   kfunc_call_fail__destroy(skel);
  

+}

static void test_main(void) { int i; @@ -87,7 +213,10 @@ static void test_main(void) if (!test__start_subtest(kfunc_tests[i].prog_name)) continue;

•           verify_success(&kfunc_tests[i]);
  

•           if (!kfunc_tests[i].expected_err_msg)
  

•                   verify_success(&kfunc_tests[i]);
  

•           else
  

•                   verify_fail(&kfunc_tests[i]);
    }
  

}

diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_fail.c b/tools/testing/selftests/bpf/progs/kfunc_call_fail.c new file mode 100644 index 000000000000..4168027f2ab1 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/kfunc_call_fail.c @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2021 Facebook */ +#include <vmlinux.h> +#include <bpf/bpf_helpers.h>

+extern void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len) __ksym;

+struct syscall_test_args {

•   __u8 data[16];
  

•   size_t size;
  

+};

+SEC("?syscall") +int kfunc_syscall_test_fail(struct syscall_test_args *args) +{

•   bpf_kfunc_call_test_mem_len_pass1(&args->data, sizeof(*args) + 1);
  

•   return 0;
  

+}

+SEC("?syscall") +int kfunc_syscall_test_null_fail(struct syscall_test_args *args) +{

•   /* Must be called with args as a NULL pointer
  

•    * we do not check for it to have the verifier consider that
  

•    * the pointer might not be null, and so we can load it.
  

•    *
  

•    * So the following can not be added:
  

•    *
  

•    * if (args)
  

•    *      return -22;
  

•    */
  

•   bpf_kfunc_call_test_mem_len_pass1(args, sizeof(*args));
  

•   return 0;
  

+}

+char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_test.c b/tools/testing/selftests/bpf/progs/kfunc_call_test.c index 5aecbb9fdc68..94c05267e5e7 100644 --- a/tools/testing/selftests/bpf/progs/kfunc_call_test.c +++ b/tools/testing/selftests/bpf/progs/kfunc_call_test.c @@ -92,4 +92,42 @@ int kfunc_call_test_pass(struct __sk_buff *skb) return 0; }

+struct syscall_test_args {

•   __u8 data[16];
  

•   size_t size;
  

+};

+SEC("syscall") +int kfunc_syscall_test(struct syscall_test_args *args) +{

•   const int size = args->size;
  

•   if (size > sizeof(args->data))
  

•           return -7; /* -E2BIG */
  

Looks like it is due to this. Verifier is confused because: r7 = args->data; r1 = r7;

then it does r1 <<= 32; r1 >>=32; clearing upper 32 bits, so both r1 and r7 lose the id association which propagates the bounds of r1 learnt from comparison of it with sizeof(args->data);

...

•   bpf_kfunc_call_test_mem_len_pass1(&args->data, sizeof(args->data));
  

•   bpf_kfunc_call_test_mem_len_pass1(&args->data, sizeof(*args));
  

Later llvm assigns r7 to r2 for this call's 2nd arg. At this point the verifier still thinks r7 is unbounded, while to make a call with mem, len pair you need non-negative min value.

Easiest way might be to just do args->size & sizeof(args->data), as the verifier log says. You might still keep the error above. Others may have better ideas/insights.

I just did s/const int size/const long size/ to fix the issues.

Also fixed commit in patch 3 that talks about max_ctx_offset and did: - BTF_KFUNC_SET_MAX_CNT = 64, + BTF_KFUNC_SET_MAX_CNT = 256,

and applied.

On Tue, 6 Sept 2022 at 17:13, Benjamin Tissoires benjamin.tissoires@redhat.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.

Signed-off-by: Benjamin Tissoires benjamin.tissoires@redhat.com

---

Thanks for adding these tests. Acked-by: Kumar Kartikeya Dwivedi memxor@gmail.com

changes in v11:

• use new TC_* declaration of tests

changes in v10:

• use new definition for tests
• remove the Makefile change, it was done before
• renamed the failed tests to be more explicit
• add 2 more test cases for return mem: oob access and non const access
• add one more test case for an invalid acquire function returning an int pointer

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 | 36 ++++++ .../selftests/bpf/prog_tests/kfunc_call.c | 7 + .../selftests/bpf/progs/kfunc_call_fail.c | 121 ++++++++++++++++++ .../selftests/bpf/progs/kfunc_call_test.c | 33 +++++ 4 files changed, 197 insertions(+)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index f16baf977a21..13d578ce2a09 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -606,6 +606,38 @@ 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);
  

+}

+/* the next 2 ones can't be really used for testing expect to ensure

• • that the verifier rejects the call.
• • Acquire functions must return struct pointers, so these ones are
• • failing.
• */

+noinline int *bpf_kfunc_call_test_acq_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 void bpf_kfunc_call_int_mem_release(int *p) +{ +}

noinline struct prog_test_ref_kfunc * bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **pp, int a, int b) { @@ -712,6 +744,10 @@ 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_acq_rdonly_mem, KF_ACQUIRE | KF_RET_NULL) +BTF_ID_FLAGS(func, bpf_kfunc_call_int_mem_release, KF_RELEASE) 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/prog_tests/kfunc_call.c b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c index d5881c3331a8..5af1ee8f0e6e 100644 --- a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c +++ b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c @@ -50,6 +50,7 @@ struct kfunc_test_params { #define SYSCALL_TEST(name, retval) __BPF_TEST_SUCCESS(name, retval, syscall_test) #define SYSCALL_NULL_CTX_TEST(name, retval) __BPF_TEST_SUCCESS(name, retval, syscall_null_ctx_test)

+#define TC_FAIL(name, retval, error_msg) __BPF_TEST_FAIL(name, retval, tc_test, error_msg) #define SYSCALL_NULL_CTX_FAIL(name, retval, error_msg) \ __BPF_TEST_FAIL(name, retval, syscall_null_ctx_test, error_msg)

@@ -62,11 +63,17 @@ static struct kfunc_test_params kfunc_tests[] = { */ SYSCALL_NULL_CTX_FAIL(kfunc_syscall_test_fail, -EINVAL, "processed 4 insns"), SYSCALL_NULL_CTX_FAIL(kfunc_syscall_test_null_fail, -EINVAL, "processed 4 insns"),

•   TC_FAIL(kfunc_call_test_get_mem_fail_rdonly, 0, "R0 cannot write into rdonly_mem"),
  

•   TC_FAIL(kfunc_call_test_get_mem_fail_use_after_free, 0, "invalid mem access 'scalar'"),
  

•   TC_FAIL(kfunc_call_test_get_mem_fail_oob, 0, "min value is outside of the allowed memory range"),
  

•   TC_FAIL(kfunc_call_test_get_mem_fail_not_const, 0, "is not a const"),
  

•   TC_FAIL(kfunc_call_test_mem_acquire_fail, 0, "acquire kernel function does not return PTR_TO_BTF_ID"),
  
    /* success cases */
    TC_TEST(kfunc_call_test1, 12),
    TC_TEST(kfunc_call_test2, 3),
    TC_TEST(kfunc_call_test_ref_btf_id, 0),
  

•   TC_TEST(kfunc_call_test_get_mem, 42),
    SYSCALL_TEST(kfunc_syscall_test, 0),
    SYSCALL_NULL_CTX_TEST(kfunc_syscall_test_null, 0),
  

}; diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_fail.c b/tools/testing/selftests/bpf/progs/kfunc_call_fail.c index 4168027f2ab1..b98313d391c6 100644 --- a/tools/testing/selftests/bpf/progs/kfunc_call_fail.c +++ b/tools/testing/selftests/bpf/progs/kfunc_call_fail.c @@ -3,7 +3,13 @@ #include <vmlinux.h> #include <bpf/bpf_helpers.h>

+extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym; +extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym; extern void bpf_kfunc_call_test_mem_len_pass1(void *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; +extern int *bpf_kfunc_call_test_acq_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym; +extern void bpf_kfunc_call_int_mem_release(int *p) __ksym;

struct syscall_test_args { __u8 data[16]; @@ -36,4 +42,119 @@ int kfunc_syscall_test_null_fail(struct syscall_test_args *args) return 0; }

+SEC("?tc") +int kfunc_call_test_get_mem_fail_rdonly(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) {
  

•           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_fail_use_after_free(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) {
  

•           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 */
  

•   return ret;
  

+}

+SEC("?tc") +int kfunc_call_test_get_mem_fail_oob(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) {
  

•           p = bpf_kfunc_call_test_get_rdonly_mem(pt, 2 * sizeof(int));
  

•           if (p)
  

•                   ret = p[2 * sizeof(int)]; /* oob access, so -EACCES */
  

•           else
  

•                   ret = -1;
  

•           bpf_kfunc_call_test_release(pt);
  

•   }
  

•   return ret;
  

+}

+int not_const_size = 2 * sizeof(int);

+SEC("?tc") +int kfunc_call_test_get_mem_fail_not_const(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) {
  

•           p = bpf_kfunc_call_test_get_rdonly_mem(pt, not_const_size); /* non const size, -EINVAL */
  

•           if (p)
  

•                   ret = p[0];
  

•           else
  

•                   ret = -1;
  

•           bpf_kfunc_call_test_release(pt);
  

•   }
  

•   return ret;
  

+}

+SEC("?tc") +int kfunc_call_test_mem_acquire_fail(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) {
  

•           /* we are failing on this one, because we are not acquiring a PTR_TO_BTF_ID (a struct ptr) */
  

•           p = bpf_kfunc_call_test_acq_rdonly_mem(pt, 2 * sizeof(int));
  

•           if (p)
  

•                   ret = p[0];
  

•           else
  

•                   ret = -1;
  

•           bpf_kfunc_call_int_mem_release(p);
  

•           bpf_kfunc_call_test_release(pt);
  

•   }
  

•   return ret;
  

+}

char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_test.c b/tools/testing/selftests/bpf/progs/kfunc_call_test.c index 94c05267e5e7..56c96f7969f0 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) @@ -130,4 +132,35 @@ int kfunc_syscall_test_null(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) {
  

•           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;
  

+}

char _license[] SEC("license") = "GPL";

2.36.1