diff --git a/src/asm_unmarshal.cpp b/src/asm_unmarshal.cpp index 4a6af8308..c5041b434 100644 --- a/src/asm_unmarshal.cpp +++ b/src/asm_unmarshal.cpp @@ -310,19 +310,21 @@ struct Unmarshaller { auto makeLddw(ebpf_inst inst, int32_t next_imm, const vector& insts, pc_t pc) -> Instruction { if (pc >= insts.size() - 1) throw InvalidInstruction(pc, "incomplete LDDW"); + ebpf_inst next = insts[pc + 1]; + if (next.opcode != 0 || next.dst != 0 || next.src != 0 || next.offset != 0) + throw InvalidInstruction(pc, "invalid LDDW"); if (inst.src > 1 || inst.dst > R10_STACK_POINTER || inst.offset != 0) throw InvalidInstruction(pc, "LDDW uses reserved fields"); if (inst.src == 1) { // magic number, meaning we're a per-process file descriptor defining the map. // (for details, look for BPF_PSEUDO_MAP_FD in the kernel) - + if (next.imm != 0) { + throw InvalidInstruction(pc, "LDDW uses reserved fields"); + } return LoadMapFd{.dst = Reg{inst.dst}, .mapfd = inst.imm}; } - ebpf_inst next = insts[pc + 1]; - if (next.opcode != 0 || next.dst != 0 || next.src != 0 || next.offset != 0) - throw InvalidInstruction(pc, "invalid LDDW"); return Bin{ .op = Bin::Op::MOV, .dst = Reg{inst.dst}, diff --git a/src/test/test_marshal.cpp b/src/test/test_marshal.cpp index ffe0c8d02..45291304c 100644 --- a/src/test/test_marshal.cpp +++ b/src/test/test_marshal.cpp @@ -23,6 +23,26 @@ static void compare_unmarshal_marshal(const ebpf_inst& ins, const ebpf_inst& exp REQUIRE(memcmp(&expected_result, &result, sizeof(result)) == 0); } +// Verify that if we unmarshal a 64-bit immediate instruction and then re-marshal it, +// we get what we expect. +static void compare_unmarshal_marshal(const ebpf_inst& ins1, const ebpf_inst& ins2, const ebpf_inst& expected_result1, + const ebpf_inst& expected_result2) { + 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{"", "", {ins1, ins2, exit, exit}, info})); + REQUIRE(parsed.size() == 3); + auto [_, single, _2] = parsed.front(); + (void)_; // unused + (void)_2; // unused + std::vector marshaled = marshal(single, 0); + REQUIRE(marshaled.size() == 2); + ebpf_inst result1 = marshaled.front(); + REQUIRE(memcmp(&expected_result1, &result1, sizeof(result1)) == 0); + ebpf_inst result2 = marshaled.back(); + REQUIRE(memcmp(&expected_result2, &result2, sizeof(result2)) == 0); +} + // 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) { @@ -53,6 +73,17 @@ static void check_unmarshal_fail(ebpf_inst inst, std::string expected_error_mess REQUIRE(error_message == expected_error_message); } +// Check that unmarshaling a 64-bit immediate instruction fails. +static void check_unmarshal_fail(ebpf_inst inst1, ebpf_inst inst2, std::string expected_error_message) { + program_info info{.platform = &g_ebpf_platform_linux, + .type = g_ebpf_platform_linux.get_program_type("unspec", "unspec")}; + std::vector insns = {inst1, inst2}; + 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); +} + static const auto ws = {1, 2, 4, 8}; TEST_CASE("disasm_marshal", "[disasm][marshal]") { @@ -301,10 +332,41 @@ TEST_CASE("fail unmarshal offset opcodes", "[disasm][marshal]") { } } +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}); + compare_unmarshal_marshal(ebpf_inst{.opcode = /* 0x18 */ INST_OP_LDDW_IMM, .src = 0, .imm = 1}, ebpf_inst{}, + ebpf_inst{.opcode = /* 0x18 */ INST_OP_LDDW_IMM, .src = 0, .imm = 1}, ebpf_inst{}); + + for (uint8_t src = 0; src <= 7; src++) { + check_unmarshal_fail(ebpf_inst{.opcode = /* 0x18 */ INST_OP_LDDW_IMM, .src = src}, "0: incomplete LDDW\n"); + check_unmarshal_fail(ebpf_inst{.opcode = /* 0x18 */ INST_OP_LDDW_IMM, .src = src}, + ebpf_inst{.opcode = /* 0x18 */ INST_OP_LDDW_IMM}, "0: invalid LDDW\n"); + } + + // No supported src values use the offset field. + for (uint8_t src = 0; src <= 1; src++) { + check_unmarshal_fail(ebpf_inst{.opcode = /* 0x18 */ INST_OP_LDDW_IMM, .src = src, .offset = 1}, ebpf_inst{}, + "0: LDDW uses reserved fields\n"); + } + + // Verify that unsupported src values fail. + // TODO: support src = 2 through 6. + for (uint8_t src = 2; src <= 7; src++) { + check_unmarshal_fail(ebpf_inst{.opcode = /* 0x18 */ INST_OP_LDDW_IMM, .src = src}, ebpf_inst{}, + "0: LDDW uses reserved fields\n"); + } + + // When src = {1, 3, 4, 5}, next_imm must be 0. + for (uint8_t src : {1, 3, 4, 5}) { + check_unmarshal_fail(ebpf_inst{.opcode = /* 0x18 */ INST_OP_LDDW_IMM, .src = src}, ebpf_inst{.imm = 1}, + "0: LDDW uses reserved fields\n"); + } +} + 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},