diff mbox series

[bpf,v2,2/2] bpf: new verifier tests for stack access

Message ID 20231126015045.1092826-3-andreimatei1@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series bpf: fix accesses to uninit stack slots | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/codegen success Generated files up to date
netdev/tree_selection success Clearly marked for bpf
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers warning 14 maintainers not CCed: song@kernel.org ast@kernel.org martin.lau@linux.dev daniel@iogearbox.net kpsingh@kernel.org john.fastabend@gmail.com sdf@google.com haoluo@google.com andrii@kernel.org yonghong.song@linux.dev mykolal@fb.com linux-kselftest@vger.kernel.org jolsa@kernel.org shuah@kernel.org
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch fail CHECK: Comparison to NULL could be written "!s" ERROR: code indent should use tabs where possible WARNING: 'unitialized' may be misspelled - perhaps 'uninitialized'? WARNING: Missing or malformed SPDX-License-Identifier tag in line 1 WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? WARNING: adding a line without newline at end of file WARNING: line length of 82 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns WARNING: line length of 90 exceeds 80 columns WARNING: please, no spaces at the start of a line
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-VM_Test-2 success Logs for Validate matrix.py
bpf/vmtest-bpf-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-3 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-4 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-6 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-8 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-VM_Test-7 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-5 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-9 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-VM_Test-14 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-VM_Test-15 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-16 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-18 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-17 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-20 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-19 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-21 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-22 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-24 success Logs for x86_64-llvm-16 / build / build for x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-23 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-25 success Logs for x86_64-llvm-16 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-26 success Logs for x86_64-llvm-16 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-27 success Logs for x86_64-llvm-16 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-28 success Logs for x86_64-llvm-16 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-29 success Logs for x86_64-llvm-16 / veristat
bpf/vmtest-bpf-VM_Test-13 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-VM_Test-12 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-VM_Test-11 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-PR success PR summary
bpf/vmtest-bpf-VM_Test-10 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc

Commit Message

Andrei Matei Nov. 26, 2023, 1:50 a.m. UTC 
This patch adds tests for the previous patch, checking the tracking of
the maximum stack size and checking that accesses to uninit stack memory
are allowed.

They are a separate patch for review purposes; whoever merges them can
consider squashing.

Signed-off-by: Andrei Matei <andreimatei1@gmail.com>
---
 tools/testing/selftests/bpf/test_verifier.c  | 24 ++++++++++++
 tools/testing/selftests/bpf/verifier/stack.c | 40 ++++++++++++++++++++
 2 files changed, 64 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/verifier/stack.c

Comments

Eduard Zingerman Nov. 28, 2023, 1:23 a.m. UTC | #1 
On Sat, 2023-11-25 at 20:50 -0500, Andrei Matei wrote:
> This patch adds tests for the previous patch, checking the tracking of
> the maximum stack size and checking that accesses to uninit stack memory
> are allowed.
> 
> They are a separate patch for review purposes; whoever merges them can
> consider squashing.
> 
> Signed-off-by: Andrei Matei <andreimatei1@gmail.com>
> ---

I think the strategy now is to add new tests using inline assembly,
e.g. as a part of verifier_* tests in test_progs.
Andrei Matei Nov. 28, 2023, 3:15 a.m. UTC | #2 
On Mon, Nov 27, 2023 at 8:23 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Sat, 2023-11-25 at 20:50 -0500, Andrei Matei wrote:
> > This patch adds tests for the previous patch, checking the tracking of
> > the maximum stack size and checking that accesses to uninit stack memory
> > are allowed.
> >
> > They are a separate patch for review purposes; whoever merges them can
> > consider squashing.
> >
> > Signed-off-by: Andrei Matei <andreimatei1@gmail.com>
> > ---
>
> I think the strategy now is to add new tests using inline assembly,
> e.g. as a part of verifier_* tests in test_progs.

Thanks, I'll try that. I see in some of the verifier tests that you
have converted to test_progs hints that they were converted
"automatically". I'm curious what tool you've used, if any.
Do you have any thoughts on how a test could assert the maximum stack
depth? test_verifier has access to the verifier verifier log and
parses it out of there; do you know how I could achieve the same in
test_progs?

For the curiosity of someone who doesn't know  much about this code
base - how come we moved from test_verifier, which seems a bit more
unit-test-y, do the higher level test_progs? Is it because of the
nicer assembly syntax?
Eduard Zingerman Nov. 28, 2023, 12:55 p.m. UTC | #3 
On Mon, 2023-11-27 at 22:15 -0500, Andrei Matei wrote:
> On Mon, Nov 27, 2023 at 8:23 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Sat, 2023-11-25 at 20:50 -0500, Andrei Matei wrote:
> > > This patch adds tests for the previous patch, checking the tracking of
> > > the maximum stack size and checking that accesses to uninit stack memory
> > > are allowed.
> > > 
> > > They are a separate patch for review purposes; whoever merges them can
> > > consider squashing.
> > > 
> > > Signed-off-by: Andrei Matei <andreimatei1@gmail.com>
> > > ---
> > 
> > I think the strategy now is to add new tests using inline assembly,
> > e.g. as a part of verifier_* tests in test_progs.
> 
> Thanks, I'll try that. I see in some of the verifier tests that you
> have converted to test_progs hints that they were converted
> "automatically". I'm curious what tool you've used, if any.

I wrote a small tree-sitter based tool for this:
https://github.com/eddyz87/verifier-tests-migrator

> Do you have any thoughts on how a test could assert the maximum stack
> depth? test_verifier has access to the verifier verifier log and
> parses it out of there; do you know how I could achieve the same in
> test_progs?

Could be done like in the patch below (set log level so that stack
depth is printed, match stack depth message):

diff --git a/tools/testing/selftests/bpf/progs/verifier_basic_stack.c b/tools/testing/selftests/bpf/progs/verifier_basic_stack.c
index 069c3f91705c..e85ac95c8dd3 100644
--- a/tools/testing/selftests/bpf/progs/verifier_basic_stack.c
+++ b/tools/testing/selftests/bpf/progs/verifier_basic_stack.c
@@ -27,7 +27,7 @@ __naked void stack_out_of_bounds(void)
 
 SEC("socket")
 __description("uninitialized stack1")
-__success
+__success __log_level(4) __msg("stack depth 8")
 __failure_unpriv __msg_unpriv("invalid indirect read from stack")
 __naked void uninitialized_stack1(void)
 {

> For the curiosity of someone who doesn't know  much about this code
> base - how come we moved from test_verifier, which seems a bit more
> unit-test-y, do the higher level test_progs? Is it because of the
> nicer assembly syntax?

Yes, ability to use assembly syntax is the main (sole?) driver.
In fact, tests that use annotations from bpf_misc.h and are registered
in tools/testing/selftests/bpf/prog_tests/verifier.c provide almost
the same functionality as test_verifier:
- interface:
  - select test to run using filter, e.g.:
    ./test_progs -a 'verifier_basic_stack/uninitialized stack1'
    ./test_progs -a 'verifier_basic_stack/uninitialized stack1 @unpriv'
  - see verifier log for the test, e.g.:
    ./test_progs -vvv -a 'verifier_basic_stack/uninitialized stack1'
- test expectations:
  - use __success / __failure to mark expected test outcome;
  - use __msg to search for expected messages in the log;
  - use __log_level to specify log level when program is loaded;
  - use __flags to enable additional knobs, e.g. BPF_F_TEST_STATE_FREQ;
  - use __retval if test program has to be executed;
  - use _unpriv suffix to specify any of the above for when test is
    executed in unprivileged mode.
  See comments in tools/testing/selftests/bpf/progs/bpf_misc.h for details.
- plus clang generates BTF and CO-RE relocations for us,
  with test_verifier one has to write these things "by hand"
  (which is only truly needed in tests for CO-RE/BTF).

The only drawback is compilation time, because all test_progs/*.c
files depend on all *.bpf.o files. Locally I mitigate this by using
ccache: make CC='ccache clang' LLVM=1 -j14 test_progs .
I probably need to look at what could be done in the makefile one day.

Unfortunately neither test_verifier nor test_progs are true unit tests.
Andrii Nakryiko Nov. 29, 2023, 6:12 a.m. UTC | #4 
On Tue, Nov 28, 2023 at 4:55 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Mon, 2023-11-27 at 22:15 -0500, Andrei Matei wrote:
> > On Mon, Nov 27, 2023 at 8:23 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > >
> > > On Sat, 2023-11-25 at 20:50 -0500, Andrei Matei wrote:
> > > > This patch adds tests for the previous patch, checking the tracking of
> > > > the maximum stack size and checking that accesses to uninit stack memory
> > > > are allowed.
> > > >
> > > > They are a separate patch for review purposes; whoever merges them can
> > > > consider squashing.
> > > >
> > > > Signed-off-by: Andrei Matei <andreimatei1@gmail.com>
> > > > ---
> > >
> > > I think the strategy now is to add new tests using inline assembly,
> > > e.g. as a part of verifier_* tests in test_progs.
> >
> > Thanks, I'll try that. I see in some of the verifier tests that you
> > have converted to test_progs hints that they were converted
> > "automatically". I'm curious what tool you've used, if any.
>
> I wrote a small tree-sitter based tool for this:
> https://github.com/eddyz87/verifier-tests-migrator
>
> > Do you have any thoughts on how a test could assert the maximum stack
> > depth? test_verifier has access to the verifier verifier log and
> > parses it out of there; do you know how I could achieve the same in
> > test_progs?
>
> Could be done like in the patch below (set log level so that stack
> depth is printed, match stack depth message):
>
> diff --git a/tools/testing/selftests/bpf/progs/verifier_basic_stack.c b/tools/testing/selftests/bpf/progs/verifier_basic_stack.c
> index 069c3f91705c..e85ac95c8dd3 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_basic_stack.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_basic_stack.c
> @@ -27,7 +27,7 @@ __naked void stack_out_of_bounds(void)
>
>  SEC("socket")
>  __description("uninitialized stack1")
> -__success
> +__success __log_level(4) __msg("stack depth 8")
>  __failure_unpriv __msg_unpriv("invalid indirect read from stack")
>  __naked void uninitialized_stack1(void)
>  {
>
> > For the curiosity of someone who doesn't know  much about this code
> > base - how come we moved from test_verifier, which seems a bit more
> > unit-test-y, do the higher level test_progs? Is it because of the
> > nicer assembly syntax?
>
> Yes, ability to use assembly syntax is the main (sole?) driver.

Not just that, test_progs by itself is a much nicer test runner for
debugging tests. It's easier to get log_level 2 logs, for example.
It's much easier to select a subset of tests to run. Plus by having
these BPF programs compiled as stand-alone .bpf.o objects, we can use
veristat to test them, bypassing test_progs altogether.

> In fact, tests that use annotations from bpf_misc.h and are registered
> in tools/testing/selftests/bpf/prog_tests/verifier.c provide almost
> the same functionality as test_verifier:
> - interface:
>   - select test to run using filter, e.g.:
>     ./test_progs -a 'verifier_basic_stack/uninitialized stack1'
>     ./test_progs -a 'verifier_basic_stack/uninitialized stack1 @unpriv'
>   - see verifier log for the test, e.g.:
>     ./test_progs -vvv -a 'verifier_basic_stack/uninitialized stack1'
> - test expectations:
>   - use __success / __failure to mark expected test outcome;
>   - use __msg to search for expected messages in the log;
>   - use __log_level to specify log level when program is loaded;
>   - use __flags to enable additional knobs, e.g. BPF_F_TEST_STATE_FREQ;
>   - use __retval if test program has to be executed;
>   - use _unpriv suffix to specify any of the above for when test is
>     executed in unprivileged mode.
>   See comments in tools/testing/selftests/bpf/progs/bpf_misc.h for details.
> - plus clang generates BTF and CO-RE relocations for us,
>   with test_verifier one has to write these things "by hand"
>   (which is only truly needed in tests for CO-RE/BTF).
>
> The only drawback is compilation time, because all test_progs/*.c
> files depend on all *.bpf.o files. Locally I mitigate this by using
> ccache: make CC='ccache clang' LLVM=1 -j14 test_progs .
> I probably need to look at what could be done in the makefile one day.

I think if we make a push to use BPF skeletons in all tests, we should
be able to get proper dependencies between prog_tests/xxx.c and its
corresponding progs/yyy.bpf.o file(s). And at which point we should be
able to teach Makefile to only recompile relevant test files.

>
> Unfortunately neither test_verifier nor test_progs are true unit tests.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 98107e0452d3..a62610585ee4 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -135,6 +135,10 @@  struct bpf_test {
 	const char *errstr;
 	const char *errstr_unpriv;
 	uint32_t insn_processed;
+	/* Expected maximum stack depth for the main subprogram. Not checked if 0.
+	 * Only checked if the program is accepted.
+	 */
+	uint16_t max_stack_depth;
 	int prog_len;
 	enum {
 		UNDEF,
@@ -1703,6 +1707,26 @@  static void do_test_single(struct bpf_test *test, bool unpriv,
 		}
 	}
 
+	/* Check the stack size if the test configured an expecation and the program
+	 * was loaded successfully.
+	 */
+	if (test->max_stack_depth && fd_prog >= 0) {
+		uint32_t max_stack;
+		char *s;
+
+		s = strstr(bpf_vlog, "stack depth ");
+		if (s == NULL) {
+			printf("FAIL\nstack depth result not found in verifier output\n");
+			goto fail_log;
+		}
+		max_stack = atoi(s + 12);
+		if (test->max_stack_depth != max_stack) {
+			printf("FAIL\nUnexpected max stack %u vs %u\n",
+			       max_stack, test->max_stack_depth);
+			goto fail_log;
+		}
+	}
+
 	if (verbose)
 		printf(", verifier log:\n%s", bpf_vlog);
 
diff --git a/tools/testing/selftests/bpf/verifier/stack.c b/tools/testing/selftests/bpf/verifier/stack.c
new file mode 100644
index 000000000000..ac571783c05e
--- /dev/null
+++ b/tools/testing/selftests/bpf/verifier/stack.c
@@ -0,0 +1,40 @@ 
+{
+	/* Check that reading unitialized stack memory is allowed only in privileged
+	 * mode. Also check that such reads maintain the max stack depth.
+	 */
+	"read uninit stack",
+	.insns = {
+		BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_10, -504),
+		/* exit(0); */
+		BPF_MOV32_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+	.result_unpriv = REJECT,
+	.errstr_unpriv = "invalid read from stack",
+    .max_stack_depth = 504,
+},
+{
+    /* Check that indirect accesses to stack maintain the max stack depth. */
+	"read (indirect) uninit stack",
+	.insns = {
+		/* We'll use probe_read_user as an arbitrary helper that can access the
+		 * stack. We're going to read into *(fp-104).
+		 */
+		BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
+		BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -104),
+		BPF_MOV32_IMM(BPF_REG_2, 8),
+        /* read from a random address */
+		BPF_MOV32_IMM(BPF_REG_3, 0x4242),
+        BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_probe_read_user),
+        BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+	    BPF_EXIT_INSN(),
+		/* exit(0); */
+		BPF_MOV32_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+	.result_unpriv = REJECT,
+	.errstr_unpriv = "",
+    .max_stack_depth = 104,
+},
\ No newline at end of file