This patch set aims to remove opcode checks in BPF verifier that have become redundant since commit 5e581dad4fec ("bpf: make unknown opcode handling more robust"), either remove them entirely, or turn them into comments in places where the redundancy may not be clear.
The exceptions here are opcode check for BPF_LD_{ABS,IND} and BPF_JMP_{JA,CALL,EXIT}; they cover opcode validation not done in bpf_opcode_in_insntable() so is not removed.
After apply the patch set test_verifier passes and does not need further modification: Summary: 1348 PASSED, 635 SKIPPED, 0 FAILED
Also, add comments at places that I find confusing while working on the removal, namely:
1. resolve_pseudo_ldimm64() also validates opcode 2. BPF_SIZE check in check_ld_imm() guards against JMP to the 2nd BPF_LD_IMM64 instruction 3. reason behind why ld_imm64 test cases should be rejected by the verifier
Shung-Hsi Yu (4): bpf: verifier: update resolve_pseudo_ldimm64() comment bpf: verifier: explain opcode check in check_ld_imm() bpf: verifier: remove redundant opcode checks selftests/bpf: add reason of rejection in ld_imm64
kernel/bpf/verifier.c | 33 ++++++++----------- .../testing/selftests/bpf/verifier/ld_imm64.c | 20 ++++++----- 2 files changed, 25 insertions(+), 28 deletions(-)
base-commit: 68084a13642001b73aade05819584f18945f3297
It may not be immediately clear why that ld_imm64 test cases are rejected, especially for test1 and test2 where JMP to the 2nd instruction of BPF_LD_IMM64 is performed.
Add brief explaination of why each test case in verifier/ld_imm64.c should be rejected.
Signed-off-by: Shung-Hsi Yu shung-hsi.yu@suse.com --- .../testing/selftests/bpf/verifier/ld_imm64.c | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/bpf/verifier/ld_imm64.c b/tools/testing/selftests/bpf/verifier/ld_imm64.c index f9297900cea6..021312641aaf 100644 --- a/tools/testing/selftests/bpf/verifier/ld_imm64.c +++ b/tools/testing/selftests/bpf/verifier/ld_imm64.c @@ -1,5 +1,6 @@ +/* Note: BPF_LD_IMM64 is composed of two instructions of class BPF_LD */ { - "test1 ld_imm64", + "test1 ld_imm64: reject JMP to 2nd instruction of BPF_LD_IMM64", .insns = { BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 1), BPF_LD_IMM64(BPF_REG_0, 0), @@ -14,7 +15,7 @@ .result = REJECT, }, { - "test2 ld_imm64", + "test2 ld_imm64: reject JMP to 2nd instruction of BPF_LD_IMM64", .insns = { BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 1), BPF_LD_IMM64(BPF_REG_0, 0), @@ -28,7 +29,7 @@ .result = REJECT, }, { - "test3 ld_imm64", + "test3 ld_imm64: reject incomplete BPF_LD_IMM64 instruction", .insns = { BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 1), BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 0), @@ -42,7 +43,7 @@ .result = REJECT, }, { - "test4 ld_imm64", + "test4 ld_imm64: reject incomplete BPF_LD_IMM64 instruction", .insns = { BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 0), BPF_EXIT_INSN(), @@ -70,7 +71,7 @@ .retval = 1, }, { - "test8 ld_imm64", + "test8 ld_imm64: reject 1st off!=0", .insns = { BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 1, 1), BPF_RAW_INSN(0, 0, 0, 0, 1), @@ -80,7 +81,7 @@ .result = REJECT, }, { - "test9 ld_imm64", + "test9 ld_imm64: reject 2nd off!=0", .insns = { BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 1), BPF_RAW_INSN(0, 0, 0, 1, 1), @@ -90,7 +91,7 @@ .result = REJECT, }, { - "test10 ld_imm64", + "test10 ld_imm64: reject 2nd dst_reg!=0", .insns = { BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 1), BPF_RAW_INSN(0, BPF_REG_1, 0, 0, 1), @@ -100,7 +101,7 @@ .result = REJECT, }, { - "test11 ld_imm64", + "test11 ld_imm64: reject 2nd src_reg!=0", .insns = { BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 1), BPF_RAW_INSN(0, 0, BPF_REG_1, 0, 1), @@ -113,6 +114,7 @@ "test12 ld_imm64", .insns = { BPF_MOV64_IMM(BPF_REG_1, 0), + /* BPF_REG_1 is interpreted as BPF_PSEUDO_MAP_FD */ BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, BPF_REG_1, 0, 1), BPF_RAW_INSN(0, 0, 0, 0, 0), BPF_EXIT_INSN(), @@ -121,7 +123,7 @@ .result = REJECT, }, { - "test13 ld_imm64", + "test13 ld_imm64: 2nd src_reg!=0", .insns = { BPF_MOV64_IMM(BPF_REG_1, 0), BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, BPF_REG_1, 0, 1),
On 5/20/22 4:37 AM, Shung-Hsi Yu wrote:
It may not be immediately clear why that ld_imm64 test cases are rejected, especially for test1 and test2 where JMP to the 2nd instruction of BPF_LD_IMM64 is performed.
Add brief explaination of why each test case in verifier/ld_imm64.c should be rejected.
Signed-off-by: Shung-Hsi Yu shung-hsi.yu@suse.com
.../testing/selftests/bpf/verifier/ld_imm64.c | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/bpf/verifier/ld_imm64.c b/tools/testing/selftests/bpf/verifier/ld_imm64.c index f9297900cea6..021312641aaf 100644 --- a/tools/testing/selftests/bpf/verifier/ld_imm64.c +++ b/tools/testing/selftests/bpf/verifier/ld_imm64.c @@ -1,5 +1,6 @@ +/* Note: BPF_LD_IMM64 is composed of two instructions of class BPF_LD */
[...]LD | BPF_IMM | BPF_DW, 0, 0, 0, 0), @@ -42,7 +43,7 @@ .result = REJECT, }, {
- "test4 ld_imm64",
- "test4 ld_imm64: reject incomplete BPF_LD_IMM64 instruction", .insns = { BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 0), BPF_EXIT_INSN(),
@@ -70,7 +71,7 @@ .retval = 1, }, {
- "test8 ld_imm64",
- "test8 ld_imm64: reject 1st off!=0",
Let add some space like 'off != 0'. The same for some of later test names.
.insns = { BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 1, 1), BPF_RAW_INSN(0, 0, 0, 0, 1), @@ -80,7 +81,7 @@ .result = REJECT, }, {
- "test9 ld_imm64",
- "test9 ld_imm64: reject 2nd off!=0", .insns = { BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 1), BPF_RAW_INSN(0, 0, 0, 1, 1),
@@ -90,7 +91,7 @@ .result = REJECT, }, {
- "test10 ld_imm64",
- "test10 ld_imm64: reject 2nd dst_reg!=0", .insns = { BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 1), BPF_RAW_INSN(0, BPF_REG_1, 0, 0, 1),
@@ -100,7 +101,7 @@ .result = REJECT, }, {
- "test11 ld_imm64",
- "test11 ld_imm64: reject 2nd src_reg!=0", .insns = { BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 1), BPF_RAW_INSN(0, 0, BPF_REG_1, 0, 1),
@@ -113,6 +114,7 @@ "test12 ld_imm64", .insns = { BPF_MOV64_IMM(BPF_REG_1, 0),
- /* BPF_REG_1 is interpreted as BPF_PSEUDO_MAP_FD */ BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, BPF_REG_1, 0, 1), BPF_RAW_INSN(0, 0, 0, 0, 0), BPF_EXIT_INSN(),
@@ -121,7 +123,7 @@ .result = REJECT, }, {
- "test13 ld_imm64",
- "test13 ld_imm64: 2nd src_reg!=0", .insns = { BPF_MOV64_IMM(BPF_REG_1, 0), BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, BPF_REG_1, 0, 1),
On Fri, May 20, 2022 at 05:27:12PM -0700, Yonghong Song wrote:
On 5/20/22 4:37 AM, Shung-Hsi Yu wrote:
It may not be immediately clear why that ld_imm64 test cases are rejected, especially for test1 and test2 where JMP to the 2nd instruction of BPF_LD_IMM64 is performed.
Add brief explaination of why each test case in verifier/ld_imm64.c should be rejected.
Signed-off-by: Shung-Hsi Yu shung-hsi.yu@suse.com
.../testing/selftests/bpf/verifier/ld_imm64.c | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/bpf/verifier/ld_imm64.c b/tools/testing/selftests/bpf/verifier/ld_imm64.c index f9297900cea6..021312641aaf 100644 --- a/tools/testing/selftests/bpf/verifier/ld_imm64.c +++ b/tools/testing/selftests/bpf/verifier/ld_imm64.c @@ -1,5 +1,6 @@ +/* Note: BPF_LD_IMM64 is composed of two instructions of class BPF_LD */
[...]LD | BPF_IMM | BPF_DW, 0, 0, 0, 0), @@ -42,7 +43,7 @@ .result = REJECT, }, {
- "test4 ld_imm64",
- "test4 ld_imm64: reject incomplete BPF_LD_IMM64 instruction", .insns = { BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 0), BPF_EXIT_INSN(),
@@ -70,7 +71,7 @@ .retval = 1, }, {
- "test8 ld_imm64",
- "test8 ld_imm64: reject 1st off!=0",
Let add some space like 'off != 0'. The same for some of later test names.
Okay, will do that in the next version. Thanks!
.insns = { BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 1, 1), BPF_RAW_INSN(0, 0, 0, 0, 1), @@ -80,7 +81,7 @@ .result = REJECT, }, {
- "test9 ld_imm64",
- "test9 ld_imm64: reject 2nd off!=0", .insns = { BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 1), BPF_RAW_INSN(0, 0, 0, 1, 1),
@@ -90,7 +91,7 @@ .result = REJECT, }, {
- "test10 ld_imm64",
- "test10 ld_imm64: reject 2nd dst_reg!=0", .insns = { BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 1), BPF_RAW_INSN(0, BPF_REG_1, 0, 0, 1),
@@ -100,7 +101,7 @@ .result = REJECT, }, {
- "test11 ld_imm64",
- "test11 ld_imm64: reject 2nd src_reg!=0", .insns = { BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, 0, 0, 1), BPF_RAW_INSN(0, 0, BPF_REG_1, 0, 1),
@@ -113,6 +114,7 @@ "test12 ld_imm64", .insns = { BPF_MOV64_IMM(BPF_REG_1, 0),
- /* BPF_REG_1 is interpreted as BPF_PSEUDO_MAP_FD */ BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, BPF_REG_1, 0, 1), BPF_RAW_INSN(0, 0, 0, 0, 0), BPF_EXIT_INSN(),
@@ -121,7 +123,7 @@ .result = REJECT, }, {
- "test13 ld_imm64",
- "test13 ld_imm64: 2nd src_reg!=0", .insns = { BPF_MOV64_IMM(BPF_REG_1, 0), BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, BPF_REG_1, 0, 1),
linux-kselftest-mirror@lists.linaro.org
-
Shung-Hsi Yu -
Yonghong Song