Index out of bounds in `regspec_label`, accessible in user code through `RegSpec::mask` and `Display`

[5225225]

fn main() {
    let reg = yaxpeax_x86::protected_mode::RegSpec::mask(31);
    dbg!(format!("{}", reg));
}

In miri:

error: Undefined Behavior: pointer arithmetic failed: alloc1285 has size 2944, so pointer to 3056 bytes starting at offset 0 is out-of-bounds
   --> /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/const_ptr.rs:283:18
    |
283 |         unsafe { intrinsics::offset(self, count) }
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ pointer arithmetic failed: alloc1285 has size 2944, so pointer to 3056 bytes starting at offset 0 is out-of-bounds
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information

    = note: inside `std::ptr::const_ptr::<impl *const &str>::offset` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/const_ptr.rs:283:18
    = note: inside `std::ptr::const_ptr::<impl *const &str>::add` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/const_ptr.rs:564:18
    = note: inside `<usize as std::slice::SliceIndex<[&str]>>::get_unchecked` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/index.rs:177:18
    = note: inside `core::slice::<impl [&str]>::get_unchecked::<usize>` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/mod.rs:385:20
    = note: inside `yaxpeax_x86::protected_mode::display::regspec_label` at /home/jess/.cargo/registry/src/github.com-1ecc6299db9ec823/yaxpeax-x86-1.1.2/src/protected_mode/display.rs:126:14
    = note: inside `yaxpeax_x86::protected_mode::display::<impl std::fmt::Display for yaxpeax_x86::protected_mode::RegSpec>::fmt` at /home/jess/.cargo/registry/src/github.com-1ecc6299db9ec823/yaxpeax-x86-1.1.2/src/protected_mode/display.rs:131:21
    = note: inside `std::fmt::write` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:1149:17
    = note: inside `<std::string::String as std::fmt::Write>::write_fmt` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:186:9
    = note: inside `std::fmt::format` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/fmt.rs:597:5
note: inside `main` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/macros.rs:111:19
   --> src/main.rs:3:10
    |
3   |     dbg!(format!("{}", reg));
    |          ^^^^^^^^^^^^^^^^^^
    = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
    = note: inside `std::sys_common::backtrace::__rust_begin_short_backtrace::<fn(), ()>` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys_common/backtrace.rs:123:18
    = note: inside closure at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:145:18
    = note: inside `std::ops::function::impls::<impl std::ops::FnOnce<()> for &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>::call_once` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:259:13
    = note: inside `std::panicking::r#try::do_call::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:406:40
    = note: inside `std::panicking::r#try::<i32, &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:370:19
    = note: inside `std::panic::catch_unwind::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:133:14
    = note: inside closure at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:128:48
    = note: inside `std::panicking::r#try::do_call::<[closure@std::rt::lang_start_internal::{closure#2}], isize>` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:406:40
    = note: inside `std::panicking::r#try::<isize, [closure@std::rt::lang_start_internal::{closure#2}]>` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:370:19
    = note: inside `std::panic::catch_unwind::<[closure@std::rt::lang_start_internal::{closure#2}], isize>` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:133:14
    = note: inside `std::rt::lang_start_internal` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:128:20
    = note: inside `std::rt::lang_start::<()>` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:144:17
    = note: this error originates in the macro `format` (in Nightly builds, run with -Z macro-backtrace for more info)

Outside of miri:

memory allocation of 94086415520320 bytes failed
zsh: abort (core dumped)  cargo run -q

This presumably just interprets some arbitrary 16 byte chunk of memory near REG_NAMES as a (pointer, length).

This wasn't a fuzzer bug, this was me just looking for dubious unsafe and this was the first one I looked at. Is this function performance critical?

Honestly I'd be inclined to drop the get_unchecked and just pay the cost of the bounds check. (This goes for all places that use get_unchecked to avoid a bounds check, not just here).

[iximeow]

ah dang, this is a really good find, thank you. your assessment is correct, unfortunately "mask registers" above 24 be totally arbitrary. 0-7 are correct and get names k0-k7, 8-15 are buggy and would get names eip or BUG, 16-23 are the same but for eflags, and 24+ is where things go off the rails.

the bound for fn mask(num: u8) should be 8 for 32- and 16-bit modes, but instead it's a copy-paste error. sigh.

thankfully, x86 doesn't have mask registers above k7 so (barring another bug) this is only reachable by users creating bogus mask registers. given that this was once correct, and eventually became incorrect through refactors, i added some tests that these helpers panic for invalid register numbers as they should. that also caught that the 64-bit RegSpec::rb helper had an incorrect limit (18 instead of 16...?) so that's fixed too. in that case it would just display as cr0 or cr1. they wouldn't compare equal to the control registers same as how the invalid mask registers won't make for invalid logic working with other mask registers - except for the Display impl.

as for get_unchecked, it's pretty noticeable when benchmarking display impl performance. i'd actually like to make the MEM_SIZE_STRINGS lookup also a get_unchecked, but as you've noticed in #15 there's more fuzzing to do first.

... incidentally, i really appreciate that you've reported these bugs, even if it has turned out that yaxpeax-x86 keels over at the lightest of attention. would you like attribution for these in the release notes? and if so, how would you like me to attribute you?

[5225225]

as for get_unchecked, it's pretty noticeable when benchmarking display impl performance.

That's fair, but maybe have a get_kinda_checked helper method that is still unsafe, but does a debug_assert that it's in bounds? And then never use the "normal" get_unchecked, but instead always use that one. Would make it less likely to go unnoticed, and would be zero cost in release mode, since the debug assert gets removed. Could also do the same thing for the use of unreachable_unchecked.

In a perfect world, the stdlib would do that for you, but :shrug:

... incidentally, i really appreciate that you've reported these bugs, even if it has turned out that yaxpeax-x86 keels over at the lightest of attention. would you like attribution for these in the release notes? and if so, how would you like me to attribute you?

:sweat_smile: no worries, fuzzers are just really good at finding bugs (citation: my 30-40+ bugs reported over the last year or so). Thankfully, none of the ones found through parsing malformed input seem exploitable or anything, it's just a panic.

https://github.com/pyfisch/httpdate/releases/tag/v1.0.2 just said

See #8, thank you @5225225!

If you mean what name I use, just use @5225225 that's cool and good, and yeah, fine to attribute me in the release notes ty ty

---

also the default branch name on this repo is great i love it

one of the more creative ones i've seen so far :D

[iximeow]

If you mean what name I use, just use @5225225 that's cool and good, and yeah, fine to attribute me in the release notes ty ty

done, though i was also wondering if you prefer github @, another name, an email, whichever you prefer.

That's fair, but maybe have a get_kinda_checked helper method that is still unsafe, but does a debug_assert that it's in bounds? And then never use the "normal" get_unchecked, but instead always use that one.

well overdue for doing this too. noted in #16 and, well, we'll see when it's actually done :D

[5225225]

though i was also wondering if you prefer github @, another name, an email, whichever you prefer.

Nah, if it's just a link to my github profile then I can keep whatever info I have on my github up to date

thanks for asking though :3