From b4d7b52212043f860bdde61d88f06123e775efcb Mon Sep 17 00:00:00 2001 From: Dave Thaler Date: Thu, 25 Jan 2024 07:30:02 -0800 Subject: [PATCH 1/7] Deprecate legacy packet access instructions The BPF ISA specification does not define them, and just says "these instructions are deprecated and should no longer be used". To comply with that, this PR disables support for legacy packet access instructions by default. If some other project depends on them, options.legacy can be set to true. Signed-off-by: Dave Thaler --- src/asm_syntax.hpp | 2 +- src/asm_unmarshal.cpp | 19 +++++------ src/asm_unmarshal.hpp | 11 ++++--- src/config.hpp | 2 ++ src/ebpf_vm_isa.hpp | 4 +-- src/ebpf_yaml.cpp | 13 +++++--- src/main/check.cpp | 3 +- src/test/test_marshal.cpp | 67 +++++++++++++++++++++++++++++++-------- src/test/test_print.cpp | 2 +- src/test/test_verify.cpp | 25 +++++++++++---- 10 files changed, 103 insertions(+), 45 deletions(-) diff --git a/src/asm_syntax.hpp b/src/asm_syntax.hpp index c8e621161..eff038ba0 100644 --- a/src/asm_syntax.hpp +++ b/src/asm_syntax.hpp @@ -202,7 +202,7 @@ struct Mem { bool is_load{}; }; -/// A special instruction for checked access to packets; it is actually a +/// A deprecated instruction for checked access to packets; it is actually a /// function call, and analyzed as one, e.g., by scratching caller-saved /// registers after it is performed. struct Packet { diff --git a/src/asm_unmarshal.cpp b/src/asm_unmarshal.cpp index 4a6af8308..068f6a184 100644 --- a/src/asm_unmarshal.cpp +++ b/src/asm_unmarshal.cpp @@ -220,17 +220,13 @@ struct Unmarshaller { switch ((inst.opcode & INST_MODE_MASK) >> 5) { case 0: throw InvalidInstruction(pc, inst.opcode); case INST_ABS: - if (!isLD) - throw InvalidInstruction(pc, "ABS but not LD"); - if (width == 8) - note("invalid opcode LDABSDW"); + if (!thread_local_options.legacy || !isLD || (width == 8)) + throw InvalidInstruction(pc, inst.opcode); return Packet{.width = width, .offset = inst.imm, .regoffset = {}}; case INST_IND: - if (!isLD) - throw InvalidInstruction(pc, "IND but not LD"); - if (width == 8) - note("invalid opcode LDINDDW"); + if (!thread_local_options.legacy || !isLD || (width == 8)) + throw InvalidInstruction(pc, inst.opcode); return Packet{.width = width, .offset = inst.imm, .regoffset = Reg{inst.src}}; case INST_MEM: { @@ -529,8 +525,9 @@ struct Unmarshaller { } }; -std::variant unmarshal(const raw_program& raw_prog, vector>& notes) { +std::variant unmarshal(const raw_program& raw_prog, const ebpf_verifier_options_t* options, vector>& notes) { global_program_info = raw_prog.info; + thread_local_options = (options != nullptr) ? *options : ebpf_verifier_default_options; try { return Unmarshaller{notes, raw_prog.info}.unmarshal(raw_prog.prog, raw_prog.line_info); } catch (InvalidInstruction& arg) { @@ -540,9 +537,9 @@ std::variant unmarshal(const raw_program& raw_prog, } } -std::variant unmarshal(const raw_program& raw_prog) { +std::variant unmarshal(const raw_program& raw_prog, const ebpf_verifier_options_t* options) { vector> notes; - return unmarshal(raw_prog, notes); + return unmarshal(raw_prog, options, notes); } Call make_call(int imm, const ebpf_platform_t& platform) diff --git a/src/asm_unmarshal.hpp b/src/asm_unmarshal.hpp index 1089d1eb0..60c968cdc 100644 --- a/src/asm_unmarshal.hpp +++ b/src/asm_unmarshal.hpp @@ -15,10 +15,13 @@ /** Translate a sequence of eBPF instructions (elf binary format) to a sequence * of Instructions. * - * \param raw_prog is the input program to parse. - * \param notes is where errors and warnings are written to. + * \param[in] raw_prog The input program to parse. + * \param[in] options Verifier options. + * \param[out] notes Where errors and warnings are written to. * \return a sequence of instruction if successful, an error string otherwise. */ -std::variant unmarshal(const raw_program& raw_prog, std::vector>& notes); -std::variant unmarshal(const raw_program& raw_prog); +std::variant unmarshal(const raw_program& raw_prog, const ebpf_verifier_options_t* options, + std::vector>& notes); +std::variant unmarshal(const raw_program& raw_prog, + const ebpf_verifier_options_t* options); Call make_call(int func, const ebpf_platform_t& platform); diff --git a/src/config.hpp b/src/config.hpp index cf3065fd9..cd71dc5cb 100644 --- a/src/config.hpp +++ b/src/config.hpp @@ -20,6 +20,8 @@ struct ebpf_verifier_options_t { bool setup_constraints; bool dump_btf_types_json; + + bool legacy; // True to enable support for legacy packet access instructions. }; struct ebpf_verifier_stats_t { diff --git a/src/ebpf_vm_isa.hpp b/src/ebpf_vm_isa.hpp index ad308b9af..d734352ff 100644 --- a/src/ebpf_vm_isa.hpp +++ b/src/ebpf_vm_isa.hpp @@ -44,8 +44,8 @@ enum { INST_MODE_MASK = 0xe0, - INST_ABS = 1, - INST_IND = 2, + INST_ABS = 1, // Deprecated + INST_IND = 2, // Deprecated INST_MEM = 3, INST_LEN = 4, INST_MSH = 5, diff --git a/src/ebpf_yaml.cpp b/src/ebpf_yaml.cpp index 25b27a006..109eb2b70 100644 --- a/src/ebpf_yaml.cpp +++ b/src/ebpf_yaml.cpp @@ -322,8 +322,15 @@ ConformanceTestResult run_conformance_test_case(const std::vector& memo raw_program raw_prog{.prog = insts}; raw_prog.info.platform = &g_ebpf_platform_linux; + ebpf_verifier_options_t options = ebpf_verifier_default_options; + if (debug) { + options.print_failures = true; + options.print_invariants = true; + options.no_simplify = true; + } + // Convert the raw program section to a set of instructions. - std::variant prog_or_error = unmarshal(raw_prog); + std::variant prog_or_error = unmarshal(raw_prog, &options); if (std::holds_alternative(prog_or_error)) { std::cerr << "unmarshaling error at " << std::get(prog_or_error) << "\n"; return {}; @@ -331,12 +338,8 @@ ConformanceTestResult run_conformance_test_case(const std::vector& memo auto& prog = std::get(prog_or_error); - ebpf_verifier_options_t options = ebpf_verifier_default_options; if (debug) { print(prog, std::cout, {}); - options.print_failures = true; - options.print_invariants = true; - options.no_simplify = true; } try { diff --git a/src/main/check.cpp b/src/main/check.cpp index 8b91d6d03..7b809ae6c 100644 --- a/src/main/check.cpp +++ b/src/main/check.cpp @@ -67,6 +67,7 @@ int main(int argc, char** argv) { app.add_flag("-s", ebpf_verifier_options.strict, "Apply additional checks that would cause runtime failures"); app.add_flag("-v", verbose, "Print both invariants and failures"); bool no_division_by_zero = false; + app.add_flag("--legacy", ebpf_verifier_options.legacy, "Allow deprecated packet access instructions"); app.add_flag("--no-division-by-zero", no_division_by_zero, "Do not allow division by zero"); app.add_flag("--no-simplify", ebpf_verifier_options.no_simplify, "Do not simplify"); app.add_flag("--line-info", ebpf_verifier_options.print_line_info, "Print line information"); @@ -143,7 +144,7 @@ int main(int argc, char** argv) { raw_program raw_prog = raw_progs.back(); // Convert the raw program section to a set of instructions. - std::variant prog_or_error = unmarshal(raw_prog); + std::variant prog_or_error = unmarshal(raw_prog, &ebpf_verifier_options); if (std::holds_alternative(prog_or_error)) { std::cout << "unmarshaling error at " << std::get(prog_or_error) << "\n"; return 1; diff --git a/src/test/test_marshal.cpp b/src/test/test_marshal.cpp index ffe0c8d02..c5570ed61 100644 --- a/src/test/test_marshal.cpp +++ b/src/test/test_marshal.cpp @@ -8,11 +8,12 @@ // Verify that if we unmarshal an instruction and then re-marshal it, // we get what we expect. -static void compare_unmarshal_marshal(const ebpf_inst& ins, const ebpf_inst& expected_result) { +static void compare_unmarshal_marshal(const ebpf_inst& ins, const ebpf_inst& expected_result, + const ebpf_verifier_options_t* options = nullptr) { program_info info{.platform = &g_ebpf_platform_linux, .type = g_ebpf_platform_linux.get_program_type("unspec", "unspec")}; const ebpf_inst exit{.opcode = INST_OP_EXIT}; - InstructionSeq parsed = std::get(unmarshal(raw_program{"", "", {ins, exit, exit}, info})); + InstructionSeq parsed = std::get(unmarshal(raw_program{"", "", {ins, exit, exit}, info}, options)); REQUIRE(parsed.size() == 3); auto [_, single, _2] = parsed.front(); (void)_; // unused @@ -25,10 +26,11 @@ static void compare_unmarshal_marshal(const ebpf_inst& ins, const ebpf_inst& exp // Verify that if we marshal an instruction and then unmarshal it, // we get the original. -static void compare_marshal_unmarshal(const Instruction& ins, bool double_cmd = false) { +static void compare_marshal_unmarshal(const Instruction& ins, bool double_cmd = false, + const ebpf_verifier_options_t* options = nullptr) { program_info info{.platform = &g_ebpf_platform_linux, .type = g_ebpf_platform_linux.get_program_type("unspec", "unspec")}; - InstructionSeq parsed = std::get(unmarshal(raw_program{"", "", marshal(ins, 0), info})); + InstructionSeq parsed = std::get(unmarshal(raw_program{"", "", marshal(ins, 0), info}, options)); REQUIRE(parsed.size() == 1); auto [_, single, _2] = parsed.back(); (void)_; // unused @@ -39,15 +41,16 @@ static void compare_marshal_unmarshal(const Instruction& ins, bool double_cmd = static void check_marshal_unmarshal_fail(const Instruction& ins, std::string expected_error_message) { program_info info{.platform = &g_ebpf_platform_linux, .type = g_ebpf_platform_linux.get_program_type("unspec", "unspec")}; - std::string error_message = std::get(unmarshal(raw_program{"", "", marshal(ins, 0), info})); + std::string error_message = std::get(unmarshal(raw_program{"", "", marshal(ins, 0), info}, nullptr)); REQUIRE(error_message == expected_error_message); } -static void check_unmarshal_fail(ebpf_inst inst, std::string expected_error_message) { +static void check_unmarshal_fail(ebpf_inst inst, std::string expected_error_message, + const ebpf_verifier_options_t* options = nullptr) { program_info info{.platform = &g_ebpf_platform_linux, .type = g_ebpf_platform_linux.get_program_type("unspec", "unspec")}; std::vector insns = {inst}; - auto result = unmarshal(raw_program{"", "", insns, info}); + auto result = unmarshal(raw_program{"", "", insns, info}, options); REQUIRE(std::holds_alternative(result)); std::string error_message = std::get(result); REQUIRE(error_message == expected_error_message); @@ -141,9 +144,13 @@ TEST_CASE("disasm_marshal", "[disasm][marshal]") { SECTION("Exit") { compare_marshal_unmarshal(Exit{}); } SECTION("Packet") { + ebpf_verifier_options_t options = ebpf_verifier_default_options; + options.legacy = true; for (int w : ws) { - compare_marshal_unmarshal(Packet{.width = w, .offset = 7, .regoffset = {}}); - compare_marshal_unmarshal(Packet{.width = w, .offset = 7, .regoffset = Reg{2}}); + if (w != 8) { + compare_marshal_unmarshal(Packet{.width = w, .offset = 7, .regoffset = {}}, false, &options); + compare_marshal_unmarshal(Packet{.width = w, .offset = 7, .regoffset = Reg{2}}, false, &options); + } } } @@ -301,14 +308,48 @@ TEST_CASE("fail unmarshal offset opcodes", "[disasm][marshal]") { } } +TEST_CASE("check unmarshal legacy opcodes", "[disasm][marshal]") { + // The following opcodes are deprecated and should no longer be used. + static uint8_t supported_legacy_opcodes[] = {0x20, 0x28, 0x30, 0x40, 0x48, 0x50}; + static uint8_t unsupported_legacy_opcodes[] = {0x21, 0x22, 0x23, 0x29, 0x2a, 0x2b, 0x31, 0x32, 0x33, + 0x38, 0x39, 0x3a, 0x3b, 0x41, 0x42, 0x43, 0x49, 0x4a, + 0x4b, 0x51, 0x52, 0x53, 0x58, 0x59, 0x5a, 0x5b}; + ebpf_verifier_options_t options = ebpf_verifier_default_options; + + // Enable support. + options.legacy = true; + + for (uint8_t opcode : unsupported_legacy_opcodes) { + std::ostringstream oss; + printf("[0x%x]\n", opcode); + oss << "0: Bad instruction op 0x" << std::hex << (int)opcode << std::endl; + check_unmarshal_fail(ebpf_inst{.opcode = opcode}, oss.str().c_str(), &options); + } + + for (uint8_t opcode : supported_legacy_opcodes) { + printf("[0x%x]\n", opcode); + compare_unmarshal_marshal(ebpf_inst{.opcode = opcode}, ebpf_inst{.opcode = opcode}, &options); + } + + // Disable support. + options.legacy = false; + + for (uint8_t opcode : unsupported_legacy_opcodes) { + std::ostringstream oss; + oss << "0: Bad instruction op 0x" << std::hex << (int)opcode << std::endl; + check_unmarshal_fail(ebpf_inst{.opcode = opcode}, oss.str().c_str(), &options); + } + for (uint8_t opcode : supported_legacy_opcodes) { + std::ostringstream oss; + oss << "0: Bad instruction op 0x" << std::hex << (int)opcode << std::endl; + check_unmarshal_fail(ebpf_inst{.opcode = opcode}, oss.str().c_str(), &options); + } +} + TEST_CASE("fail unmarshal misc", "[disasm][marshal]") { check_unmarshal_fail(ebpf_inst{.opcode = /* 0x06 */ INST_CLS_JMP32}, "0: jump out of bounds\n"); check_unmarshal_fail(ebpf_inst{.opcode = /* 0x16 */ 0x10 | INST_CLS_JMP32}, "0: jump out of bounds\n"); check_unmarshal_fail(ebpf_inst{.opcode = /* 0x18 */ INST_OP_LDDW_IMM}, "0: incomplete LDDW\n"); - check_unmarshal_fail(ebpf_inst{.opcode = /* 0x21 */ (INST_ABS << 5) | INST_SIZE_W | INST_CLS_LDX, .imm = 8}, - "0: ABS but not LD\n"); - check_unmarshal_fail(ebpf_inst{.opcode = /* 0x41 */ (INST_IND << 5) | INST_SIZE_W | INST_CLS_LDX, .imm = 8}, - "0: IND but not LD\n"); check_unmarshal_fail(ebpf_inst{.opcode = /* 0x71 */ ((INST_MEM << 5) | INST_SIZE_B | INST_CLS_LDX), .dst = 11, .imm = 8}, "0: Bad register\n"); check_unmarshal_fail(ebpf_inst{.opcode = /* 0x71 */ ((INST_MEM << 5) | INST_SIZE_B | INST_CLS_LDX), .dst = 1, .src = 11}, diff --git a/src/test/test_print.cpp b/src/test/test_print.cpp index 5980c7852..60966d434 100644 --- a/src/test/test_print.cpp +++ b/src/test/test_print.cpp @@ -27,7 +27,7 @@ void verify_printed_string(const std::string& file) std::stringstream generated_output; auto raw_progs = read_elf(std::string(TEST_OBJECT_FILE_DIRECTORY) + file + ".o", "", nullptr, &g_ebpf_platform_linux); raw_program raw_prog = raw_progs.back(); - std::variant prog_or_error = unmarshal(raw_prog); + std::variant prog_or_error = unmarshal(raw_prog, nullptr); REQUIRE(std::holds_alternative(prog_or_error)); auto& program = std::get(prog_or_error); print(program, generated_output, {}); diff --git a/src/test/test_verify.cpp b/src/test/test_verify.cpp index 023d780cf..33fbbec07 100644 --- a/src/test/test_verify.cpp +++ b/src/test/test_verify.cpp @@ -24,7 +24,7 @@ FAIL_LOAD_ELF("invalid", "badsymsize.o", "xdp_redirect_map") auto raw_progs = read_elf("ebpf-samples/" dirname "/" filename, sectionname, nullptr, &g_ebpf_platform_linux); \ REQUIRE(raw_progs.size() == 1); \ raw_program raw_prog = raw_progs.back(); \ - std::variant prog_or_error = unmarshal(raw_prog); \ + std::variant prog_or_error = unmarshal(raw_prog, nullptr); \ REQUIRE(std::holds_alternative(prog_or_error)); \ } @@ -37,7 +37,7 @@ FAIL_UNMARSHAL("invalid", "invalid-lddw.o", ".text") auto raw_progs = read_elf("ebpf-samples/" dirname "/" filename, sectionname, nullptr, &g_ebpf_platform_linux); \ REQUIRE(raw_progs.size() == 1); \ raw_program raw_prog = raw_progs.back(); \ - std::variant prog_or_error = unmarshal(raw_prog); \ + std::variant prog_or_error = unmarshal(raw_prog, options); \ REQUIRE(std::holds_alternative(prog_or_error)); \ auto& prog = std::get(prog_or_error); \ bool res = ebpf_verify_program(std::cout, prog, raw_prog.info, options, nullptr); \ @@ -75,6 +75,15 @@ FAIL_UNMARSHAL("invalid", "invalid-lddw.o", ".text") VERIFY_SECTION(project, filename, section, nullptr, false); \ } +#define TEST_SECTION_LEGACY(project, filename, section) \ + TEST_CASE("./check ebpf-samples/" project "/" filename " " section, "[verify][samples][" project "]") { \ + ebpf_verifier_options_t options = ebpf_verifier_default_options; \ + options.legacy = true; \ + VERIFY_SECTION(project, filename, section, &options, true); \ + options.legacy = false; \ + VERIFY_SECTION(project, filename, section, &options, false); \ + } + TEST_SECTION("bpf_cilium_test", "bpf_lxc_jit.o", "1/0xdc06") TEST_SECTION("bpf_cilium_test", "bpf_lxc_jit.o", "2/1") TEST_SECTION("bpf_cilium_test", "bpf_lxc_jit.o", "2/3") @@ -191,7 +200,7 @@ TEST_SECTION("linux", "offwaketime_kern.o", "tracepoint/sched/sched_switch") TEST_SECTION("linux", "sampleip_kern.o", "perf_event") TEST_SECTION("linux", "sock_flags_kern.o", "cgroup/sock1") TEST_SECTION("linux", "sock_flags_kern.o", "cgroup/sock2") -TEST_SECTION("linux", "sockex1_kern.o", "socket1") +TEST_SECTION_LEGACY("linux", "sockex1_kern.o", "socket1") TEST_SECTION("linux", "sockex2_kern.o", "socket2") TEST_SECTION("linux", "sockex3_kern.o", "socket/3") TEST_SECTION("linux", "sockex3_kern.o", "socket/4") @@ -544,18 +553,20 @@ void test_analyze_thread(cfg_t* cfg, program_info* info, bool* res) { // Test multithreading TEST_CASE("multithreading", "[verify][multithreading]") { - auto raw_progs1 = read_elf("ebpf-samples/bpf_cilium_test/bpf_netdev.o", "2/1", nullptr, &g_ebpf_platform_linux); + ebpf_verifier_options_t* options = nullptr; + + auto raw_progs1 = read_elf("ebpf-samples/bpf_cilium_test/bpf_netdev.o", "2/1", options, &g_ebpf_platform_linux); REQUIRE(raw_progs1.size() == 1); raw_program raw_prog1 = raw_progs1.back(); - std::variant prog_or_error1 = unmarshal(raw_prog1); + std::variant prog_or_error1 = unmarshal(raw_prog1, options); REQUIRE(std::holds_alternative(prog_or_error1)); auto& prog1 = std::get(prog_or_error1); cfg_t cfg1 = prepare_cfg(prog1, raw_prog1.info, true); - auto raw_progs2 = read_elf("ebpf-samples/bpf_cilium_test/bpf_netdev.o", "2/2", nullptr, &g_ebpf_platform_linux); + auto raw_progs2 = read_elf("ebpf-samples/bpf_cilium_test/bpf_netdev.o", "2/2", options, &g_ebpf_platform_linux); REQUIRE(raw_progs2.size() == 1); raw_program raw_prog2 = raw_progs2.back(); - std::variant prog_or_error2 = unmarshal(raw_prog2); + std::variant prog_or_error2 = unmarshal(raw_prog2, options); REQUIRE(std::holds_alternative(prog_or_error2)); auto& prog2 = std::get(prog_or_error2); cfg_t cfg2 = prepare_cfg(prog2, raw_prog2.info, true); From b81f85c1a989a6293d97dcdbb39c73497c56022f Mon Sep 17 00:00:00 2001 From: Dave Thaler Date: Fri, 26 Jan 2024 12:19:52 -0800 Subject: [PATCH 2/7] Update tests Signed-off-by: Dave Thaler --- src/test/test_marshal.cpp | 2 -- src/test/test_verify.cpp | 36 ++++++++++++++++++------------------ 2 files changed, 18 insertions(+), 20 deletions(-) diff --git a/src/test/test_marshal.cpp b/src/test/test_marshal.cpp index c5570ed61..3d434817f 100644 --- a/src/test/test_marshal.cpp +++ b/src/test/test_marshal.cpp @@ -321,13 +321,11 @@ TEST_CASE("check unmarshal legacy opcodes", "[disasm][marshal]") { for (uint8_t opcode : unsupported_legacy_opcodes) { std::ostringstream oss; - printf("[0x%x]\n", opcode); oss << "0: Bad instruction op 0x" << std::hex << (int)opcode << std::endl; check_unmarshal_fail(ebpf_inst{.opcode = opcode}, oss.str().c_str(), &options); } for (uint8_t opcode : supported_legacy_opcodes) { - printf("[0x%x]\n", opcode); compare_unmarshal_marshal(ebpf_inst{.opcode = opcode}, ebpf_inst{.opcode = opcode}, &options); } diff --git a/src/test/test_verify.cpp b/src/test/test_verify.cpp index 33fbbec07..ce92b0bfd 100644 --- a/src/test/test_verify.cpp +++ b/src/test/test_verify.cpp @@ -91,7 +91,7 @@ TEST_SECTION("bpf_cilium_test", "bpf_lxc_jit.o", "2/4") TEST_SECTION("bpf_cilium_test", "bpf_lxc_jit.o", "2/5") TEST_SECTION("bpf_cilium_test", "bpf_lxc_jit.o", "2/6") TEST_SECTION("bpf_cilium_test", "bpf_lxc_jit.o", "2/7") -TEST_SECTION("bpf_cilium_test", "bpf_lxc_jit.o", "2/10") +TEST_SECTION_LEGACY("bpf_cilium_test", "bpf_lxc_jit.o", "2/10") TEST_SECTION("bpf_cilium_test", "bpf_lxc_jit.o", "from-container") TEST_SECTION("bpf_cilium_test", "bpf_lxc-DUNKNOWN.o", "1/0x1010") @@ -102,7 +102,7 @@ TEST_SECTION("bpf_cilium_test", "bpf_lxc-DUNKNOWN.o", "2/4") TEST_SECTION("bpf_cilium_test", "bpf_lxc-DUNKNOWN.o", "2/5") TEST_SECTION("bpf_cilium_test", "bpf_lxc-DUNKNOWN.o", "2/6") TEST_SECTION("bpf_cilium_test", "bpf_lxc-DUNKNOWN.o", "2/7") -TEST_SECTION("bpf_cilium_test", "bpf_lxc-DUNKNOWN.o", "from-container") +TEST_SECTION_LEGACY("bpf_cilium_test", "bpf_lxc-DUNKNOWN.o", "from-container") TEST_SECTION("bpf_cilium_test", "bpf_lxc-DDROP_ALL.o", "1/0x1010") TEST_SECTION("bpf_cilium_test", "bpf_lxc-DDROP_ALL.o", "2/1") @@ -120,7 +120,7 @@ TEST_SECTION("bpf_cilium_test", "bpf_netdev.o", "2/3") TEST_SECTION("bpf_cilium_test", "bpf_netdev.o", "2/4") TEST_SECTION("bpf_cilium_test", "bpf_netdev.o", "2/5") TEST_SECTION("bpf_cilium_test", "bpf_netdev.o", "2/7") -TEST_SECTION("bpf_cilium_test", "bpf_netdev.o", "from-netdev") +TEST_SECTION_LEGACY("bpf_cilium_test", "bpf_netdev.o", "from-netdev") TEST_SECTION("bpf_cilium_test", "bpf_overlay.o", "2/1") TEST_SECTION("bpf_cilium_test", "bpf_overlay.o", "2/2") @@ -129,7 +129,7 @@ TEST_SECTION("bpf_cilium_test", "bpf_overlay.o", "2/4") TEST_SECTION("bpf_cilium_test", "bpf_overlay.o", "2/5") TEST_SECTION("bpf_cilium_test", "bpf_overlay.o", "2/7") TEST_SECTION("bpf_cilium_test", "bpf_overlay.o", "3/2") -TEST_SECTION("bpf_cilium_test", "bpf_overlay.o", "from-overlay") +TEST_SECTION_LEGACY("bpf_cilium_test", "bpf_overlay.o", "from-overlay") TEST_SECTION("bpf_cilium_test", "bpf_lb-DLB_L3.o", "2/1") TEST_SECTION("bpf_cilium_test", "bpf_lb-DLB_L3.o", "2/2") @@ -155,7 +155,7 @@ TEST_SECTION("cilium", "bpf_lxc.o", "2/6") TEST_SECTION("cilium", "bpf_lxc.o", "2/7") TEST_SECTION("cilium", "bpf_lxc.o", "2/8") TEST_SECTION("cilium", "bpf_lxc.o", "2/9") -TEST_SECTION("cilium", "bpf_lxc.o", "2/10") +TEST_SECTION_LEGACY("cilium", "bpf_lxc.o", "2/10") TEST_SECTION("cilium", "bpf_lxc.o", "2/11") TEST_SECTION("cilium", "bpf_lxc.o", "2/12") TEST_SECTION("cilium", "bpf_lxc.o", "from-container") @@ -165,14 +165,14 @@ TEST_SECTION("cilium", "bpf_netdev.o", "2/3") TEST_SECTION("cilium", "bpf_netdev.o", "2/4") TEST_SECTION("cilium", "bpf_netdev.o", "2/5") TEST_SECTION("cilium", "bpf_netdev.o", "2/7") -TEST_SECTION("cilium", "bpf_netdev.o", "from-netdev") +TEST_SECTION_LEGACY("cilium", "bpf_netdev.o", "from-netdev") TEST_SECTION("cilium", "bpf_overlay.o", "2/1") TEST_SECTION("cilium", "bpf_overlay.o", "2/3") TEST_SECTION("cilium", "bpf_overlay.o", "2/4") TEST_SECTION("cilium", "bpf_overlay.o", "2/5") TEST_SECTION("cilium", "bpf_overlay.o", "2/7") -TEST_SECTION("cilium", "bpf_overlay.o", "from-overlay") +TEST_SECTION_LEGACY("cilium", "bpf_overlay.o", "from-overlay") TEST_SECTION("cilium", "bpf_xdp.o", "from-netdev") @@ -201,12 +201,12 @@ TEST_SECTION("linux", "sampleip_kern.o", "perf_event") TEST_SECTION("linux", "sock_flags_kern.o", "cgroup/sock1") TEST_SECTION("linux", "sock_flags_kern.o", "cgroup/sock2") TEST_SECTION_LEGACY("linux", "sockex1_kern.o", "socket1") -TEST_SECTION("linux", "sockex2_kern.o", "socket2") -TEST_SECTION("linux", "sockex3_kern.o", "socket/3") -TEST_SECTION("linux", "sockex3_kern.o", "socket/4") -TEST_SECTION("linux", "sockex3_kern.o", "socket/1") -TEST_SECTION("linux", "sockex3_kern.o", "socket/2") -TEST_SECTION("linux", "sockex3_kern.o", "socket/0") +TEST_SECTION_LEGACY("linux", "sockex2_kern.o", "socket2") +TEST_SECTION_LEGACY("linux", "sockex3_kern.o", "socket/3") +TEST_SECTION_LEGACY("linux", "sockex3_kern.o", "socket/4") +TEST_SECTION_LEGACY("linux", "sockex3_kern.o", "socket/1") +TEST_SECTION_LEGACY("linux", "sockex3_kern.o", "socket/2") +TEST_SECTION_LEGACY("linux", "sockex3_kern.o", "socket/0") TEST_SECTION("linux", "spintest_kern.o", "kprobe/__htab_percpu_map_update_elem") TEST_SECTION("linux", "spintest_kern.o", "kprobe/_raw_spin_lock") TEST_SECTION("linux", "spintest_kern.o", "kprobe/_raw_spin_lock_bh") @@ -236,7 +236,7 @@ TEST_SECTION("linux", "tcp_basertt_kern.o", "sockops") TEST_SECTION("linux", "tcp_bufs_kern.o", "sockops") TEST_SECTION("linux", "tcp_cong_kern.o", "sockops") TEST_SECTION("linux", "tcp_iw_kern.o", "sockops") -TEST_SECTION("linux", "tcbpf1_kern.o", "classifier") +TEST_SECTION_LEGACY("linux", "tcbpf1_kern.o", "classifier") TEST_SECTION("linux", "tcbpf1_kern.o", "clone_redirect_recv") TEST_SECTION("linux", "tcbpf1_kern.o", "clone_redirect_xmit") TEST_SECTION("linux", "tcbpf1_kern.o", "redirect_recv") @@ -344,7 +344,7 @@ TEST_SECTION("prototype-kernel", "xdp_vlan01_kern.o", "xdp_vlan_remove_outer2") TEST_SECTION("ovs", "datapath.o", "tail-0") TEST_SECTION("ovs", "datapath.o", "tail-1") TEST_SECTION("ovs", "datapath.o", "tail-2") -TEST_SECTION("ovs", "datapath.o", "tail-3") +TEST_SECTION_LEGACY("ovs", "datapath.o", "tail-3") TEST_SECTION("ovs", "datapath.o", "tail-4") TEST_SECTION("ovs", "datapath.o", "tail-5") TEST_SECTION("ovs", "datapath.o", "tail-7") @@ -352,7 +352,7 @@ TEST_SECTION("ovs", "datapath.o", "tail-8") TEST_SECTION("ovs", "datapath.o", "tail-11") TEST_SECTION("ovs", "datapath.o", "tail-12") TEST_SECTION("ovs", "datapath.o", "tail-13") -TEST_SECTION("ovs", "datapath.o", "tail-32") +TEST_SECTION_LEGACY("ovs", "datapath.o", "tail-32") TEST_SECTION("ovs", "datapath.o", "tail-33") TEST_SECTION("ovs", "datapath.o", "tail-35") TEST_SECTION("ovs", "datapath.o", "af_xdp") @@ -361,8 +361,8 @@ TEST_SECTION("ovs", "datapath.o", "egress") TEST_SECTION("ovs", "datapath.o", "ingress") TEST_SECTION("ovs", "datapath.o", "xdp") -TEST_SECTION("suricata", "bypass_filter.o", "filter") -TEST_SECTION("suricata", "lb.o", "loadbalancer") +TEST_SECTION_LEGACY("suricata", "bypass_filter.o", "filter") +TEST_SECTION_LEGACY("suricata", "lb.o", "loadbalancer") TEST_SECTION("suricata", "filter.o", "filter") TEST_SECTION("suricata", "vlan_filter.o", "filter") TEST_SECTION("suricata", "xdp_filter.o", "xdp") From 7fd2c49b12630da791c6c758f7768046fb80b084 Mon Sep 17 00:00:00 2001 From: Dave Thaler Date: Fri, 26 Jan 2024 12:58:15 -0800 Subject: [PATCH 3/7] Fix tests Signed-off-by: Dave Thaler --- src/test/test_verify.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/test/test_verify.cpp b/src/test/test_verify.cpp index ce92b0bfd..7ad81797d 100644 --- a/src/test/test_verify.cpp +++ b/src/test/test_verify.cpp @@ -20,7 +20,7 @@ FAIL_LOAD_ELF("build", "badrelo.o", ".text") FAIL_LOAD_ELF("invalid", "badsymsize.o", "xdp_redirect_map") #define FAIL_UNMARSHAL(dirname, filename, sectionname) \ - TEST_CASE("Try unmarshalling bad program: " dirname "/" filename, "[unmarshal]") { \ + TEST_CASE("Try unmarshalling bad program: " dirname "/" filename " " sectionname, "[unmarshal]") { \ auto raw_progs = read_elf("ebpf-samples/" dirname "/" filename, sectionname, nullptr, &g_ebpf_platform_linux); \ REQUIRE(raw_progs.size() == 1); \ raw_program raw_prog = raw_progs.back(); \ @@ -80,9 +80,8 @@ FAIL_UNMARSHAL("invalid", "invalid-lddw.o", ".text") ebpf_verifier_options_t options = ebpf_verifier_default_options; \ options.legacy = true; \ VERIFY_SECTION(project, filename, section, &options, true); \ - options.legacy = false; \ - VERIFY_SECTION(project, filename, section, &options, false); \ - } + } \ + FAIL_UNMARSHAL(project, filename, section) TEST_SECTION("bpf_cilium_test", "bpf_lxc_jit.o", "1/0xdc06") TEST_SECTION("bpf_cilium_test", "bpf_lxc_jit.o", "2/1") From 123c47e65991c475c8aed2d5a26ebaadf8ef50b0 Mon Sep 17 00:00:00 2001 From: Dave Thaler Date: Fri, 26 Jan 2024 13:49:19 -0800 Subject: [PATCH 4/7] Add YAML tests for packet access instructions Signed-off-by: Dave Thaler --- src/ebpf_yaml.cpp | 1 + test-data/packet.yaml | 102 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+) diff --git a/src/ebpf_yaml.cpp b/src/ebpf_yaml.cpp index 109eb2b70..f91eb437e 100644 --- a/src/ebpf_yaml.cpp +++ b/src/ebpf_yaml.cpp @@ -169,6 +169,7 @@ static ebpf_verifier_options_t raw_options_to_options(const std::set& ra // All YAML tests use no_simplify and !setup_constraints. options.no_simplify = true; options.setup_constraints = false; + options.legacy = true; for (const string& name : raw_options) { if (name == "!allow_division_by_zero") { diff --git a/test-data/packet.yaml b/test-data/packet.yaml index 4de7f6745..57c31c203 100644 --- a/test-data/packet.yaml +++ b/test-data/packet.yaml @@ -106,3 +106,105 @@ post: [ "r3.svalue=r3.uvalue", "r2.packet_offset-r3.packet_offset<=65526", "r3.packet_offset-r2.packet_offset<=8" ] +--- +test-case: legacy 1 byte packet access imm + +pre: [ + "r1.type=number", + "r6.type=ctx", "r6.ctx_offset=0" +] + +code: + : | + r0 = *(u8 *)skb[23] + +post: [ + "r0.type=number", + "r6.type=ctx", "r6.ctx_offset=0" +] +--- +test-case: legacy 2 byte packet access imm + +pre: [ + "r1.type=number", + "r6.type=ctx", "r6.ctx_offset=0" +] + +code: + : | + r0 = *(u16 *)skb[23] + +post: [ + "r0.type=number", + "r6.type=ctx", "r6.ctx_offset=0" +] +--- +test-case: legacy 4 byte packet access imm + +pre: [ + "r1.type=number", + "r6.type=ctx", "r6.ctx_offset=0" +] + +code: + : | + r0 = *(u32 *)skb[23] + +post: [ + "r0.type=number", + "r6.type=ctx", "r6.ctx_offset=0" +] +--- +test-case: legacy 1 byte packet access reg + +pre: [ + "r1.type=number", + "r6.type=ctx", "r6.ctx_offset=0", + "r7.type=number", "r7.svalue=23", "r7.uvalue=23" +] + +code: + : | + r0 = *(u8 *)skb[r7] + +post: [ + "r0.type=number", + "r6.type=ctx", "r6.ctx_offset=0", + "r7.type=number", "r7.svalue=23", "r7.uvalue=23" +] +--- +test-case: legacy 2 byte packet access reg + +pre: [ + "r1.type=number", + "r6.type=ctx", "r6.ctx_offset=0", + "r7.type=number", "r7.svalue=23", "r7.uvalue=23" +] + +code: + : | + r0 = *(u16 *)skb[r7] + +post: [ + "r0.type=number", + "r6.type=ctx", "r6.ctx_offset=0", + "r7.type=number", "r7.svalue=23", "r7.uvalue=23" +] +--- +test-case: legacy 4 byte packet access reg + +pre: [ + "r1.type=number", + "r6.type=ctx", "r6.ctx_offset=0", + "r7.type=number", "r7.svalue=23", "r7.uvalue=23" +] + +code: + : | + r0 = *(u32 *)skb[r7] + +post: [ + "r0.type=number", + "r6.type=ctx", "r6.ctx_offset=0", + "r7.type=number", "r7.svalue=23", "r7.uvalue=23" +] From 9516e0ef1c72a522925863187b90835b8874dc80 Mon Sep 17 00:00:00 2001 From: Dave Thaler Date: Sun, 4 Feb 2024 10:52:43 -0800 Subject: [PATCH 5/7] Convert legacy option into a platform bool Signed-off-by: Dave Thaler --- src/asm_unmarshal.cpp | 4 +-- src/config.hpp | 2 -- src/ebpf_yaml.cpp | 4 +-- src/linux/linux_platform.cpp | 3 ++- src/main/check.cpp | 10 ++++--- src/platform.hpp | 3 +++ src/test/test_marshal.cpp | 51 ++++++++++++++++++------------------ src/test/test_verify.cpp | 34 +++++++++++++----------- 8 files changed, 59 insertions(+), 52 deletions(-) diff --git a/src/asm_unmarshal.cpp b/src/asm_unmarshal.cpp index 068f6a184..001bcc37c 100644 --- a/src/asm_unmarshal.cpp +++ b/src/asm_unmarshal.cpp @@ -220,12 +220,12 @@ struct Unmarshaller { switch ((inst.opcode & INST_MODE_MASK) >> 5) { case 0: throw InvalidInstruction(pc, inst.opcode); case INST_ABS: - if (!thread_local_options.legacy || !isLD || (width == 8)) + if (!info.platform->legacy || !isLD || (width == 8)) throw InvalidInstruction(pc, inst.opcode); return Packet{.width = width, .offset = inst.imm, .regoffset = {}}; case INST_IND: - if (!thread_local_options.legacy || !isLD || (width == 8)) + if (!info.platform->legacy || !isLD || (width == 8)) throw InvalidInstruction(pc, inst.opcode); return Packet{.width = width, .offset = inst.imm, .regoffset = Reg{inst.src}}; diff --git a/src/config.hpp b/src/config.hpp index cd71dc5cb..cf3065fd9 100644 --- a/src/config.hpp +++ b/src/config.hpp @@ -20,8 +20,6 @@ struct ebpf_verifier_options_t { bool setup_constraints; bool dump_btf_types_json; - - bool legacy; // True to enable support for legacy packet access instructions. }; struct ebpf_verifier_stats_t { diff --git a/src/ebpf_yaml.cpp b/src/ebpf_yaml.cpp index f91eb437e..f9d654eca 100644 --- a/src/ebpf_yaml.cpp +++ b/src/ebpf_yaml.cpp @@ -57,7 +57,8 @@ ebpf_platform_t g_platform_test = { .map_record_size = 0, .parse_maps_section = ebpf_parse_maps_section, .get_map_descriptor = ebpf_get_map_descriptor, - .get_map_type = ebpf_get_map_type + .get_map_type = ebpf_get_map_type, + .legacy = true, }; static EbpfProgramType make_program_type(const string& name, ebpf_context_descriptor_t* context_descriptor) { @@ -169,7 +170,6 @@ static ebpf_verifier_options_t raw_options_to_options(const std::set& ra // All YAML tests use no_simplify and !setup_constraints. options.no_simplify = true; options.setup_constraints = false; - options.legacy = true; for (const string& name : raw_options) { if (name == "!allow_division_by_zero") { diff --git a/src/linux/linux_platform.cpp b/src/linux/linux_platform.cpp index 85fb568af..1ae825cdc 100644 --- a/src/linux/linux_platform.cpp +++ b/src/linux/linux_platform.cpp @@ -254,5 +254,6 @@ const ebpf_platform_t g_ebpf_platform_linux = { parse_maps_section_linux, get_map_descriptor_linux, get_map_type_linux, - resolve_inner_map_references_linux + resolve_inner_map_references_linux, + true // Legacy packet access instructions }; diff --git a/src/main/check.cpp b/src/main/check.cpp index 7b809ae6c..85e244080 100644 --- a/src/main/check.cpp +++ b/src/main/check.cpp @@ -66,8 +66,9 @@ int main(int argc, char** argv) { app.add_flag("-f", ebpf_verifier_options.print_failures, "Print verifier's failure logs"); app.add_flag("-s", ebpf_verifier_options.strict, "Apply additional checks that would cause runtime failures"); app.add_flag("-v", verbose, "Print both invariants and failures"); + bool legacy = false; + app.add_flag("--legacy", legacy, "Allow deprecated packet access instructions"); bool no_division_by_zero = false; - app.add_flag("--legacy", ebpf_verifier_options.legacy, "Allow deprecated packet access instructions"); app.add_flag("--no-division-by-zero", no_division_by_zero, "Do not allow division by zero"); app.add_flag("--no-simplify", ebpf_verifier_options.no_simplify, "Do not simplify"); app.add_flag("--line-info", ebpf_verifier_options.print_line_info, "Print line information"); @@ -112,12 +113,13 @@ int main(int argc, char** argv) { if (domain == "linux") ebpf_verifier_options.mock_map_fds = false; - const ebpf_platform_t* platform = &g_ebpf_platform_linux; + ebpf_platform_t platform = g_ebpf_platform_linux; + platform.legacy = legacy; // Read a set of raw program sections from an ELF file. vector raw_progs; try { - raw_progs = read_elf(filename, desired_section, &ebpf_verifier_options, platform); + raw_progs = read_elf(filename, desired_section, &ebpf_verifier_options, &platform); } catch (std::runtime_error& e) { std::cerr << "error: " << e.what() << std::endl; return 1; @@ -131,7 +133,7 @@ int main(int argc, char** argv) { if (!desired_section.empty() && raw_progs.empty()) { // We could not find the desired section, so get the full list // of possibilities. - raw_progs = read_elf(filename, string(), &ebpf_verifier_options, platform); + raw_progs = read_elf(filename, string(), &ebpf_verifier_options, &platform); } for (const raw_program& raw_prog : raw_progs) { std::cout << raw_prog.section << " "; diff --git a/src/platform.hpp b/src/platform.hpp index 0816479ab..38ca90be5 100644 --- a/src/platform.hpp +++ b/src/platform.hpp @@ -42,6 +42,9 @@ struct ebpf_platform_t { ebpf_get_map_descriptor_fn get_map_descriptor; ebpf_get_map_type_fn get_map_type; ebpf_resolve_inner_map_references_fn resolve_inner_map_references; + + // Options indicating support for various instructions. + bool legacy; }; extern const ebpf_platform_t g_ebpf_platform_linux; diff --git a/src/test/test_marshal.cpp b/src/test/test_marshal.cpp index 3d434817f..6753af108 100644 --- a/src/test/test_marshal.cpp +++ b/src/test/test_marshal.cpp @@ -9,9 +9,10 @@ // Verify that if we unmarshal an instruction and then re-marshal it, // we get what we expect. static void compare_unmarshal_marshal(const ebpf_inst& ins, const ebpf_inst& expected_result, - const ebpf_verifier_options_t* options = nullptr) { - program_info info{.platform = &g_ebpf_platform_linux, - .type = g_ebpf_platform_linux.get_program_type("unspec", "unspec")}; + const ebpf_verifier_options_t* options = nullptr, + const ebpf_platform_t* platform = &g_ebpf_platform_linux) { + program_info info{.platform = platform, + .type = platform->get_program_type("unspec", "unspec")}; const ebpf_inst exit{.opcode = INST_OP_EXIT}; InstructionSeq parsed = std::get(unmarshal(raw_program{"", "", {ins, exit, exit}, info}, options)); REQUIRE(parsed.size() == 3); @@ -27,9 +28,10 @@ static void compare_unmarshal_marshal(const ebpf_inst& ins, const ebpf_inst& exp // Verify that if we marshal an instruction and then unmarshal it, // we get the original. static void compare_marshal_unmarshal(const Instruction& ins, bool double_cmd = false, - const ebpf_verifier_options_t* options = nullptr) { - program_info info{.platform = &g_ebpf_platform_linux, - .type = g_ebpf_platform_linux.get_program_type("unspec", "unspec")}; + const ebpf_verifier_options_t* options = nullptr, + const ebpf_platform_t* platform = &g_ebpf_platform_linux) { + program_info info{.platform = platform, + .type = platform->get_program_type("unspec", "unspec")}; InstructionSeq parsed = std::get(unmarshal(raw_program{"", "", marshal(ins, 0), info}, options)); REQUIRE(parsed.size() == 1); auto [_, single, _2] = parsed.back(); @@ -38,17 +40,19 @@ static void compare_marshal_unmarshal(const Instruction& ins, bool double_cmd = REQUIRE(single == ins); } -static void check_marshal_unmarshal_fail(const Instruction& ins, std::string expected_error_message) { - program_info info{.platform = &g_ebpf_platform_linux, - .type = g_ebpf_platform_linux.get_program_type("unspec", "unspec")}; +static void check_marshal_unmarshal_fail(const Instruction& ins, std::string expected_error_message, + const ebpf_platform_t* platform = &g_ebpf_platform_linux) { + program_info info{.platform = platform, + .type = platform->get_program_type("unspec", "unspec")}; std::string error_message = std::get(unmarshal(raw_program{"", "", marshal(ins, 0), info}, nullptr)); REQUIRE(error_message == expected_error_message); } static void check_unmarshal_fail(ebpf_inst inst, std::string expected_error_message, - const ebpf_verifier_options_t* options = nullptr) { - program_info info{.platform = &g_ebpf_platform_linux, - .type = g_ebpf_platform_linux.get_program_type("unspec", "unspec")}; + const ebpf_verifier_options_t* options = nullptr, + const ebpf_platform_t* platform = &g_ebpf_platform_linux) { + program_info info{.platform = platform, + .type = platform->get_program_type("unspec", "unspec")}; std::vector insns = {inst}; auto result = unmarshal(raw_program{"", "", insns, info}, options); REQUIRE(std::holds_alternative(result)); @@ -144,12 +148,10 @@ TEST_CASE("disasm_marshal", "[disasm][marshal]") { SECTION("Exit") { compare_marshal_unmarshal(Exit{}); } SECTION("Packet") { - ebpf_verifier_options_t options = ebpf_verifier_default_options; - options.legacy = true; for (int w : ws) { if (w != 8) { - compare_marshal_unmarshal(Packet{.width = w, .offset = 7, .regoffset = {}}, false, &options); - compare_marshal_unmarshal(Packet{.width = w, .offset = 7, .regoffset = Reg{2}}, false, &options); + compare_marshal_unmarshal(Packet{.width = w, .offset = 7, .regoffset = {}}); + compare_marshal_unmarshal(Packet{.width = w, .offset = 7, .regoffset = Reg{2}}); } } } @@ -314,33 +316,30 @@ TEST_CASE("check unmarshal legacy opcodes", "[disasm][marshal]") { static uint8_t unsupported_legacy_opcodes[] = {0x21, 0x22, 0x23, 0x29, 0x2a, 0x2b, 0x31, 0x32, 0x33, 0x38, 0x39, 0x3a, 0x3b, 0x41, 0x42, 0x43, 0x49, 0x4a, 0x4b, 0x51, 0x52, 0x53, 0x58, 0x59, 0x5a, 0x5b}; - ebpf_verifier_options_t options = ebpf_verifier_default_options; - - // Enable support. - options.legacy = true; for (uint8_t opcode : unsupported_legacy_opcodes) { std::ostringstream oss; oss << "0: Bad instruction op 0x" << std::hex << (int)opcode << std::endl; - check_unmarshal_fail(ebpf_inst{.opcode = opcode}, oss.str().c_str(), &options); + check_unmarshal_fail(ebpf_inst{.opcode = opcode}, oss.str().c_str()); } for (uint8_t opcode : supported_legacy_opcodes) { - compare_unmarshal_marshal(ebpf_inst{.opcode = opcode}, ebpf_inst{.opcode = opcode}, &options); + compare_unmarshal_marshal(ebpf_inst{.opcode = opcode}, ebpf_inst{.opcode = opcode}); } - // Disable support. - options.legacy = false; + // Disable legacy support. + ebpf_platform_t platform = g_ebpf_platform_linux; + platform.legacy = false; for (uint8_t opcode : unsupported_legacy_opcodes) { std::ostringstream oss; oss << "0: Bad instruction op 0x" << std::hex << (int)opcode << std::endl; - check_unmarshal_fail(ebpf_inst{.opcode = opcode}, oss.str().c_str(), &options); + check_unmarshal_fail(ebpf_inst{.opcode = opcode}, oss.str().c_str(), nullptr, &platform); } for (uint8_t opcode : supported_legacy_opcodes) { std::ostringstream oss; oss << "0: Bad instruction op 0x" << std::hex << (int)opcode << std::endl; - check_unmarshal_fail(ebpf_inst{.opcode = opcode}, oss.str().c_str(), &options); + check_unmarshal_fail(ebpf_inst{.opcode = opcode}, oss.str().c_str(), nullptr, &platform); } } diff --git a/src/test/test_verify.cpp b/src/test/test_verify.cpp index 7ad81797d..59a6963ee 100644 --- a/src/test/test_verify.cpp +++ b/src/test/test_verify.cpp @@ -32,9 +32,9 @@ FAIL_LOAD_ELF("invalid", "badsymsize.o", "xdp_redirect_map") FAIL_UNMARSHAL("build", "wronghelper.o", "xdp") FAIL_UNMARSHAL("invalid", "invalid-lddw.o", ".text") -#define VERIFY_SECTION(dirname, filename, sectionname, options, pass) \ +#define VERIFY_SECTION(dirname, filename, sectionname, options, platform, pass) \ do { \ - auto raw_progs = read_elf("ebpf-samples/" dirname "/" filename, sectionname, nullptr, &g_ebpf_platform_linux); \ + auto raw_progs = read_elf("ebpf-samples/" dirname "/" filename, sectionname, nullptr, platform); \ REQUIRE(raw_progs.size() == 1); \ raw_program raw_prog = raw_progs.back(); \ std::variant prog_or_error = unmarshal(raw_prog, options); \ @@ -49,39 +49,43 @@ FAIL_UNMARSHAL("invalid", "invalid-lddw.o", ".text") #define TEST_SECTION(project, filename, section) \ TEST_CASE("./check ebpf-samples/" project "/" filename " " section, "[verify][samples][" project "]") { \ - VERIFY_SECTION(project, filename, section, nullptr, true); \ + VERIFY_SECTION(project, filename, section, nullptr, &g_ebpf_platform_linux, true); \ } #define TEST_SECTION_REJECT(project, filename, section) \ TEST_CASE("./check ebpf-samples/" project "/" filename " " section, "[verify][samples][" project "]") { \ - VERIFY_SECTION(project, filename, section, nullptr, false); \ + VERIFY_SECTION(project, filename, section, nullptr, &g_ebpf_platform_linux, false); \ } #define TEST_SECTION_REJECT_IF_STRICT(project, filename, section) \ TEST_CASE("./check ebpf-samples/" project "/" filename " " section, "[verify][samples][" project "]") { \ ebpf_verifier_options_t options = ebpf_verifier_default_options; \ - VERIFY_SECTION(project, filename, section, &options, true); \ + VERIFY_SECTION(project, filename, section, &options, &g_ebpf_platform_linux, true); \ options.strict = true; \ - VERIFY_SECTION(project, filename, section, &options, false); \ + VERIFY_SECTION(project, filename, section, &options, &g_ebpf_platform_linux, false); \ } #define TEST_SECTION_FAIL(project, filename, section) \ TEST_CASE("expect failure ebpf-samples/" project "/" filename " " section, "[!shouldfail][verify][samples][" project "]") { \ - VERIFY_SECTION(project, filename, section, nullptr, true); \ + VERIFY_SECTION(project, filename, section, nullptr, &g_ebpf_platform_linux, true); \ } #define TEST_SECTION_REJECT_FAIL(project, filename, section) \ TEST_CASE("expect failure ebpf-samples/" project "/" filename " " section, "[!shouldfail][verify][samples][" project "]") { \ - VERIFY_SECTION(project, filename, section, nullptr, false); \ + VERIFY_SECTION(project, filename, section, nullptr, &g_ebpf_platform_linux, false); \ } -#define TEST_SECTION_LEGACY(project, filename, section) \ - TEST_CASE("./check ebpf-samples/" project "/" filename " " section, "[verify][samples][" project "]") { \ - ebpf_verifier_options_t options = ebpf_verifier_default_options; \ - options.legacy = true; \ - VERIFY_SECTION(project, filename, section, &options, true); \ - } \ - FAIL_UNMARSHAL(project, filename, section) +#define TEST_SECTION_LEGACY(dirname, filename, sectionname) \ + TEST_SECTION(dirname, filename, sectionname) \ + TEST_CASE("Try unmarshalling bad program: " dirname "/" filename " " sectionname, "[unmarshal]") { \ + ebpf_platform_t platform = g_ebpf_platform_linux; \ + platform.legacy = false; \ + auto raw_progs = read_elf("ebpf-samples/" dirname "/" filename, sectionname, nullptr, &platform); \ + REQUIRE(raw_progs.size() == 1); \ + raw_program raw_prog = raw_progs.back(); \ + std::variant prog_or_error = unmarshal(raw_prog, nullptr); \ + REQUIRE(std::holds_alternative(prog_or_error)); \ + } TEST_SECTION("bpf_cilium_test", "bpf_lxc_jit.o", "1/0xdc06") TEST_SECTION("bpf_cilium_test", "bpf_lxc_jit.o", "2/1") From 844632cc834e35d6c2caeb37a6236be54ac901bb Mon Sep 17 00:00:00 2001 From: Dave Thaler Date: Wed, 7 Feb 2024 14:41:53 -0800 Subject: [PATCH 6/7] Remove unnecessary changes Signed-off-by: Dave Thaler --- src/asm_unmarshal.cpp | 7 +++---- src/asm_unmarshal.hpp | 11 ++++------- src/ebpf_yaml.cpp | 13 +++++-------- src/main/check.cpp | 2 +- src/test/test_marshal.cpp | 27 ++++++++++----------------- src/test/test_print.cpp | 2 +- src/test/test_verify.cpp | 16 +++++++--------- 7 files changed, 31 insertions(+), 47 deletions(-) diff --git a/src/asm_unmarshal.cpp b/src/asm_unmarshal.cpp index 964f23d69..fae277be6 100644 --- a/src/asm_unmarshal.cpp +++ b/src/asm_unmarshal.cpp @@ -529,9 +529,8 @@ struct Unmarshaller { } }; -std::variant unmarshal(const raw_program& raw_prog, const ebpf_verifier_options_t* options, vector>& notes) { +std::variant unmarshal(const raw_program& raw_prog, vector>& notes) { global_program_info = raw_prog.info; - thread_local_options = (options != nullptr) ? *options : ebpf_verifier_default_options; try { return Unmarshaller{notes, raw_prog.info}.unmarshal(raw_prog.prog, raw_prog.line_info); } catch (InvalidInstruction& arg) { @@ -541,9 +540,9 @@ std::variant unmarshal(const raw_program& raw_prog, } } -std::variant unmarshal(const raw_program& raw_prog, const ebpf_verifier_options_t* options) { +std::variant unmarshal(const raw_program& raw_prog) { vector> notes; - return unmarshal(raw_prog, options, notes); + return unmarshal(raw_prog, notes); } Call make_call(int imm, const ebpf_platform_t& platform) diff --git a/src/asm_unmarshal.hpp b/src/asm_unmarshal.hpp index 60c968cdc..d29c47e08 100644 --- a/src/asm_unmarshal.hpp +++ b/src/asm_unmarshal.hpp @@ -15,13 +15,10 @@ /** Translate a sequence of eBPF instructions (elf binary format) to a sequence * of Instructions. * - * \param[in] raw_prog The input program to parse. - * \param[in] options Verifier options. - * \param[out] notes Where errors and warnings are written to. + * \param raw_prog is the input program to parse. + * \param[out] notes is errors and warnings are written to. * \return a sequence of instruction if successful, an error string otherwise. */ -std::variant unmarshal(const raw_program& raw_prog, const ebpf_verifier_options_t* options, - std::vector>& notes); -std::variant unmarshal(const raw_program& raw_prog, - const ebpf_verifier_options_t* options); +std::variant unmarshal(const raw_program& raw_prog, std::vector>& notes); +std::variant unmarshal(const raw_program& raw_prog); Call make_call(int func, const ebpf_platform_t& platform); diff --git a/src/ebpf_yaml.cpp b/src/ebpf_yaml.cpp index f9d654eca..f728d6c88 100644 --- a/src/ebpf_yaml.cpp +++ b/src/ebpf_yaml.cpp @@ -323,15 +323,8 @@ ConformanceTestResult run_conformance_test_case(const std::vector& memo raw_program raw_prog{.prog = insts}; raw_prog.info.platform = &g_ebpf_platform_linux; - ebpf_verifier_options_t options = ebpf_verifier_default_options; - if (debug) { - options.print_failures = true; - options.print_invariants = true; - options.no_simplify = true; - } - // Convert the raw program section to a set of instructions. - std::variant prog_or_error = unmarshal(raw_prog, &options); + std::variant prog_or_error = unmarshal(raw_prog); if (std::holds_alternative(prog_or_error)) { std::cerr << "unmarshaling error at " << std::get(prog_or_error) << "\n"; return {}; @@ -339,8 +332,12 @@ ConformanceTestResult run_conformance_test_case(const std::vector& memo auto& prog = std::get(prog_or_error); + ebpf_verifier_options_t options = ebpf_verifier_default_options; if (debug) { print(prog, std::cout, {}); + options.print_failures = true; + options.print_invariants = true; + options.no_simplify = true; } try { diff --git a/src/main/check.cpp b/src/main/check.cpp index 85e244080..bf832fc73 100644 --- a/src/main/check.cpp +++ b/src/main/check.cpp @@ -146,7 +146,7 @@ int main(int argc, char** argv) { raw_program raw_prog = raw_progs.back(); // Convert the raw program section to a set of instructions. - std::variant prog_or_error = unmarshal(raw_prog, &ebpf_verifier_options); + std::variant prog_or_error = unmarshal(raw_prog); if (std::holds_alternative(prog_or_error)) { std::cout << "unmarshaling error at " << std::get(prog_or_error) << "\n"; return 1; diff --git a/src/test/test_marshal.cpp b/src/test/test_marshal.cpp index c59b59ab6..9df512289 100644 --- a/src/test/test_marshal.cpp +++ b/src/test/test_marshal.cpp @@ -8,13 +8,11 @@ // Verify that if we unmarshal an instruction and then re-marshal it, // we get what we expect. -static void compare_unmarshal_marshal(const ebpf_inst& ins, const ebpf_inst& expected_result, - const ebpf_verifier_options_t* options = nullptr, - const ebpf_platform_t* platform = &g_ebpf_platform_linux) { +static void compare_unmarshal_marshal(const ebpf_inst& ins, const ebpf_inst& expected_result, const ebpf_platform_t* platform = &g_ebpf_platform_linux) { program_info info{.platform = platform, .type = platform->get_program_type("unspec", "unspec")}; const ebpf_inst exit{.opcode = INST_OP_EXIT}; - InstructionSeq parsed = std::get(unmarshal(raw_program{"", "", {ins, exit, exit}, info}, options)); + InstructionSeq parsed = std::get(unmarshal(raw_program{"", "", {ins, exit, exit}, info})); REQUIRE(parsed.size() == 3); auto [_, single, _2] = parsed.front(); (void)_; // unused @@ -47,12 +45,10 @@ static void compare_unmarshal_marshal(const ebpf_inst& ins1, const ebpf_inst& in // Verify that if we marshal an instruction and then unmarshal it, // we get the original. -static void compare_marshal_unmarshal(const Instruction& ins, bool double_cmd = false, - const ebpf_verifier_options_t* options = nullptr, - const ebpf_platform_t* platform = &g_ebpf_platform_linux) { +static void compare_marshal_unmarshal(const Instruction& ins, bool double_cmd = false, const ebpf_platform_t* platform = &g_ebpf_platform_linux) { program_info info{.platform = platform, .type = platform->get_program_type("unspec", "unspec")}; - InstructionSeq parsed = std::get(unmarshal(raw_program{"", "", marshal(ins, 0), info}, options)); + InstructionSeq parsed = std::get(unmarshal(raw_program{"", "", marshal(ins, 0), info})); REQUIRE(parsed.size() == 1); auto [_, single, _2] = parsed.back(); (void)_; // unused @@ -60,21 +56,18 @@ static void compare_marshal_unmarshal(const Instruction& ins, bool double_cmd = REQUIRE(single == ins); } -static void check_marshal_unmarshal_fail(const Instruction& ins, std::string expected_error_message, - const ebpf_platform_t* platform = &g_ebpf_platform_linux) { +static void check_marshal_unmarshal_fail(const Instruction& ins, std::string expected_error_message, const ebpf_platform_t* platform = &g_ebpf_platform_linux) { program_info info{.platform = platform, .type = platform->get_program_type("unspec", "unspec")}; - std::string error_message = std::get(unmarshal(raw_program{"", "", marshal(ins, 0), info}, nullptr)); + std::string error_message = std::get(unmarshal(raw_program{"", "", marshal(ins, 0), info})); REQUIRE(error_message == expected_error_message); } -static void check_unmarshal_fail(ebpf_inst inst, std::string expected_error_message, - const ebpf_verifier_options_t* options = nullptr, - const ebpf_platform_t* platform = &g_ebpf_platform_linux) { +static void check_unmarshal_fail(ebpf_inst inst, std::string expected_error_message, const ebpf_platform_t* platform = &g_ebpf_platform_linux) { program_info info{.platform = platform, .type = platform->get_program_type("unspec", "unspec")}; std::vector insns = {inst}; - auto result = unmarshal(raw_program{"", "", insns, info}, options); + auto result = unmarshal(raw_program{"", "", insns, info}); REQUIRE(std::holds_alternative(result)); std::string error_message = std::get(result); REQUIRE(error_message == expected_error_message); @@ -369,12 +362,12 @@ TEST_CASE("check unmarshal legacy opcodes", "[disasm][marshal]") { for (uint8_t opcode : unsupported_legacy_opcodes) { std::ostringstream oss; oss << "0: Bad instruction op 0x" << std::hex << (int)opcode << std::endl; - check_unmarshal_fail(ebpf_inst{.opcode = opcode}, oss.str().c_str(), nullptr, &platform); + check_unmarshal_fail(ebpf_inst{.opcode = opcode}, oss.str().c_str(), &platform); } for (uint8_t opcode : supported_legacy_opcodes) { std::ostringstream oss; oss << "0: Bad instruction op 0x" << std::hex << (int)opcode << std::endl; - check_unmarshal_fail(ebpf_inst{.opcode = opcode}, oss.str().c_str(), nullptr, &platform); + check_unmarshal_fail(ebpf_inst{.opcode = opcode}, oss.str().c_str(), &platform); } } diff --git a/src/test/test_print.cpp b/src/test/test_print.cpp index 60966d434..5980c7852 100644 --- a/src/test/test_print.cpp +++ b/src/test/test_print.cpp @@ -27,7 +27,7 @@ void verify_printed_string(const std::string& file) std::stringstream generated_output; auto raw_progs = read_elf(std::string(TEST_OBJECT_FILE_DIRECTORY) + file + ".o", "", nullptr, &g_ebpf_platform_linux); raw_program raw_prog = raw_progs.back(); - std::variant prog_or_error = unmarshal(raw_prog, nullptr); + std::variant prog_or_error = unmarshal(raw_prog); REQUIRE(std::holds_alternative(prog_or_error)); auto& program = std::get(prog_or_error); print(program, generated_output, {}); diff --git a/src/test/test_verify.cpp b/src/test/test_verify.cpp index 59a6963ee..bca93d05b 100644 --- a/src/test/test_verify.cpp +++ b/src/test/test_verify.cpp @@ -24,7 +24,7 @@ FAIL_LOAD_ELF("invalid", "badsymsize.o", "xdp_redirect_map") auto raw_progs = read_elf("ebpf-samples/" dirname "/" filename, sectionname, nullptr, &g_ebpf_platform_linux); \ REQUIRE(raw_progs.size() == 1); \ raw_program raw_prog = raw_progs.back(); \ - std::variant prog_or_error = unmarshal(raw_prog, nullptr); \ + std::variant prog_or_error = unmarshal(raw_prog); \ REQUIRE(std::holds_alternative(prog_or_error)); \ } @@ -37,7 +37,7 @@ FAIL_UNMARSHAL("invalid", "invalid-lddw.o", ".text") auto raw_progs = read_elf("ebpf-samples/" dirname "/" filename, sectionname, nullptr, platform); \ REQUIRE(raw_progs.size() == 1); \ raw_program raw_prog = raw_progs.back(); \ - std::variant prog_or_error = unmarshal(raw_prog, options); \ + std::variant prog_or_error = unmarshal(raw_prog); \ REQUIRE(std::holds_alternative(prog_or_error)); \ auto& prog = std::get(prog_or_error); \ bool res = ebpf_verify_program(std::cout, prog, raw_prog.info, options, nullptr); \ @@ -83,7 +83,7 @@ FAIL_UNMARSHAL("invalid", "invalid-lddw.o", ".text") auto raw_progs = read_elf("ebpf-samples/" dirname "/" filename, sectionname, nullptr, &platform); \ REQUIRE(raw_progs.size() == 1); \ raw_program raw_prog = raw_progs.back(); \ - std::variant prog_or_error = unmarshal(raw_prog, nullptr); \ + std::variant prog_or_error = unmarshal(raw_prog); \ REQUIRE(std::holds_alternative(prog_or_error)); \ } @@ -556,20 +556,18 @@ void test_analyze_thread(cfg_t* cfg, program_info* info, bool* res) { // Test multithreading TEST_CASE("multithreading", "[verify][multithreading]") { - ebpf_verifier_options_t* options = nullptr; - - auto raw_progs1 = read_elf("ebpf-samples/bpf_cilium_test/bpf_netdev.o", "2/1", options, &g_ebpf_platform_linux); + auto raw_progs1 = read_elf("ebpf-samples/bpf_cilium_test/bpf_netdev.o", "2/1", nullptr, &g_ebpf_platform_linux); REQUIRE(raw_progs1.size() == 1); raw_program raw_prog1 = raw_progs1.back(); - std::variant prog_or_error1 = unmarshal(raw_prog1, options); + std::variant prog_or_error1 = unmarshal(raw_prog1); REQUIRE(std::holds_alternative(prog_or_error1)); auto& prog1 = std::get(prog_or_error1); cfg_t cfg1 = prepare_cfg(prog1, raw_prog1.info, true); - auto raw_progs2 = read_elf("ebpf-samples/bpf_cilium_test/bpf_netdev.o", "2/2", options, &g_ebpf_platform_linux); + auto raw_progs2 = read_elf("ebpf-samples/bpf_cilium_test/bpf_netdev.o", "2/2", nullptr, &g_ebpf_platform_linux); REQUIRE(raw_progs2.size() == 1); raw_program raw_prog2 = raw_progs2.back(); - std::variant prog_or_error2 = unmarshal(raw_prog2, options); + std::variant prog_or_error2 = unmarshal(raw_prog2); REQUIRE(std::holds_alternative(prog_or_error2)); auto& prog2 = std::get(prog_or_error2); cfg_t cfg2 = prepare_cfg(prog2, raw_prog2.info, true); From 97e076a3f9e7f08d637324d7a526ed2eaf15ec64 Mon Sep 17 00:00:00 2001 From: Dave Thaler Date: Thu, 15 Feb 2024 23:16:23 -1000 Subject: [PATCH 7/7] Accepted grammar suggestions from Elazar Signed-off-by: Dave Thaler --- src/asm_unmarshal.hpp | 2 +- src/platform.hpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/asm_unmarshal.hpp b/src/asm_unmarshal.hpp index d29c47e08..f5d2ce85f 100644 --- a/src/asm_unmarshal.hpp +++ b/src/asm_unmarshal.hpp @@ -16,7 +16,7 @@ * of Instructions. * * \param raw_prog is the input program to parse. - * \param[out] notes is errors and warnings are written to. + * \param[out] notes is a vector for storing errors and warnings. * \return a sequence of instruction if successful, an error string otherwise. */ std::variant unmarshal(const raw_program& raw_prog, std::vector>& notes); diff --git a/src/platform.hpp b/src/platform.hpp index 38ca90be5..bc50b9601 100644 --- a/src/platform.hpp +++ b/src/platform.hpp @@ -43,7 +43,7 @@ struct ebpf_platform_t { ebpf_get_map_type_fn get_map_type; ebpf_resolve_inner_map_references_fn resolve_inner_map_references; - // Options indicating support for various instructions. + // Option indicating support for various deprecated instructions. bool legacy; };