[PATCH 2/2] selftests/bpf: add verifier test for PTR_TO_MEM spill

Subsystems: bpf [general] (safe dynamic programs and tools), bpf [selftests] (test runners & infrastructure), kernel selftest framework, the rest

STALE2047d

7 messages, 4 authors, 2021-01-12 · open the first message on its own page

[PATCH 2/2] selftests/bpf: add verifier test for PTR_TO_MEM spill

From: Gilad Reti <hidden>
Date: 2021-01-12 09:16:57

Add test to check that the verifier is able to recognize spilling of
PTR_TO_MEM registers.

The patch was partially contibuted by CyberArk Software, Inc.

Signed-off-by: Gilad Reti <redacted>
---
 tools/testing/selftests/bpf/test_verifier.c   | 12 +++++++-
 .../selftests/bpf/verifier/spill_fill.c       | 30 +++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 777a81404fdb..f8569f04064b 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -50,7 +50,7 @@
 #define MAX_INSNS	BPF_MAXINSNS
 #define MAX_TEST_INSNS	1000000
 #define MAX_FIXUPS	8
-#define MAX_NR_MAPS	20
+#define MAX_NR_MAPS	21
 #define MAX_TEST_RUNS	8
 #define POINTER_VALUE	0xcafe4all
 #define TEST_DATA_LEN	64
@@ -87,6 +87,7 @@ struct bpf_test {
 	int fixup_sk_storage_map[MAX_FIXUPS];
 	int fixup_map_event_output[MAX_FIXUPS];
 	int fixup_map_reuseport_array[MAX_FIXUPS];
+	int fixup_map_ringbuf[MAX_FIXUPS];
 	const char *errstr;
 	const char *errstr_unpriv;
 	uint32_t insn_processed;
@@ -640,6 +641,7 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
 	int *fixup_sk_storage_map = test->fixup_sk_storage_map;
 	int *fixup_map_event_output = test->fixup_map_event_output;
 	int *fixup_map_reuseport_array = test->fixup_map_reuseport_array;
+	int *fixup_map_ringbuf = test->fixup_map_ringbuf;
 
 	if (test->fill_helper) {
 		test->fill_insns = calloc(MAX_TEST_INSNS, sizeof(struct bpf_insn));
@@ -817,6 +819,14 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
 			fixup_map_reuseport_array++;
 		} while (*fixup_map_reuseport_array);
 	}
+	if (*fixup_map_ringbuf) {
+		map_fds[20] = create_map(BPF_MAP_TYPE_RINGBUF, 0,
+					   0, 4096);
+		do {
+			prog[*fixup_map_ringbuf].imm = map_fds[20];
+			fixup_map_ringbuf++;
+		} while (*fixup_map_ringbuf);
+	}
 }
 
 struct libcap {
diff --git a/tools/testing/selftests/bpf/verifier/spill_fill.c b/tools/testing/selftests/bpf/verifier/spill_fill.c
index 45d43bf82f26..1833b6c730dd 100644
--- a/tools/testing/selftests/bpf/verifier/spill_fill.c
+++ b/tools/testing/selftests/bpf/verifier/spill_fill.c
@@ -28,6 +28,36 @@
 	.result = ACCEPT,
 	.result_unpriv = ACCEPT,
 },
+{
+	"check valid spill/fill, ptr to mem",
+	.insns = {
+	/* reserve 8 byte ringbuf memory */
+	BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
+	BPF_LD_MAP_FD(BPF_REG_1, 0),
+	BPF_MOV64_IMM(BPF_REG_2, 8),
+	BPF_MOV64_IMM(BPF_REG_3, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_reserve),
+	/* store a pointer to the reserved memory in R6 */
+	BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+	/* check whether the reservation was successful */
+	BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 6),
+	/* spill R6(mem) into the stack */
+	BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_6, -8),
+	/* fill it back in R7 */
+	BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_10, -8),
+	/* should be able to access *(R7) = 0 */
+	BPF_ST_MEM(BPF_DW, BPF_REG_7, 0, 0),
+	/* submit the reserved rungbuf memory */
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_7),
+	BPF_MOV64_IMM(BPF_REG_2, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_submit),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.fixup_map_ringbuf = { 1 },
+	.result = ACCEPT,
+	.result_unpriv = ACCEPT,
+},
 {
 	"check corrupted spill/fill",
 	.insns = {
-- 
2.27.0

Re: [PATCH 2/2] selftests/bpf: add verifier test for PTR_TO_MEM spill

From: KP Singh <kpsingh@kernel.org>
Date: 2021-01-12 14:57:20

On Tue, Jan 12, 2021 at 10:16 AM Gilad Reti [off-list ref] wrote:
Add test to check that the verifier is able to recognize spilling of
PTR_TO_MEM registers.
It would be nice to have some explanation of what the test does to
recognize the spilling of the PTR_TO_MEM registers in the commit
log as well.

Would it be possible to augment an existing test_progs
program like tools/testing/selftests/bpf/progs/test_ringbuf.c to test
this functionality?


quoted hunk
The patch was partially contibuted by CyberArk Software, Inc.

Signed-off-by: Gilad Reti <redacted>
---
 tools/testing/selftests/bpf/test_verifier.c   | 12 +++++++-
 .../selftests/bpf/verifier/spill_fill.c       | 30 +++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 777a81404fdb..f8569f04064b 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -50,7 +50,7 @@
 #define MAX_INSNS      BPF_MAXINSNS
 #define MAX_TEST_INSNS 1000000
 #define MAX_FIXUPS     8
-#define MAX_NR_MAPS    20
+#define MAX_NR_MAPS    21
 #define MAX_TEST_RUNS  8
 #define POINTER_VALUE  0xcafe4all
 #define TEST_DATA_LEN  64
@@ -87,6 +87,7 @@ struct bpf_test {
        int fixup_sk_storage_map[MAX_FIXUPS];
        int fixup_map_event_output[MAX_FIXUPS];
        int fixup_map_reuseport_array[MAX_FIXUPS];
+       int fixup_map_ringbuf[MAX_FIXUPS];
        const char *errstr;
        const char *errstr_unpriv;
        uint32_t insn_processed;
@@ -640,6 +641,7 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
        int *fixup_sk_storage_map = test->fixup_sk_storage_map;
        int *fixup_map_event_output = test->fixup_map_event_output;
        int *fixup_map_reuseport_array = test->fixup_map_reuseport_array;
+       int *fixup_map_ringbuf = test->fixup_map_ringbuf;

        if (test->fill_helper) {
                test->fill_insns = calloc(MAX_TEST_INSNS, sizeof(struct bpf_insn));
@@ -817,6 +819,14 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
                        fixup_map_reuseport_array++;
                } while (*fixup_map_reuseport_array);
        }
+       if (*fixup_map_ringbuf) {
+               map_fds[20] = create_map(BPF_MAP_TYPE_RINGBUF, 0,
+                                          0, 4096);
+               do {
+                       prog[*fixup_map_ringbuf].imm = map_fds[20];
+                       fixup_map_ringbuf++;
+               } while (*fixup_map_ringbuf);
+       }
 }

 struct libcap {
diff --git a/tools/testing/selftests/bpf/verifier/spill_fill.c b/tools/testing/selftests/bpf/verifier/spill_fill.c
index 45d43bf82f26..1833b6c730dd 100644
--- a/tools/testing/selftests/bpf/verifier/spill_fill.c
+++ b/tools/testing/selftests/bpf/verifier/spill_fill.c
@@ -28,6 +28,36 @@
        .result = ACCEPT,
        .result_unpriv = ACCEPT,
 },
+{
+       "check valid spill/fill, ptr to mem",
+       .insns = {
+       /* reserve 8 byte ringbuf memory */
+       BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
+       BPF_LD_MAP_FD(BPF_REG_1, 0),
+       BPF_MOV64_IMM(BPF_REG_2, 8),
+       BPF_MOV64_IMM(BPF_REG_3, 0),
+       BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_reserve),
+       /* store a pointer to the reserved memory in R6 */
+       BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+       /* check whether the reservation was successful */
+       BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 6),
+       /* spill R6(mem) into the stack */
+       BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_6, -8),
+       /* fill it back in R7 */
+       BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_10, -8),
+       /* should be able to access *(R7) = 0 */
+       BPF_ST_MEM(BPF_DW, BPF_REG_7, 0, 0),
+       /* submit the reserved rungbuf memory */
+       BPF_MOV64_REG(BPF_REG_1, BPF_REG_7),
+       BPF_MOV64_IMM(BPF_REG_2, 0),
+       BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_submit),
+       BPF_MOV64_IMM(BPF_REG_0, 0),
+       BPF_EXIT_INSN(),
+       },
+       .fixup_map_ringbuf = { 1 },
+       .result = ACCEPT,
+       .result_unpriv = ACCEPT,
+},
 {
        "check corrupted spill/fill",
        .insns = {
--
2.27.0

Re: [PATCH 2/2] selftests/bpf: add verifier test for PTR_TO_MEM spill

From: Gilad Reti <hidden>
Date: 2021-01-12 15:37:18

On Tue, Jan 12, 2021 at 4:56 PM KP Singh [off-list ref] wrote:
On Tue, Jan 12, 2021 at 10:16 AM Gilad Reti [off-list ref] wrote:
quoted
Add test to check that the verifier is able to recognize spilling of
PTR_TO_MEM registers.
It would be nice to have some explanation of what the test does to
recognize the spilling of the PTR_TO_MEM registers in the commit
log as well.

Would it be possible to augment an existing test_progs
program like tools/testing/selftests/bpf/progs/test_ringbuf.c to test
this functionality?
It may be possible, but from what I understood from Daniel's comment here

https://lore.kernel.org/bpf/17629073-4fab-a922-ecc3-25b019960f44@iogearbox.net/

the test should be a part of the verifier tests (which is reasonable
to me since it is
a verifier bugfix)
quoted
The patch was partially contibuted by CyberArk Software, Inc.

Signed-off-by: Gilad Reti <redacted>
---
 tools/testing/selftests/bpf/test_verifier.c   | 12 +++++++-
 .../selftests/bpf/verifier/spill_fill.c       | 30 +++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 777a81404fdb..f8569f04064b 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -50,7 +50,7 @@
 #define MAX_INSNS      BPF_MAXINSNS
 #define MAX_TEST_INSNS 1000000
 #define MAX_FIXUPS     8
-#define MAX_NR_MAPS    20
+#define MAX_NR_MAPS    21
 #define MAX_TEST_RUNS  8
 #define POINTER_VALUE  0xcafe4all
 #define TEST_DATA_LEN  64
@@ -87,6 +87,7 @@ struct bpf_test {
        int fixup_sk_storage_map[MAX_FIXUPS];
        int fixup_map_event_output[MAX_FIXUPS];
        int fixup_map_reuseport_array[MAX_FIXUPS];
+       int fixup_map_ringbuf[MAX_FIXUPS];
        const char *errstr;
        const char *errstr_unpriv;
        uint32_t insn_processed;
@@ -640,6 +641,7 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
        int *fixup_sk_storage_map = test->fixup_sk_storage_map;
        int *fixup_map_event_output = test->fixup_map_event_output;
        int *fixup_map_reuseport_array = test->fixup_map_reuseport_array;
+       int *fixup_map_ringbuf = test->fixup_map_ringbuf;

        if (test->fill_helper) {
                test->fill_insns = calloc(MAX_TEST_INSNS, sizeof(struct bpf_insn));
@@ -817,6 +819,14 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
                        fixup_map_reuseport_array++;
                } while (*fixup_map_reuseport_array);
        }
+       if (*fixup_map_ringbuf) {
+               map_fds[20] = create_map(BPF_MAP_TYPE_RINGBUF, 0,
+                                          0, 4096);
+               do {
+                       prog[*fixup_map_ringbuf].imm = map_fds[20];
+                       fixup_map_ringbuf++;
+               } while (*fixup_map_ringbuf);
+       }
 }

 struct libcap {
diff --git a/tools/testing/selftests/bpf/verifier/spill_fill.c b/tools/testing/selftests/bpf/verifier/spill_fill.c
index 45d43bf82f26..1833b6c730dd 100644
--- a/tools/testing/selftests/bpf/verifier/spill_fill.c
+++ b/tools/testing/selftests/bpf/verifier/spill_fill.c
@@ -28,6 +28,36 @@
        .result = ACCEPT,
        .result_unpriv = ACCEPT,
 },
+{
+       "check valid spill/fill, ptr to mem",
+       .insns = {
+       /* reserve 8 byte ringbuf memory */
+       BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
+       BPF_LD_MAP_FD(BPF_REG_1, 0),
+       BPF_MOV64_IMM(BPF_REG_2, 8),
+       BPF_MOV64_IMM(BPF_REG_3, 0),
+       BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_reserve),
+       /* store a pointer to the reserved memory in R6 */
+       BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+       /* check whether the reservation was successful */
+       BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 6),
+       /* spill R6(mem) into the stack */
+       BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_6, -8),
+       /* fill it back in R7 */
+       BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_10, -8),
+       /* should be able to access *(R7) = 0 */
+       BPF_ST_MEM(BPF_DW, BPF_REG_7, 0, 0),
+       /* submit the reserved rungbuf memory */
+       BPF_MOV64_REG(BPF_REG_1, BPF_REG_7),
+       BPF_MOV64_IMM(BPF_REG_2, 0),
+       BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_submit),
+       BPF_MOV64_IMM(BPF_REG_0, 0),
+       BPF_EXIT_INSN(),
+       },
+       .fixup_map_ringbuf = { 1 },
+       .result = ACCEPT,
+       .result_unpriv = ACCEPT,
+},
 {
        "check corrupted spill/fill",
        .insns = {
--
2.27.0

Re: [PATCH 2/2] selftests/bpf: add verifier test for PTR_TO_MEM spill

From: Daniel Borkmann <daniel@iogearbox.net>
Date: 2021-01-12 15:44:20

On 1/12/21 4:35 PM, Gilad Reti wrote:
On Tue, Jan 12, 2021 at 4:56 PM KP Singh [off-list ref] wrote:
quoted
On Tue, Jan 12, 2021 at 10:16 AM Gilad Reti [off-list ref] wrote:
quoted
Add test to check that the verifier is able to recognize spilling of
PTR_TO_MEM registers.
It would be nice to have some explanation of what the test does to
recognize the spilling of the PTR_TO_MEM registers in the commit
log as well.

Would it be possible to augment an existing test_progs
program like tools/testing/selftests/bpf/progs/test_ringbuf.c to test
this functionality?
How would you guarantee that LLVM generates the spill/fill, via inline asm?
It may be possible, but from what I understood from Daniel's comment here

https://lore.kernel.org/bpf/17629073-4fab-a922-ecc3-25b019960f44@iogearbox.net/

the test should be a part of the verifier tests (which is reasonable
to me since it is
a verifier bugfix)
Yeah, the test_verifier case as you have is definitely the most straight
forward way to add coverage in this case.

Re: [PATCH 2/2] selftests/bpf: add verifier test for PTR_TO_MEM spill

From: KP Singh <kpsingh@kernel.org>
Date: 2021-01-12 16:19:05

On Tue, Jan 12, 2021 at 4:43 PM Daniel Borkmann [off-list ref] wrote:
On 1/12/21 4:35 PM, Gilad Reti wrote:
quoted
On Tue, Jan 12, 2021 at 4:56 PM KP Singh [off-list ref] wrote:
quoted
On Tue, Jan 12, 2021 at 10:16 AM Gilad Reti [off-list ref] wrote:
quoted
Add test to check that the verifier is able to recognize spilling of
PTR_TO_MEM registers.
It would be nice to have some explanation of what the test does to
recognize the spilling of the PTR_TO_MEM registers in the commit
log as well.

Would it be possible to augment an existing test_progs
program like tools/testing/selftests/bpf/progs/test_ringbuf.c to test
this functionality?
How would you guarantee that LLVM generates the spill/fill, via inline asm?
Yeah, I guess there is no sure-shot way to do it and, adding inline asm would
just be doing the same thing as this verifier test. You can ignore me
on this one :)

It would, however, be nice to have a better description about what the test is
actually doing./

quoted
It may be possible, but from what I understood from Daniel's comment here

https://lore.kernel.org/bpf/17629073-4fab-a922-ecc3-25b019960f44@iogearbox.net/

the test should be a part of the verifier tests (which is reasonable
to me since it is
a verifier bugfix)
Yeah, the test_verifier case as you have is definitely the most straight
forward way to add coverage in this case.

Re: [PATCH 2/2] selftests/bpf: add verifier test for PTR_TO_MEM spill

From: Gilad Reti <hidden>
Date: 2021-01-12 16:25:52

On Tue, Jan 12, 2021 at 6:17 PM KP Singh [off-list ref] wrote:
On Tue, Jan 12, 2021 at 4:43 PM Daniel Borkmann [off-list ref] wrote:
quoted
On 1/12/21 4:35 PM, Gilad Reti wrote:
quoted
On Tue, Jan 12, 2021 at 4:56 PM KP Singh [off-list ref] wrote:
quoted
On Tue, Jan 12, 2021 at 10:16 AM Gilad Reti [off-list ref] wrote:
quoted
Add test to check that the verifier is able to recognize spilling of
PTR_TO_MEM registers.
It would be nice to have some explanation of what the test does to
recognize the spilling of the PTR_TO_MEM registers in the commit
log as well.

Would it be possible to augment an existing test_progs
program like tools/testing/selftests/bpf/progs/test_ringbuf.c to test
this functionality?
How would you guarantee that LLVM generates the spill/fill, via inline asm?
Yeah, I guess there is no sure-shot way to do it and, adding inline asm would
just be doing the same thing as this verifier test. You can ignore me
on this one :)

It would, however, be nice to have a better description about what the test is
actually doing./
I will re-submit the patch tomorrow. Thank you all for your patience.
quoted
quoted
It may be possible, but from what I understood from Daniel's comment here

https://lore.kernel.org/bpf/17629073-4fab-a922-ecc3-25b019960f44@iogearbox.net/

the test should be a part of the verifier tests (which is reasonable
to me since it is
a verifier bugfix)
Yeah, the test_verifier case as you have is definitely the most straight
forward way to add coverage in this case.

Re: [PATCH 2/2] selftests/bpf: add verifier test for PTR_TO_MEM spill

From: Yonghong Song <hidden>
Date: 2021-01-12 17:00:45


On 1/12/21 7:43 AM, Daniel Borkmann wrote:
On 1/12/21 4:35 PM, Gilad Reti wrote:
quoted
On Tue, Jan 12, 2021 at 4:56 PM KP Singh [off-list ref] wrote:
quoted
On Tue, Jan 12, 2021 at 10:16 AM Gilad Reti [off-list ref] 
wrote:
quoted
Add test to check that the verifier is able to recognize spilling of
PTR_TO_MEM registers.
It would be nice to have some explanation of what the test does to
recognize the spilling of the PTR_TO_MEM registers in the commit
log as well.

Would it be possible to augment an existing test_progs
program like tools/testing/selftests/bpf/progs/test_ringbuf.c to test
this functionality?
How would you guarantee that LLVM generates the spill/fill, via inline asm?
You can make the following change to force the return value ("sample" 
here) of bpf_ringbuf_reserve() to spill on the stack.
diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf.c 
b/tools/testing/selftests/bpf/progs/test_ringbuf.c
index 8ba9959b036b..011521170856 100644
--- a/tools/testing/selftests/bpf/progs/test_ringbuf.c
+++ b/tools/testing/selftests/bpf/progs/test_ringbuf.c
@@ -40,7 +40,7 @@ SEC("tp/syscalls/sys_enter_getpgid")
  int test_ringbuf(void *ctx)
  {
         int cur_pid = bpf_get_current_pid_tgid() >> 32;
-       struct sample *sample;
+       struct sample * volatile sample;
         int zero = 0;

         if (cur_pid != pid)
This change will cause verifier failure without Patch #1.
quoted
It may be possible, but from what I understood from Daniel's comment here

https://lore.kernel.org/bpf/17629073-4fab-a922-ecc3-25b019960f44@iogearbox.net/ 


the test should be a part of the verifier tests (which is reasonable
to me since it is
a verifier bugfix)
Yeah, the test_verifier case as you have is definitely the most straight
forward way to add coverage in this case.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help