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 gilad.reti@gmail.com --- 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 = {
On Tue, Jan 12, 2021 at 10:16 AM Gilad Reti gilad.reti@gmail.com 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?
The patch was partially contibuted by CyberArk Software, Inc.
Signed-off-by: Gilad Reti gilad.reti@gmail.com
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
On Tue, Jan 12, 2021 at 4:56 PM KP Singh kpsingh@kernel.org wrote:
On Tue, Jan 12, 2021 at 10:16 AM Gilad Reti gilad.reti@gmail.com 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?
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.n...
the test should be a part of the verifier tests (which is reasonable to me since it is a verifier bugfix)
The patch was partially contibuted by CyberArk Software, Inc.
Signed-off-by: Gilad Reti gilad.reti@gmail.com
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
On 1/12/21 4:35 PM, Gilad Reti wrote:
On Tue, Jan 12, 2021 at 4:56 PM KP Singh kpsingh@kernel.org wrote:
On Tue, Jan 12, 2021 at 10:16 AM Gilad Reti gilad.reti@gmail.com 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?
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.n...
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.
On Tue, Jan 12, 2021 at 4:43 PM Daniel Borkmann daniel@iogearbox.net wrote:
On 1/12/21 4:35 PM, Gilad Reti wrote:
On Tue, Jan 12, 2021 at 4:56 PM KP Singh kpsingh@kernel.org wrote:
On Tue, Jan 12, 2021 at 10:16 AM Gilad Reti gilad.reti@gmail.com 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?
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./
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.n...
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.
On Tue, Jan 12, 2021 at 6:17 PM KP Singh kpsingh@kernel.org wrote:
On Tue, Jan 12, 2021 at 4:43 PM Daniel Borkmann daniel@iogearbox.net wrote:
On 1/12/21 4:35 PM, Gilad Reti wrote:
On Tue, Jan 12, 2021 at 4:56 PM KP Singh kpsingh@kernel.org wrote:
On Tue, Jan 12, 2021 at 10:16 AM Gilad Reti gilad.reti@gmail.com 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?
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.
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.n...
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.
On 1/12/21 7:43 AM, Daniel Borkmann wrote:
On 1/12/21 4:35 PM, Gilad Reti wrote:
On Tue, Jan 12, 2021 at 4:56 PM KP Singh kpsingh@kernel.org wrote:
On Tue, Jan 12, 2021 at 10:16 AM Gilad Reti gilad.reti@gmail.com 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?
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.
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.n...
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.
linux-kselftest-mirror@lists.linaro.org
-
Daniel Borkmann -
Gilad Reti -
KP Singh -
Yonghong Song