iximeow / yaxpeax-x86

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

Another multiply with overflow panic #13

Closed 5225225 closed 2 years ago

5225225 commented 2 years ago
fn main() {
    let decoder = yaxpeax_x86::amd64::InstDecoder::default();
    drop(decoder.decode_slice(&[98, 242, 253, 15, 138, 98, 242, 253, 15, 138]));
}

thread 'main' panicked at 'attempt to multiply with overflow', /home/jess/.cargo/registry/src/github.com-1ecc6299db9ec823/yaxpeax-x86-1.1.2/src/long_mode/../shared/evex.in:241:11

The fuzzer I'm using is just

#![no_main]
use libfuzzer_sys::fuzz_target;

fuzz_target!(|data: &[u8]| {
    let decoder = yaxpeax_x86::amd64::InstDecoder::default();
    drop(decoder.decode_slice(data));
});

(the drop is to ignore the must_use on a Result).

Probably should add this to the repo and run it yourself. See https://github.com/rust-fuzz/cargo-fuzz

iximeow commented 2 years ago

i was wondering how you were fuzzing - i'd written off cargo fuzz maybe a little too eagerly as i figured it would mostly-uninterestingly be exploring evex decoding logic. which, i suppose, is exactly what could have used some more scrutiny.

this is a foil of the other overflow you found

        if let Some(size) = overridden_size {
          instruction.disp *= size;
        } else {
          apply_disp_scale(instruction);
        }

where disp is signed-in-spirit-but-unsigned-in-practice. so that's a wrapping_mul now. and the other fix probably should have been wrapping_mul too. so it is now.

on cargo fuzz: i figured out that all the cargo fuzz issues i've had in the past were due to my having an ancient version of cargo fuzz installed. i was wondering how you got it working without fiddling with llvm pass manager arguments and other weird hacks... it doesn't seem to be shouting about specific fuzz cases so maybe these were the only two? haha :grimacing:

5225225 commented 2 years ago

Yeah, I think there was a brief time maybe a few months ago where cargo-fuzz didn't work out of the box (because rust was moving to a different llvm / pass manager or whatever it was).

But nowadays (cargo-fuzz 0.11.0), it works flawlessly for me. On linux, granted, so your mileage may vary if you're on windows/mac.

iximeow commented 2 years ago

between the immediate issue being fixed, and that there are now in-tree cargo fuzz targets for this (and that i'm running them), i'm going to close this as being resolved