Skip to content

Commit

Permalink
Add more negative unmarshaling tests (#591)
Browse files Browse the repository at this point in the history 
* Add more negative unmarshaling tests
* table based tests
* Lower case unmarshaling errors for consistency
* Simplify c_str uses
---------

Signed-off-by: Dave Thaler <[email protected]>
  • Loading branch information
@dthaler
dthaler authored Feb 28, 2024
1 parent ec074df commit e0c25fc
Show file tree
Hide file tree
Showing 4 changed files with 385 additions and 137 deletions.
66 changes: 48 additions & 18 deletions src/asm_unmarshal.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ struct InvalidInstruction : std::invalid_argument {
size_t pc;
explicit InvalidInstruction(size_t pc, const char* what) : std::invalid_argument{what}, pc{pc} {}
InvalidInstruction(size_t pc, std::string what) : std::invalid_argument{what}, pc{pc} {}
InvalidInstruction(size_t pc, uint8_t opcode) : std::invalid_argument{make_opcode_message("Bad instruction", opcode)}, pc{pc} {}
InvalidInstruction(size_t pc, uint8_t opcode) : std::invalid_argument{make_opcode_message("bad instruction", opcode)}, pc{pc} {}
};

struct UnsupportedMemoryMode : std::invalid_argument {
Expand Down Expand Up @@ -115,7 +115,7 @@ struct Unmarshaller {
}
case INST_ALU_OP_MOV:
if (inst.offset > 0 && !(inst.opcode & INST_SRC_REG))
throw InvalidInstruction(pc, make_opcode_message("invalid offset for", inst.opcode));
throw InvalidInstruction(pc, make_opcode_message("nonzero offset for", inst.opcode));
switch (inst.offset) {
case 0: return Bin::Op::MOV;
case 8: return Bin::Op::MOVSX8;
Expand All @@ -142,7 +142,7 @@ struct Unmarshaller {
if (inst.opcode & INST_SRC_REG)
throw InvalidInstruction{pc, inst.opcode};
if (inst.src != 0)
throw InvalidInstruction{pc, make_opcode_message("nonzero src for register", inst.opcode)};
throw InvalidInstruction{pc, inst.opcode};
if (inst.imm != 0)
throw InvalidInstruction(pc, make_opcode_message("nonzero imm for", inst.opcode));
return Un::Op::NEG;
Expand All @@ -153,23 +153,23 @@ struct Unmarshaller {
return Bin::Op::ARSH;
case INST_ALU_OP_END:
if (inst.src != 0)
throw InvalidInstruction{pc, make_opcode_message("nonzero src for register", inst.opcode)};
throw InvalidInstruction{pc, inst.opcode};
if ((inst.opcode & INST_CLS_MASK) == INST_CLS_ALU64) {
if (inst.opcode & INST_END_BE)
throw InvalidInstruction(pc, inst.opcode);
switch (inst.imm) {
case 16: return Un::Op::SWAP16;
case 32: return Un::Op::SWAP32;
case 64: return Un::Op::SWAP64;
default: throw InvalidInstruction(pc, "invalid endian immediate");
default: throw InvalidInstruction(pc, "unsupported immediate");
}
}
switch (inst.imm) {
case 16: return (inst.opcode & INST_END_BE) ? Un::Op::BE16 : Un::Op::LE16;
case 32: return (inst.opcode & INST_END_BE) ? Un::Op::BE32 : Un::Op::LE32;
case 64: return (inst.opcode & INST_END_BE) ? Un::Op::BE64 : Un::Op::LE64;
default:
throw InvalidInstruction(pc, "invalid endian immediate");
throw InvalidInstruction(pc, "unsupported immediate");
}
case 0xe0: throw InvalidInstruction{pc, inst.opcode};
case 0xf0: throw InvalidInstruction{pc, inst.opcode};
Expand All @@ -188,7 +188,7 @@ struct Unmarshaller {
return Reg{inst.src};
} else {
if (inst.src != 0)
throw InvalidInstruction{pc, make_opcode_message("nonzero src for register", inst.opcode)};
throw InvalidInstruction{pc, inst.opcode};
// Imm is a signed 32-bit number. Sign extend it to 64-bits for storage.
return Imm{sign_extend(inst.imm)};
}
Expand Down Expand Up @@ -219,7 +219,7 @@ struct Unmarshaller {

auto makeMemOp(pc_t pc, ebpf_inst inst) -> Instruction {
if (inst.dst > R10_STACK_POINTER || inst.src > R10_STACK_POINTER)
throw InvalidInstruction(pc, "Bad register");
throw InvalidInstruction(pc, "bad register");

int width = getMemWidth(inst.opcode);
bool isLD = (inst.opcode & INST_CLS_MASK) == INST_CLS_LD;
Expand All @@ -228,11 +228,23 @@ struct Unmarshaller {
case INST_ABS:
if (!info.platform->legacy || !isLD || (width == 8))
throw InvalidInstruction(pc, inst.opcode);
if (inst.dst != 0)
throw InvalidInstruction(pc, make_opcode_message("nonzero dst for register", inst.opcode));
if (inst.src > 0)
throw InvalidInstruction(pc, make_opcode_message("bad instruction", inst.opcode));
if (inst.offset != 0)
throw InvalidInstruction(pc, make_opcode_message("nonzero offset for", inst.opcode));
return Packet{.width = width, .offset = inst.imm, .regoffset = {}};

case INST_IND:
if (!info.platform->legacy || !isLD || (width == 8))
throw InvalidInstruction(pc, inst.opcode);
if (inst.dst != 0)
throw InvalidInstruction(pc, make_opcode_message("nonzero dst for register", inst.opcode));
if (inst.src > R10_STACK_POINTER)
throw InvalidInstruction(pc, "bad register");
if (inst.offset != 0)
throw InvalidInstruction(pc, make_opcode_message("nonzero offset for", inst.opcode));
return Packet{.width = width, .offset = inst.imm, .regoffset = Reg{inst.src}};

case INST_MEM: {
Expand All @@ -243,7 +255,7 @@ struct Unmarshaller {
throw InvalidInstruction(pc, "Cannot modify r10");
bool isImm = !(inst.opcode & 1);
if (isImm && inst.src != 0)
throw InvalidInstruction(pc, make_opcode_message("nonzero src for register", inst.opcode));
throw InvalidInstruction(pc, inst.opcode);
if (!isImm && inst.imm != 0)
throw InvalidInstruction(pc, make_opcode_message("nonzero imm for", inst.opcode));

Expand Down Expand Up @@ -273,7 +285,7 @@ struct Unmarshaller {
(inst.opcode & INST_SIZE_MASK) != INST_SIZE_DW))
throw InvalidInstruction(pc, inst.opcode);
if (inst.imm != 0)
throw InvalidInstruction(pc, "Unsupported atomic instruction");
throw InvalidInstruction(pc, "unsupported immediate");
return LockAdd{
.access =
Deref{
Expand All @@ -292,7 +304,7 @@ struct Unmarshaller {
if (inst.dst == R10_STACK_POINTER)
throw InvalidInstruction(pc, "Invalid target r10");
if (inst.dst > R10_STACK_POINTER || inst.src > R10_STACK_POINTER)
throw InvalidInstruction(pc, "Bad register");
throw InvalidInstruction(pc, "bad register");
bool is64 = (inst.opcode & INST_CLS_MASK) == INST_CLS_ALU64;
return std::visit(overloaded{[&](Un::Op op) -> Instruction { return Un{.op = op, .dst = Reg{inst.dst}, .is64 = is64}; },
[&](Bin::Op op) -> Instruction {
Expand All @@ -312,18 +324,22 @@ struct Unmarshaller {

auto makeLddw(ebpf_inst inst, int32_t next_imm, const vector<ebpf_inst>& insts, pc_t pc) -> Instruction {
if (pc >= insts.size() - 1)
throw InvalidInstruction(pc, "incomplete LDDW");
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");
throw InvalidInstruction(pc, "invalid lddw");
if (inst.src > 1)
throw InvalidInstruction(pc, make_opcode_message("bad instruction", inst.opcode));
if (inst.offset != 0)
throw InvalidInstruction(pc, make_opcode_message("nonzero offset for", inst.opcode));
if (inst.dst > R10_STACK_POINTER)
throw InvalidInstruction(pc, "bad register");

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");
throw InvalidInstruction(pc, "lddw uses reserved fields");
}
return LoadMapFd{.dst = Reg{inst.dst}, .mapfd = inst.imm};
}
Expand Down Expand Up @@ -413,16 +429,22 @@ struct Unmarshaller {
throw InvalidInstruction(pc, inst.opcode);
if (inst.opcode & INST_SRC_REG)
throw InvalidInstruction(pc, inst.opcode);
if (inst.src > 0)
throw InvalidInstruction(pc, inst.opcode);
if (inst.offset != 0)
throw InvalidInstruction(pc, make_opcode_message("nonzero offset for", inst.opcode));
if (inst.dst != 0)
throw InvalidInstruction(pc, make_opcode_message("nonzero dst for register", inst.opcode));
if (!info.platform->is_helper_usable(inst.imm))
throw InvalidInstruction(pc, "invalid helper function id");
return makeCall(inst.imm);
case INST_EXIT:
if ((inst.opcode & INST_CLS_MASK) != INST_CLS_JMP || (inst.opcode & INST_SRC_REG))
throw InvalidInstruction(pc, inst.opcode);
if (inst.src != 0)
throw InvalidInstruction(pc, make_opcode_message("nonzero src for register", inst.opcode));
throw InvalidInstruction(pc, inst.opcode);
if (inst.dst != 0)
throw InvalidInstruction(pc, make_opcode_message("nonzero dst for register", inst.opcode));
if (inst.imm != 0)
throw InvalidInstruction(pc, make_opcode_message("nonzero imm for", inst.opcode));
if (inst.offset != 0)
Expand All @@ -438,11 +460,13 @@ struct Unmarshaller {
throw InvalidInstruction(pc, make_opcode_message("nonzero imm for", inst.opcode));
if ((inst.opcode & INST_CLS_MASK) == INST_CLS_JMP32 && (inst.offset != 0))
throw InvalidInstruction(pc, make_opcode_message("nonzero offset for", inst.opcode));
if (inst.dst != 0)
throw InvalidInstruction(pc, make_opcode_message("nonzero dst for register", inst.opcode));
default: {
// First validate the opcode, src, and imm.
auto op = getJmpOp(pc, inst.opcode);
if (!(inst.opcode & INST_SRC_REG) && (inst.src != 0))
throw InvalidInstruction(pc, make_opcode_message("nonzero src for register", inst.opcode));
throw InvalidInstruction(pc, inst.opcode);
if ((inst.opcode & INST_SRC_REG) && (inst.imm != 0))
throw InvalidInstruction(pc, make_opcode_message("nonzero imm for", inst.opcode));

Expand All @@ -452,6 +476,12 @@ struct Unmarshaller {
throw InvalidInstruction(pc, "jump out of bounds");
else if (insts[new_pc].opcode == 0)
throw InvalidInstruction(pc, "jump to middle of lddw");
if (inst.opcode != INST_OP_JA16 && inst.opcode != INST_OP_JA32) {
if (inst.dst > R10_STACK_POINTER)
throw InvalidInstruction(pc, "bad register");
if ((inst.opcode & INST_SRC_REG) && (inst.src > R10_STACK_POINTER))
throw InvalidInstruction(pc, "bad register");
}

auto cond = (inst.opcode == INST_OP_JA16 || inst.opcode == INST_OP_JA32)
? std::optional<Condition>{}
Expand Down
1 change: 1 addition & 0 deletions src/ebpf_vm_isa.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ struct ebpf_inst {
std::uint8_t src : 4; //< Source register
std::int16_t offset;
std::int32_t imm; //< Immediate constant
constexpr bool operator==(const ebpf_inst&) const = default;
};

enum {
Expand Down
2 changes: 1 addition & 1 deletion src/test/test_conformance.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ void test_conformance(std::string filename, bpf_conformance_test_result_t expect
test_path.remove_filename().append("conformance_check" + extension.string()).string();
std::map<std::filesystem::path, std::tuple<bpf_conformance_test_result_t, std::string>> result = bpf_conformance(
test_files, plugin_path, {}, {}, {}, bpf_conformance_test_CPU_version_t::v4,
_bpf_conformance_groups::default_groups, bpf_conformance_list_instructions_t::LIST_INSTRUCTIONS_NONE, true);
bpf_conformance_groups_t::default_groups, bpf_conformance_list_instructions_t::LIST_INSTRUCTIONS_NONE, true);
for (auto file : test_files) {
auto& [file_result, reason] = result[file];
REQUIRE(file_result == expected_result);
Expand Down
Loading

0 comments on commit e0c25fc

Please sign in to comment.