Skip to content

Commit

Permalink
Restrict comparisons with pointers to legal operations (#758)
Browse files Browse the repository at this point in the history
Signed-off-by: Alan Jowett <alan.jowett@microsoft.com>
Alan-Jowett authored Oct 24, 2024
1 parent 6f944ff commit 3308618
Showing 3 changed files with 566 additions and 1 deletion.
28 changes: 27 additions & 1 deletion src/assertions.cpp
Original file line number Diff line number Diff line change
@@ -137,6 +137,31 @@ class AssertExtractor {
// no need to check for valid access, it must be a number
res.emplace_back(TypeConstraint{cond.left, TypeGroup::number});
} else {
bool allow_pointers = false;

switch (cond.op) {
case Condition::Op::EQ: allow_pointers = true; break;
case Condition::Op::NE: allow_pointers = true; break;
case Condition::Op::SET: allow_pointers = true; break;
case Condition::Op::NSET: allow_pointers = true; break;
case Condition::Op::LT: allow_pointers = true; break;
case Condition::Op::LE: allow_pointers = true; break;
case Condition::Op::GT: allow_pointers = true; break;
case Condition::Op::GE: allow_pointers = true; break;
case Condition::Op::SLT: allow_pointers = false; break;
case Condition::Op::SLE: allow_pointers = false; break;
case Condition::Op::SGT: allow_pointers = false; break;
case Condition::Op::SGE: allow_pointers = false; break;
default: assert(!"unexpected condition");
}

// Only permit pointer comparisons if the operation is 64-bit.
allow_pointers &= cond.is64;

if (!allow_pointers) {
res.emplace_back(TypeConstraint{cond.left, TypeGroup::number});
}

res.emplace_back(ValidAccess{cond.left});
// OK - map_fd is just another pointer
// Anything can be compared to 0
@@ -241,7 +266,8 @@ class AssertExtractor {
}
return {Assert{TypeConstraint{ins.dst, TypeGroup::number}}};
}
// For all other binary operations, the destination register must be a number and the source must either be an immediate or a number.
// For all other binary operations, the destination register must be a number and the source must either be an
// immediate or a number.
default:
if (const auto src = std::get_if<Reg>(&ins.v)) {
return {Assert{TypeConstraint{ins.dst, TypeGroup::number}},
5 changes: 5 additions & 0 deletions src/test/ebpf_yaml.cpp
Original file line number Diff line number Diff line change
@@ -179,6 +179,9 @@ static ebpf_verifier_options_t raw_options_to_options(const std::set<string>& ra
// Default to the machine's native endianness.
options.big_endian = (std::endian::native == std::endian::big);

// Default to not assuming assertions.
options.assume_assertions = false;

for (const string& name : raw_options) {
if (name == "!allow_division_by_zero") {
options.allow_division_by_zero = false;
@@ -192,6 +195,8 @@ static ebpf_verifier_options_t raw_options_to_options(const std::set<string>& ra
options.big_endian = true;
} else if (name == "!big_endian") {
options.big_endian = false;
} else if (name == "assume_assertions") {
options.assume_assertions = true;
} else {
throw std::runtime_error("Unknown option: " + name);
}
Loading

0 comments on commit 3308618

Please sign in to comment.