From: Eduard Zingerman <eddyz87@gmail.com>
To: bpf@vger.kernel.org, ast@kernel.org
Cc: andrii@kernel.org, daniel@iogearbox.net, kernel-team@fb.com,
yhs@fb.com, memxor@gmail.com, ecree.xilinx@gmail.com,
Eduard Zingerman <eddyz87@gmail.com>
Subject: [PATCH bpf-next 2/7] selftests/bpf: test cases for regsafe() bug skipping check_id()
Date: Fri, 9 Dec 2022 15:57:28 +0200 [thread overview]
Message-ID: <20221209135733.28851-3-eddyz87@gmail.com> (raw)
In-Reply-To: <20221209135733.28851-1-eddyz87@gmail.com>
Under certain conditions it was possible for verifier.c:regsafe() to
skip check_id() call. This commit adds negative test cases previously
errorneously accepted as safe.
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
.../bpf/verifier/direct_packet_access.c | 54 +++++++++++++++++++
.../selftests/bpf/verifier/value_or_null.c | 49 +++++++++++++++++
2 files changed, 103 insertions(+)
diff --git a/tools/testing/selftests/bpf/verifier/direct_packet_access.c b/tools/testing/selftests/bpf/verifier/direct_packet_access.c
index 11acd1855acf..dce2e28aeb43 100644
--- a/tools/testing/selftests/bpf/verifier/direct_packet_access.c
+++ b/tools/testing/selftests/bpf/verifier/direct_packet_access.c
@@ -654,3 +654,57 @@
.result = ACCEPT,
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
},
+{
+ "direct packet access: test30 (check_id() in regsafe(), bad access)",
+ .insns = {
+ /* r9 = ctx */
+ BPF_MOV64_REG(BPF_REG_9, BPF_REG_1),
+ /* r7 = ktime_get_ns() */
+ BPF_EMIT_CALL(BPF_FUNC_ktime_get_ns),
+ BPF_MOV64_REG(BPF_REG_7, BPF_REG_0),
+ /* r6 = ktime_get_ns() */
+ BPF_EMIT_CALL(BPF_FUNC_ktime_get_ns),
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+ /* r2 = ctx->data
+ * r3 = ctx->data
+ * r4 = ctx->data_end
+ */
+ BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_9, offsetof(struct __sk_buff, data)),
+ BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_9, offsetof(struct __sk_buff, data)),
+ BPF_LDX_MEM(BPF_W, BPF_REG_4, BPF_REG_9, offsetof(struct __sk_buff, data_end)),
+ /* if r6 > 100 goto exit
+ * if r7 > 100 goto exit
+ */
+ BPF_JMP_IMM(BPF_JGT, BPF_REG_6, 100, 9),
+ BPF_JMP_IMM(BPF_JGT, BPF_REG_7, 100, 8),
+ /* r2 += r6 ; this forces assignment of ID to r2
+ * r2 += 1 ; get some fixed off for r2
+ * r3 += r7 ; this forces assignment of ID to r3
+ * r3 += 1 ; get some fixed off for r3
+ */
+ BPF_ALU64_REG(BPF_ADD, BPF_REG_2, BPF_REG_6),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, 1),
+ BPF_ALU64_REG(BPF_ADD, BPF_REG_3, BPF_REG_7),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, 1),
+ /* if r6 > r7 goto +1 ; no new information about the state is derived from
+ * ; this check, thus produced verifier states differ
+ * ; only in 'insn_idx'
+ * r2 = r3 ; optionally share ID between r2 and r3
+ */
+ BPF_JMP_REG(BPF_JNE, BPF_REG_6, BPF_REG_7, 1),
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_3),
+ /* if r3 > ctx->data_end goto exit */
+ BPF_JMP_REG(BPF_JGT, BPF_REG_3, BPF_REG_4, 1),
+ /* r5 = *(u8 *) (r2 - 1) ; access packet memory using r2,
+ * ; this is not always safe
+ */
+ BPF_LDX_MEM(BPF_B, BPF_REG_5, BPF_REG_2, -1),
+ /* exit(0) */
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .flags = BPF_F_TEST_STATE_FREQ,
+ .result = REJECT,
+ .errstr = "invalid access to packet, off=0 size=1, R2",
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+},
diff --git a/tools/testing/selftests/bpf/verifier/value_or_null.c b/tools/testing/selftests/bpf/verifier/value_or_null.c
index 3ecb70a3d939..52a8bca14f03 100644
--- a/tools/testing/selftests/bpf/verifier/value_or_null.c
+++ b/tools/testing/selftests/bpf/verifier/value_or_null.c
@@ -169,3 +169,52 @@
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
.result = ACCEPT,
},
+{
+ "MAP_VALUE_OR_NULL check_ids() in regsafe()",
+ .insns = {
+ BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
+ /* r9 = map_lookup_elem(...) */
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+ BPF_LD_MAP_FD(BPF_REG_1,
+ 0),
+ BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
+ BPF_MOV64_REG(BPF_REG_9, BPF_REG_0),
+ /* r8 = map_lookup_elem(...) */
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+ BPF_LD_MAP_FD(BPF_REG_1,
+ 0),
+ BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
+ BPF_MOV64_REG(BPF_REG_8, BPF_REG_0),
+ /* r7 = ktime_get_ns() */
+ BPF_EMIT_CALL(BPF_FUNC_ktime_get_ns),
+ BPF_MOV64_REG(BPF_REG_7, BPF_REG_0),
+ /* r6 = ktime_get_ns() */
+ BPF_EMIT_CALL(BPF_FUNC_ktime_get_ns),
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+ /* if r6 > r7 goto +1 ; no new information about the state is derived from
+ * ; this check, thus produced verifier states differ
+ * ; only in 'insn_idx'
+ * r9 = r8 ; optionally share ID between r9 and r8
+ */
+ BPF_JMP_REG(BPF_JGT, BPF_REG_6, BPF_REG_7, 1),
+ BPF_MOV64_REG(BPF_REG_9, BPF_REG_8),
+ /* if r9 == 0 goto <exit> */
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_9, 0, 1),
+ /* read map value via r8, this is not always
+ * safe because r8 might be not equal to r9.
+ */
+ BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_8, 0),
+ /* exit 0 */
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .flags = BPF_F_TEST_STATE_FREQ,
+ .fixup_map_hash_8b = { 3, 9 },
+ .result = REJECT,
+ .errstr = "R8 invalid mem access 'map_value_or_null'",
+ .result_unpriv = REJECT,
+ .errstr_unpriv = "",
+ .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
+},
--
2.34.1
next prev parent reply other threads:[~2022-12-09 13:58 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-09 13:57 [PATCH bpf-next 0/7] stricter register ID checking in regsafe() Eduard Zingerman
2022-12-09 13:57 ` [PATCH bpf-next 1/7] bpf: regsafe() must not skip check_ids() Eduard Zingerman
2022-12-14 0:35 ` Andrii Nakryiko
2022-12-14 13:25 ` Eduard Zingerman
2022-12-14 19:37 ` Andrii Nakryiko
2022-12-09 13:57 ` Eduard Zingerman [this message]
2022-12-09 13:57 ` [PATCH bpf-next 3/7] bpf: states_equal() must build idmap for all function frames Eduard Zingerman
2022-12-14 0:35 ` Andrii Nakryiko
2022-12-14 15:33 ` Eduard Zingerman
2022-12-14 17:24 ` Andrii Nakryiko
2022-12-09 13:57 ` [PATCH bpf-next 4/7] selftests/bpf: verify states_equal() maintains idmap across all frames Eduard Zingerman
2022-12-14 0:35 ` Andrii Nakryiko
2022-12-14 16:38 ` Eduard Zingerman
2022-12-14 17:10 ` Andrii Nakryiko
2022-12-09 13:57 ` [PATCH bpf-next 5/7] bpf: use check_ids() for active_lock comparison Eduard Zingerman
2022-12-09 13:57 ` [PATCH bpf-next 6/7] selftests/bpf: Add pruning test case for bpf_spin_lock Eduard Zingerman
2022-12-10 21:45 ` Alexei Starovoitov
2022-12-09 13:57 ` [PATCH bpf-next 7/7] selftests/bpf: test case for relaxed prunning of active_lock.id Eduard Zingerman
2022-12-10 21:50 ` [PATCH bpf-next 0/7] stricter register ID checking in regsafe() patchwork-bot+netdevbpf
2022-12-14 0:34 ` Andrii Nakryiko
2022-12-14 16:28 ` Eduard Zingerman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221209135733.28851-3-eddyz87@gmail.com \
--to=eddyz87@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=ecree.xilinx@gmail.com \
--cc=kernel-team@fb.com \
--cc=memxor@gmail.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.