Deprecate legacy packet access instructions (#577)

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. * Deprecate legacy packet access instructions * Add YAML tests for packet access instructions Signed-off-by: Dave Thaler <[email protected]>
vbpf · Feb 18, 2024 · 1056586 · 1056586
1 parent 2567b30
commit 1056586
Show file tree

Hide file tree

Showing 11 changed files with 213 additions and 63 deletions.
diff --git 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
@@ -226,17 +226,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 (!info.platform->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 (!info.platform->legacy || !isLD || (width == 8))
+                throw InvalidInstruction(pc, inst.opcode);
             return Packet{.width = width, .offset = inst.imm, .regoffset = Reg{inst.src}};
 
         case INST_MEM: {

diff --git 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 notes is where 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<InstructionSeq, std::string> unmarshal(const raw_program& raw_prog, std::vector<std::vector<std::string>>& notes);

diff --git 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
@@ -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) {

diff --git 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
@@ -66,6 +66,8 @@ 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("--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");
@@ -111,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_program> 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;
@@ -130,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
@@ -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;
+
+    // Option indicating support for various deprecated 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
@@ -8,9 +8,9 @@
 
 // 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) {
-    program_info info{.platform = &g_ebpf_platform_linux,
-                      .type = g_ebpf_platform_linux.get_program_type("unspec", "unspec")};
+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<InstructionSeq>(unmarshal(raw_program{"", "", {ins, exit, exit}, info}));
     REQUIRE(parsed.size() == 3);
@@ -62,9 +62,9 @@ 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) {
-    program_info info{.platform = &g_ebpf_platform_linux,
-                      .type = g_ebpf_platform_linux.get_program_type("unspec", "unspec")};
+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<InstructionSeq>(unmarshal(raw_program{"", "", marshal(ins, 0), info}));
     REQUIRE(parsed.size() == 1);
     auto [_, single, _2] = parsed.back();
@@ -73,16 +73,16 @@ 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<std::string>(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) {
-    program_info info{.platform = &g_ebpf_platform_linux,
-                      .type = g_ebpf_platform_linux.get_program_type("unspec", "unspec")};
+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<ebpf_inst> insns = {inst};
     auto result = unmarshal(raw_program{"", "", insns, info});
     REQUIRE(std::holds_alternative<std::string>(result));
@@ -194,8 +194,10 @@ TEST_CASE("disasm_marshal", "[disasm][marshal]") {
 
     SECTION("Packet") {
         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 = {}});
+                compare_marshal_unmarshal(Packet{.width = w, .offset = 7, .regoffset = Reg{2}});
+            }
         }
     }
 
@@ -367,6 +369,39 @@ 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};
+
+    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());
+    }
+
+    for (uint8_t opcode : supported_legacy_opcodes) {
+        compare_unmarshal_marshal(ebpf_inst{.opcode = opcode}, ebpf_inst{.opcode = opcode});
+    }
+
+    // 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(), &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(), &platform);
+    }
+}
+
 TEST_CASE("unmarshal 64bit immediate", "[disasm][marshal]") {
     compare_unmarshal_marshal(ebpf_inst{.opcode = /* 0x18 */ INST_OP_LDDW_IMM, .src = 0, .imm = 1}, ebpf_inst{.imm = 2},
                               ebpf_inst{.opcode = /* 0x18 */ INST_OP_LDDW_IMM, .src = 0, .imm = 1}, ebpf_inst{.imm = 2});
@@ -399,13 +434,10 @@ TEST_CASE("unmarshal 64bit immediate", "[disasm][marshal]") {
     }
 }
 
+
 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 = /* 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},