From 110f797005cca70e18cbcc0975397d26d8045245 Mon Sep 17 00:00:00 2001 From: iximeow Date: Sat, 16 Dec 2023 13:26:48 -0800 Subject: fix opportunity for unhandled register synonyms registers `al`, `cl`, `dl`, and `bl` could have two different representations - with `rex.w` and without. these two forms of `RegSpec` would not compare equal, nor has the same, so for code relying on `RegSpec` to faithfully represent a 1-1 mapping to x86 registers, these synonyms would introduce bugs in register analysis. for example, in `yaxpeax-core`, this would result in instructions writing to `rex.w al` not being visible as definitions for a future read of `!rex.w al`. fix this in `x86_64` code, add new test cases about the confusion, adjust register names to make this situation more clearly a bug, and introduce two new fuzz targets that would have helped spot this error. --- fuzz/Cargo.toml | 12 +++++ .../does_not_decode_invalid_registers.rs | 30 ++++++++++++ .../small_reg_is_always_old_bank_if_possible.rs | 56 ++++++++++++++++++++++ 3 files changed, 98 insertions(+) create mode 100644 fuzz/fuzz_targets/does_not_decode_invalid_registers.rs create mode 100644 fuzz/fuzz_targets/small_reg_is_always_old_bank_if_possible.rs (limited to 'fuzz') diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 53bcb81..60690f6 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -32,3 +32,15 @@ name = "display_c_does_not_panic" path = "fuzz_targets/display_c_does_not_panic.rs" test = false doc = false + +[[bin]] +name = "does_not_decode_invalid_registers" +path = "fuzz_targets/does_not_decode_invalid_registers.rs" +test = false +doc = false + +[[bin]] +name = "small_reg_is_always_old_bank_if_possible" +path = "fuzz_targets/small_reg_is_always_old_bank_if_possible.rs" +test = false +doc = false diff --git a/fuzz/fuzz_targets/does_not_decode_invalid_registers.rs b/fuzz/fuzz_targets/does_not_decode_invalid_registers.rs new file mode 100644 index 0000000..0f32f73 --- /dev/null +++ b/fuzz/fuzz_targets/does_not_decode_invalid_registers.rs @@ -0,0 +1,30 @@ +//! instruction text should never include the word BUG - this is a symptom of selecting an invalid +//! RegSpec while disassembling. + +#![no_main] +#[macro_use] extern crate libfuzzer_sys; +extern crate yaxpeax_x86; + +fuzz_target!(|data: &[u8]| { + let x86_64_decoder = yaxpeax_x86::long_mode::InstDecoder::default(); + let x86_32_decoder = yaxpeax_x86::protected_mode::InstDecoder::default(); + let x86_16_decoder = yaxpeax_x86::real_mode::InstDecoder::default(); + + if let Ok(inst) = x86_64_decoder.decode_slice(data) { + let mut res = String::new(); + inst.write_to(&mut res).expect("format does not panic"); + assert!(!res.contains("BUG")); + }; + + if let Ok(inst) = x86_32_decoder.decode_slice(data) { + let mut res = String::new(); + inst.write_to(&mut res).expect("format does not panic"); + assert!(!res.contains("BUG")); + }; + + if let Ok(inst) = x86_16_decoder.decode_slice(data) { + let mut res = String::new(); + inst.write_to(&mut res).expect("format does not panic"); + assert!(!res.contains("BUG")); + }; +}); diff --git a/fuzz/fuzz_targets/small_reg_is_always_old_bank_if_possible.rs b/fuzz/fuzz_targets/small_reg_is_always_old_bank_if_possible.rs new file mode 100644 index 0000000..a143205 --- /dev/null +++ b/fuzz/fuzz_targets/small_reg_is_always_old_bank_if_possible.rs @@ -0,0 +1,56 @@ +//! if a register has a single-byte register operand, and it's one of `al`, `bl`, `cl`, or `dl`, it +//! should compare equal to the `RegSpec` produced by `RegSpec::al()` and so on. +//! +//! at one point this was a bug; `RegSpec::al()` would use `RegisterBank::B`, but an instruction +//! with `rex.w` set could get an `al` backed by a `RegSpec` in `RegisterBank::rB`. + +#![no_main] +#[macro_use] extern crate libfuzzer_sys; +extern crate yaxpeax_x86; + +// this test is not meaningful for 32-bit or 16-bit modes, there are no register synonyms in those +// cases. leaving them in for fuzz targets to match other cases, and In Case Of Future Change. +fuzz_target!(|data: &[u8]| { + let x86_64_decoder = yaxpeax_x86::long_mode::InstDecoder::default(); + let x86_32_decoder = yaxpeax_x86::protected_mode::InstDecoder::default(); + let x86_16_decoder = yaxpeax_x86::real_mode::InstDecoder::default(); + + if let Ok(inst) = x86_64_decoder.decode_slice(data) { + for i in 0..inst.operand_count() { + match inst.operand(i) { + yaxpeax_x86::long_mode::Operand::Register(reg) => { + if reg.num() < 4 && reg.class() == yaxpeax_x86::long_mode::register_class::RB { + assert!(false, "instruction has rex.w register that aliases old byte registers"); + } else { + /* not a potentially-unwanted register */ + } + }, + _ => { /* not a relevant operand kind. immediate or memory of some kind. */ } + } + } + }; + + /* + if let Ok(inst) = x86_32_decoder.decode_slice(data) { + for i in 0..inst.operand_count() { + match inst.operand(i) { + Operand::Register(_reg) => { + /* not a potentially-unwanted register */ + }, + _ => { /* not a relevant operand kind. immediate or memory of some kind. */ } + } + } + }; + + if let Ok(inst) = x86_16_decoder.decode_slice(data) { + for i in 0..inst.operand_count() { + match inst.operand(i) { + Operand::Register(_reg) => { + /* not a potentially-unwanted register */ + }, + _ => { /* not a relevant operand kind. immediate or memory of some kind. */ } + } + } + }; + */ +}); -- cgit v1.1