opentmk: add invariant runner - #4332
Conversation
Adds copyright headers and fix versioning
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon |
There was a problem hiding this comment.
Pull request overview
This PR adds an “invariant” execution target on top of OpenTMK, introducing a new opentmk_invariant runtime that receives serialized commands over serial I/O, decodes them (initially via a syzkaller-oriented decoder), and dispatches mapped fuzz functions (I/O port operations + Hyper-V hypercalls).
Changes:
- Introduces new crates
inv_packet(wire protocol) andinv_decoder(syz program decode/exec) plus a newopentmk_invariantUEFI/no_std entrypoint with serial comms, executor loop, and deserializer plumbing. - Extends workspace dependencies/members to include the new crates and adjusts
anyhowworkspace defaults (disabling default features; enablingstdwhere required). - Adds unit tests for executor/comms and the syzlang deserializer mapping/dispatch behavior.
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| xtask/Cargo.toml | Enables anyhow std feature explicitly after workspace default-features change. |
| flowey/flowey_hvlite/Cargo.toml | Enables anyhow std feature explicitly after workspace default-features change. |
| Cargo.toml | Adds new workspace members/deps; makes anyhow default-features=false; adds postcard/num-traits workspace deps. |
| Cargo.lock | Locks new crates and dependency changes. |
| opentmk/opentmk_invariant/Cargo.toml | New invariant runtime crate manifest and deps. |
| opentmk/opentmk_invariant/src/main.rs | New invariant entrypoint + UEFI entry + executor loop startup. |
| opentmk/opentmk_invariant/src/rt.rs | UEFI-only panic handler module. |
| opentmk/opentmk_invariant/src/prelude.rs | Prelude to bridge alloc/std for UEFI vs host builds. |
| opentmk/opentmk_invariant/src/comms/mod.rs | Serial handshake + packet framing + blocking read/write implementation. |
| opentmk/opentmk_invariant/src/comms/test.rs | Unit tests for handshake and packet framing/parse. |
| opentmk/opentmk_invariant/src/executor/mod.rs | Core executor, configuration handling, and dispatch loop. |
| opentmk/opentmk_invariant/src/executor/test.rs | Unit tests for executor protocol behavior and response packets. |
| opentmk/opentmk_invariant/src/deserializer/mod.rs | Deserializer trait + module wiring. |
| opentmk/opentmk_invariant/src/deserializer/syzlang/mod.rs | Syzlang deserializer integration with inv_decoder + mapping + dispatch. |
| opentmk/opentmk_invariant/src/deserializer/syzlang/test.rs | Unit tests for call mapping and pointer/copyin behavior. |
| opentmk/opentmk_invariant/src/functions/mod.rs | Function module registry exports. |
| opentmk/opentmk_invariant/src/functions/registry.rs | Name-to-function registry and dispatch helper. |
| opentmk/opentmk_invariant/src/functions/variable.rs | Fuzz function variable model + arg validation helper. |
| opentmk/opentmk_invariant/src/functions/io_port/mod.rs | I/O port read/write fuzz functions with port allowlist. |
| opentmk/opentmk_invariant/src/functions/hyperv.rs | Hyper-V hypercall fuzz function wrapper. |
| opentmk/opentmk_invariant/src/functions/hvcall_meta.rs | Hvcall metadata wire decoding helper. |
| opentmk/inv_packet/Cargo.toml | New wire-protocol crate manifest. |
| opentmk/inv_packet/src/lib.rs | Packet types + magic constants for serial framing/handshake. |
| opentmk/inv_decoder/Cargo.toml | New syzkaller decode/exec crate manifest. |
| opentmk/inv_decoder/src/wire.rs | On-the-wire structs and constants for syz format. |
| opentmk/inv_decoder/src/safememory.rs | Safe memory map abstraction for copyin/copyout. |
| opentmk/inv_decoder/src/atomicrefqueue.rs | Concurrent pop queue used by decoder execution. |
| opentmk/inv_decoder/src/lib.rs | Main decode/execute engine + copyin/copyout helpers + tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (6)
Previously missed (5) — in code that hasn't changed since the last review.
opentmk/inv_decoder/src/atomicrefqueue.rs:67
- AtomicRefQueue::pop_ref_conditional updates start_idx using the slice-relative
idxand compares it to the absolutestart_idx. When start_idx > 0 this can store a smaller (relative) value and cause rescans (or prevent advancing at all), defeating the optimization and potentially looping over already-checked entries.
if update_start_idx && idx > start_idx {
// Just use the current idx as the start idx, even though the current is currently being
// returned and marked. Prevents the need for checking edge cases e.g. if the current idx is the
// last.
//
opentmk/opentmk_invariant/src/functions/hyperv.rs:53
- hvcall allocates
vec![0; input_len]directly from the fuzzer-providedinput_len. Even though HvTestCtx::hypercall clamps to the hypercall page size internally, this allocation can still be attacker-controlled and cause OOM / watchdog resets.
let meta = unpack_hvcall_meta(meta.expect_int("meta")?);
let input = input.expect_int("input")? as usize;
let input_len = input_len.expect_int("input_len")? as usize;
// Read in the input page from `input` (only if any input is
// expected — `void`-input hypercalls send `input_len == 0`).
let mut hvc_lock = CALLS.lock();
let (hvc, init) = &mut *hvc_lock;
if !*init {
hvc.init(Vtl::Vtl0)
.map_err(|e| format!("Failed to initialize HvTestCtx: {e}"))?;
*init = true;
}
let mut in_args = vec![0; input_len];
match mem.try_read_mem(input, &mut in_args) {
opentmk/inv_decoder/src/lib.rs:686
- read_arg can panic on malformed input (
cnt == 0) and also usestodo!()for arg type 3. Since this parser processes untrusted on-wire program data, these should be surfaced as recoverable decode errors (Err) rather than crashing the whole executor.
// Read out each data word.
let cnt = arg.size.div_ceil(8);
if cnt == 0 {
panic!("data argument with size of zero!?");
}
let mut words = Vec::new();
for _i in 0..cnt {
words.push(read_inc_input(buf).context("unexpected end of input")?);
}
Arg::Data((arg, words))
}
// arg_csum
0x3 => todo!(),
// Catchall
_ => bail!("unsupported argument type: {typ}"),
opentmk/inv_decoder/src/lib.rs:307
- exec_single uses SafeMemoryMap::write_mem for ARG_DATA copyins. write_mem panics on partial/failed writes, so malformed/untrusted addresses can crash the decoder instead of returning a structured error to the caller.
Arg::Data((a, d)) => {
mem.write_mem(
(i.wire.addr + COPYIN_OFFSET) as usize,
&d.as_bytes()[..a.size as usize],
);
}
Cargo.toml:477
- Switching the workspace
anyhowdependency todefault-features = falsechanges the default for every crate that usesanyhow.workspace = true. Only a couple of crates were updated to explicitly re-enablefeatures = ["std"], but many other std crates (e.g. flowey_cli) still depend onanyhow.workspace = trueand will now build against a no-std-configured anyhow, which is likely to cause build breaks or behavior changes (missing std/backtrace integration).
# --- Error handling ---
anyhow = { version = "1.0", default-features = false }
thiserror = { version = "2", default-features = false }
opentmk/opentmk_invariant/src/comms/mod.rs:122
- read_packet_blocking trusts the on-wire payload size. A large
payload_szcan cause huge allocations (viaVec::with_capacity(payload_sz as usize)) and long blocking reads, which is an easy DoS vector if the serial stream is corrupted or hostile.
//2. Read packet size
let payload_sz = self.read_u64_blocking();
//3. Read payload
let mut buffer: Vec<u8> = Vec::with_capacity(payload_sz as usize);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated 3 comments.
Suppressed comments (6)
Previously missed (3) — in code that hasn't changed since the last review.
opentmk/inv_decoder/src/atomicrefqueue.rs:68
idxfromenumerate()is relative toself.data[start_idx..], but the code compares it tostart_idx(an absolute index) and stores the relativeidxback intostart_idx. Oncestart_idx != 0, this preventsstart_idxfrom advancing (and can even move it backwards), causing unnecessary rescans.
if update_start_idx && idx > start_idx {
// Just use the current idx as the start idx, even though the current is currently being
// returned and marked. Prevents the need for checking edge cases e.g. if the current idx is the
// last.
//
opentmk/opentmk_invariant/src/main.rs:8
- Doc comment has a few grammar/spelling issues (e.g., "customly" and missing terminal punctuation), which makes the crate-level documentation harder to read/search.
//! Opentmk Invariant is a bare-bones operating system based off of opentmk
//! framework used to accept customly crafted program consisting of an encoded
//! series of functions that would invoke specific functions into this OS
opentmk/opentmk_invariant/src/prelude.rs:5
- Doc comment grammar: "a extended" should be "an extended", and "rust" should be capitalized as "Rust" in docs.
//! This is a extended prelude crate that imports a number of common rust API entities that
//! would've been imported from the `alloc` crate.
opentmk/opentmk_invariant/src/functions/hvcall_meta.rs:31
size_ofisn't in scope here, so this assertion will fail to compile (cannot find functionsize_ofin this scope). Prefer fully qualifying the call (or importing it explicitly) to make the check robust.
const _: () = assert!(size_of::<HvcallMeta>() == 8);
opentmk/opentmk_invariant/src/deserializer/syzlang/mod.rs:149
src.len()is used to sliceself.tc_slicewithout bounds checking. Since the testcase bytes originate from the serial packet, an oversized payload will panic here and take down the runtime rather than returning a structured error.
let src = &testcase.testcase_vcpu0.as_slice();
self.tc_slice[..src.len()].copy_from_slice(src);
self.mem.0.as_mut_slice().fill(0);
opentmk/opentmk_invariant/src/executor/mod.rs:96
- Method names
on_recieve_ack_packet/on_recieve_error_packetare misspelled (recieve->receive). Since these are part of the executor's API surface and are used at call sites, it’s better to fix the spelling now before the interface is consumed more widely.
OpenTMKPacket::Ack(a) => self.on_recieve_ack_packet(&a)?,
OpenTMKPacket::Error(a) => self.on_recieve_error_packet(&a)?,
};
There was a problem hiding this comment.
🟡 Changes recommended
There is a confirmed indexing bug in AtomicRefQueue’s start_idx update logic and the workspace-wide anyhow feature flip is likely to cause widespread unintended build/behavior changes unless re-scoped.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
Previously missed (4) — in code that hasn't changed since the last review.
opentmk/inv_decoder/src/atomicrefqueue.rs:67
start_idxupdate usesidxfromenumerate()(relative tostart_idx) but compares it to/stores it as if it were an absolute index. This preventsstart_idxfrom advancing correctly oncestart_idx != 0, and can also movestart_idxbackwards, leading to repeated rescans and O(n^2) behavior under contention.
opentmk/inv_decoder/Cargo.toml:12inv_decoderis#![no_std], so it should explicitly disableanyhowdefault features to avoid accidentally pulling instdif the workspace dependency is std-enabled (which is the typical configuration for the rest of the workspace).
opentmk/opentmk_invariant/src/functions/io_port/mod.rs:100- Typo in comment/log message: this path uses
arch::io::inw, but the text saysinh. This makes grepping/debugging I/O port reads harder.
opentmk/opentmk_invariant/src/main.rs:8 - Doc comment has a spelling/grammar issue: “customly crafted program” should be “custom-crafted program” (or similar) to read correctly.
- Files reviewed: 28/29 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
There is at least one confirmed build-breaking issue (missing size_of import/qualification) plus a concrete indexing bug affecting the new AtomicRefQueue optimization logic.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
opentmk/inv_decoder/src/atomicrefqueue.rs:67
idxfromenumerate()is relative tostart_idx, but the code compares it to (and stores it into) the absolutestart_idx. This preventsstart_idxfrom ever advancing in the sequential case and can even move it backwards if the condition were changed, leading to unnecessary rescans.
opentmk/opentmk_invariant/src/functions/hvcall_meta.rs:31
size_ofisn’t in scope in this module, so this compile-time assert will fail to compile. Fully-qualify it (or importcore::mem::size_of) so the file builds standalone.
const _: () = assert!(size_of::<HvcallMeta>() == 8);
- Files reviewed: 28/29 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
It contains at least one confirmed logic issue (AtomicRefQueue’s start_idx update uses a relative index) plus a misleading size requirement doc comment that should be corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 26/27 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
A verified compile error (size_of not in scope) and a mismatched/incorrect public doc comment should be fixed before approval.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
opentmk/inv_decoder/src/lib.rs:825
- This doc comment refers to an
addr_sizeparameter that doesn’t exist and also claims0x1000000is 4MB (it’s 16MiB). Update it to describe the actualsyz_exec_mem/syz_input_buffersizing requirements.
opentmk/opentmk_invariant/src/functions/hvcall_meta.rs:31
size_ofis not in scope here (and there’s no local import), so this assertion won’t compile. Fully-qualify it tocore::mem::size_of(or add an explicit import).
const _: () = assert!(size_of::<HvcallMeta>() == 8);
- Files reviewed: 26/27 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
inv_decoder can return early on dependent-call failure without marking results executed, which can block later scheduling and cause incomplete/incorrect program execution.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
opentmk/inv_packet/src/lib.rs:39
OpenTMKGrammarDeserializer::Noneis documented as “No encoding — just use raw bytes”, but the executor currently treatsNoneas an invalid configuration (NoDeserializerEnabled). Either support the raw-bytes mode or update this doc comment to match the actual behavior to avoid misleading protocol users.
- Files reviewed: 26/27 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
There are confirmed correctness/compile issues in newly added code paths (e.g., missing size_of qualification/import and unchecked u64→usize payload sizing in comms) that should be fixed before approval.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
opentmk/opentmk_invariant/src/comms/mod.rs:89
payload_szis read as au64but then used both as ausize(Vec capacity) and as the loop bound. On 32-bit targets this can truncate and/or make it impossible to allocate/read consistently. Convert tousizewith a checked conversion before allocating/looping.
opentmk/inv_packet/src/lib.rs:40OpenTMKGrammarDeserializer::Noneis documented as a valid “raw bytes” mode, but the executor currently treats it as an error (NoDeserializerEnabled). Either implement the raw-bytes mode or update this doc comment to reflect thatNoneis not supported.
opentmk/opentmk_invariant/src/functions/hvcall_meta.rs:31
size_ofis referenced without being in scope; this won’t compile unlesssize_ofhas been imported. Prefer fully qualifying it in the const-assert to avoid relying on an implicit import.
const _: () = assert!(size_of::<HvcallMeta>() == 8);
- Files reviewed: 27/28 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
There are verified compile-breaking issues in newly added code paths (X86_REGISTRY iteration destructuring and an unqualified size_of use) that must be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
opentmk/opentmk_invariant/src/functions/hvcall_meta.rs:31
size_of::<HvcallMeta>()is used without being in scope; this file doesn't import it, andsize_ofis not in the Rust prelude. This will fail to compile (cannot find functionsize_ofin this scope). Prefer fully qualifying it.
const _: () = assert!(size_of::<HvcallMeta>() == 8);
- Files reviewed: 27/28 changed files
- Comments generated: 1
- Review effort level: Lite
This adds invariant runner/executor on top of opentmk
Invariant is a fuzzer framework, with opentmk-invariant being the opentmk program that takes in a set of commands (currently only via serial I/O) and executes them determinant on the invariant fuzzer.