iximeow / yaxpeax-x86

x86 decoders for the yaxpeax project
BSD Zero Clause License
129 stars 23 forks source link

Overflow in multiply when formatting instruction #15

Closed 5225225 closed 2 years ago

5225225 commented 2 years ago

(grr, the second time i hit enter and made an issue too early, sorry if you have email notifs on!)

fn main() {
    let decoder = yaxpeax_x86::amd64::InstDecoder::default();
    let mut s = String::new();
    let i = decoder.decode_slice(&[243, 15, 30, 15]).expect("unreachable");
    i.write_to(&mut s).expect("unreachable");
}

Gives a multiply with overflow with integer overflow checks on, gives a index out of bounds panic (thankfully not unchecked indexing) in release

5225225 commented 2 years ago

The fuzzer I used to find this is just

#![no_main]
use libfuzzer_sys::fuzz_target;

fuzz_target!(|data: &[u8]| {
    let decoder = yaxpeax_x86::amd64::InstDecoder::default();
    let mut s = String::new();
    if let Ok(i) = decoder.decode_slice(data) {
        i.write_to(&mut s).expect("writing to never fail");
    }
});
iximeow commented 2 years ago

i see this yield a attempt to subtract with overflow, rather than multiply? the cause of this is a little deeper than just the formatting though: the nop instruction is reported as having a memory operand, and then never gets an explicit mem_size, so it keeps the default of 0. then in..

out.write_str(MEM_SIZE_STRINGS[instr.mem_size as usize - 1])

you can see why it would panic .. :D

fixed in e7dec7b.

and then i've added fuzz targets for both decoding and displaying instructions in 26e019c. i'm gonna let cargo fuzz go a while longer before cutting a new release, though, since i doubt this is the only instruction i missed when adding the explicit mem_size field.