diff --git a/Makefile b/Makefile index 21be92a..4874774 100644 --- a/Makefile +++ b/Makefile @@ -98,9 +98,9 @@ test: test_common .PHONY: clean clean: $(CARGO) clean $(CARGO_MANIFEST) - rm $(seabios_blob) + rm -f $(seabios_blob) make -C seabios clean - rm $(GUEST_ASSETS) + rm -f $(GUEST_ASSETS) .PHONY: dev-init dev-init: install-git-hooks diff --git a/mythril/Cargo.lock b/mythril/Cargo.lock index 1fc5619..5f6c073 100644 --- a/mythril/Cargo.lock +++ b/mythril/Cargo.lock @@ -172,6 +172,7 @@ dependencies = [ "serde", "serde_json", "spin", + "testing_logger", "ux", "x86", ] @@ -343,6 +344,15 @@ dependencies = [ "unicode-xid", ] +[[package]] +name = "testing_logger" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6d92b727cb45d33ae956f7f46b966b25f1bc712092aeef9dba5ac798fc89f720" +dependencies = [ + "log", +] + [[package]] name = "unicode-xid" version = "0.2.1" diff --git a/mythril/Cargo.toml b/mythril/Cargo.toml index 6081519..3cf59cb 100644 --- a/mythril/Cargo.toml +++ b/mythril/Cargo.toml @@ -29,6 +29,9 @@ spin = "0.5" ux = { version = "0.1.3", default-features = false } managed = { version = "0.8.0", features = ["map", "alloc"], default-features = false } +[dev-dependencies] +testing_logger = "0.1.1" + [dependencies.arrayvec] version = "0.5.2" default-features = false diff --git a/mythril/src/acpi/madt.rs b/mythril/src/acpi/madt.rs index a29e61a..9ebd51d 100644 --- a/mythril/src/acpi/madt.rs +++ b/mythril/src/acpi/madt.rs @@ -80,10 +80,8 @@ impl IcsType { if length == self.expected_len() as usize - 2 { Ok(()) } else { - Err(Error::InvalidValue(format!( - "Invalid length={} for type=0x{:x}", - *self as u8, length - ))) + error!("Invalid length={} for type=0x{:x}", *self as u8, length); + Err(Error::InvalidValue) } } } @@ -254,11 +252,14 @@ impl Ics { ), apic_proc_uid: NativeEndian::read_u32(&bytes[10..14]), }), - _ => Err(Error::NotImplemented(format!( - "type=0x{:x} length={} not implemented", - ty as u8, - bytes.len() - ))), + _ => { + error!( + "type=0x{:x} length={} not implemented", + ty as u8, + bytes.len() + ); + Err(Error::NotImplemented) + } } } @@ -294,10 +295,11 @@ impl Ics { NativeEndian::write_u32(&mut tmp_buf[8..12], gsi_base); } _ => { - return Err(Error::NotImplemented(format!( + error!( "The ICS Type {:?} has not been implemented", self.ics_type() - ))) + ); + return Err(Error::NotImplemented); } } buffer.try_extend_from_slice( @@ -393,26 +395,23 @@ impl<'a> Iterator for IcsIterator<'a> { let ty = match IcsType::try_from(self.bytes[0]) { Ok(ty) => ty, _ => { - return Some(Err(Error::InvalidValue(format!( - "Invalid ICS type: {}", - self.bytes[0] - )))); + error!("Invalid ICS type: {}", self.bytes[0]); + return Some(Err(Error::InvalidValue)); } }; let len = self.bytes[1] as usize; if len > self.bytes.len() { - return Some(Err(Error::InvalidValue(format!( + error!( "Payload for type=0x{:x} and len={} to big for buffer len={}", ty as u8, len, self.bytes.len() - )))); + ); + return Some(Err(Error::InvalidValue)); } else if len < 3 { - return Some(Err(Error::InvalidValue(format!( - "length `{}` provided is too small", - len, - )))); + error!("length `{}` provided is too small", len); + return Some(Err(Error::InvalidValue)); } let bytes = &self.bytes[2..len]; diff --git a/mythril/src/acpi/mod.rs b/mythril/src/acpi/mod.rs index 4b157ff..05983e7 100644 --- a/mythril/src/acpi/mod.rs +++ b/mythril/src/acpi/mod.rs @@ -48,11 +48,12 @@ pub(self) fn verify_checksum(bytes: &[u8], cksum_idx: usize) -> Result<()> { if (result & 0xff) == 0x00 { Ok(()) } else { - Err(Error::InvalidValue(format!( + error!( "Checksum mismatch checksum={:x} {:x} != 0x00", bytes[cksum_idx], - result & 0xff, - ))) + result & 0xff + ); + Err(Error::InvalidValue) } } @@ -137,11 +138,12 @@ impl GenericAddressStructure { /// Create a new GAS from a slice of bytes. pub fn new(bytes: &[u8]) -> Result { if bytes.len() != GAS_SIZE { - return Err(Error::InvalidValue(format!( + error!( "Invalid number of bytes for GAS: {} != {}", bytes.len(), GAS_SIZE - ))); + ); + return Err(Error::InvalidValue); } let address_space = @@ -168,10 +170,8 @@ impl GenericAddressStructure { // verify that the address is only 32 bits for 32-bit platforms. if !is_64bit && ((address >> 32) & 0xFFFFFFFF) != 0 { - return Err(Error::InvalidValue(format!( - "Invalid address for a 32-bit system: {:x}", - address - ))); + error!("Invalid address for a 32-bit system: {:x}", address); + return Err(Error::InvalidValue); } } diff --git a/mythril/src/acpi/rsdp.rs b/mythril/src/acpi/rsdp.rs index 9a8e787..62fd08e 100644 --- a/mythril/src/acpi/rsdp.rs +++ b/mythril/src/acpi/rsdp.rs @@ -26,6 +26,7 @@ pub const RSDP_SIGNATURE: &[u8; 8] = b"RSD PTR "; /// Offsets from `ACPI ยง 5.2.5.3` pub mod offsets { use super::*; + /// Well known bytes, "RST PTR ". pub const SIGNATURE: Range = 0..8; /// Checksum of the fields defined by ACPI 1.0. @@ -116,11 +117,8 @@ impl RSDP { xsdt_addr: NativeEndian::read_u64(&bytes[offsets::XSDT_ADDR]), }, _ => { - return Err(Error::InvalidValue(format!( - "Invalid RSDP revision: {}", - bytes[offsets::REVISION] - ))) - .into(); + error!("Invalid RSDP revision: {}", bytes[offsets::REVISION]); + return Err(Error::InvalidValue).into(); } }; @@ -178,11 +176,14 @@ impl RSDP { 2 if rsdp_v2_end < range.len() => { Ok(&range[i..rsdp_v2_end]) } - _ => Err(Error::InvalidValue(format!( - "Invalid RSDP revision: {} at {:p}", - candidate[offsets::REVISION], - candidate.as_ptr() - ))), + _ => { + error!( + "Invalid RSDP revision: {} at {:p}", + candidate[offsets::REVISION], + candidate.as_ptr() + ); + Err(Error::InvalidValue) + } }; } } @@ -203,10 +204,10 @@ impl RSDP { &bytes[..offsets::RESERVED.end], offsets::EXT_CHECKSUM, ), - _ => Err(Error::InvalidValue(format!( - "Invalid RSDP revision: {}", - bytes[offsets::REVISION] - ))), + _ => { + error!("Invalid RSDP revision: {}", bytes[offsets::REVISION]); + Err(Error::InvalidValue) + } } } diff --git a/mythril/src/acpi/rsdt.rs b/mythril/src/acpi/rsdt.rs index 8a57114..993971e 100644 --- a/mythril/src/acpi/rsdt.rs +++ b/mythril/src/acpi/rsdt.rs @@ -246,11 +246,12 @@ fn write_sdt_header( // The SDT length value is the value of the entire SDT including // the header. if buffer.len() < sdt_len { - return Err(Error::InvalidValue(format!( + error!( "Buffer length should be at least `{}` but was `{}`", sdt_len, buffer.len() - ))); + ); + return Err(Error::InvalidValue); } // Fill in the SDT header with the implementations values buffer[offsets::SIGNATURE].copy_from_slice(signature); @@ -339,10 +340,11 @@ impl<'a, T: Array> RSDTBuilder<'a, T> { Ok(()) } } else { - Err(Error::InvalidValue(format!( + error!( "The key `{}` already exists", str::from_utf8(&U::SIGNATURE).unwrap() - ))) + ); + Err(Error::InvalidValue) } } @@ -373,16 +375,22 @@ impl<'a, T: Array> RSDTBuilder<'a, T> { })?; for (i, (name, (sdt, size))) in self.map.iter().enumerate() { - let table_name = format!("etc/mythril/{}", str::from_utf8(name)?); + const ETC_MYTHRIL: &'static str = "etc/mythril/"; + const LEN_OF_NAME: usize = 4; + let mut table_name_bytes = [0u8; ETC_MYTHRIL.len() + LEN_OF_NAME]; + table_name_bytes[0..ETC_MYTHRIL.len()] + .copy_from_slice(ETC_MYTHRIL.as_bytes()); + table_name_bytes[ETC_MYTHRIL.len()..].copy_from_slice(name); + let table_name = str::from_utf8(&table_name_bytes)?; table_loader.add_command(TableLoaderCommand::Allocate { - file: &table_name, + file: table_name, align: 8, zone: AllocZone::Fseg, })?; table_loader.add_command(TableLoaderCommand::AddPointer { - src: &table_name, + src: table_name, dst: "etc/mythril/xsdt", offset: ((i * 8) + offsets::CREATOR_REVISION.end) as u32, size: 8, diff --git a/mythril/src/emulate/controlreg.rs b/mythril/src/emulate/controlreg.rs index b683bbd..2b7348f 100644 --- a/mythril/src/emulate/controlreg.rs +++ b/mythril/src/emulate/controlreg.rs @@ -60,9 +60,8 @@ pub fn emulate_access( op => panic!("Unsupported MovFromCr cr0 operation: {:?}", op), }, _ => { - return Err(Error::InvalidValue(format!( - "Unsupported CR number access" - ))) + error!("Unsupported CR number access"); + return Err(Error::InvalidValue); } } Ok(()) diff --git a/mythril/src/emulate/memio.rs b/mythril/src/emulate/memio.rs index ddfc982..1f3804f 100644 --- a/mythril/src/emulate/memio.rs +++ b/mythril/src/emulate/memio.rs @@ -34,7 +34,8 @@ macro_rules! read_register { ($out:ident, $value:expr, $type:ty) => {{ let data = ($value as $type).to_be_bytes(); $out.try_extend_from_slice(&data).map_err(|_| { - Error::InvalidValue("Invalid length with reading register".into()) + error!("Invalid length with reading register"); + Error::InvalidValue })?; }}; } @@ -143,10 +144,8 @@ fn read_register_value( iced_x86::Register::RBP => read_register!(res, guest_cpu.rbp, u64), _ => { - return Err(Error::InvalidValue(format!( - "Invalid register '{:?}'", - register - ))) + error!("Invalid register '{:?}'", register); + return Err(Error::InvalidValue); } } @@ -352,10 +351,8 @@ fn do_mmio_read( } register => { - return Err(Error::InvalidValue(format!( - "mmio read into invalid register '{:?}'", - register - ))) + error!("mmio read into invalid register '{:?}'", register); + return Err(Error::InvalidValue); } }, _ => return Err(Error::NotSupported), @@ -433,12 +430,13 @@ fn process_memio_op( { do_mmio_read(addr, vcpu, guest_cpu, responses, instr, on_read)?; } else { - return Err(Error::InvalidValue(format!( + error!( "Unsupported mmio instruction: {:?} (rip=0x{:x}, bytes={:?})", instr.code(), ip, - bytes, - ))); + bytes + ); + return Err(Error::InvalidValue); } Ok(()) } diff --git a/mythril/src/error.rs b/mythril/src/error.rs index 873d80c..9e43aba 100644 --- a/mythril/src/error.rs +++ b/mythril/src/error.rs @@ -1,5 +1,4 @@ use crate::vmcs; -use alloc::string::String; use arrayvec::CapacityError; use core::convert::TryFrom; use core::num::TryFromIntError; @@ -44,25 +43,29 @@ pub enum VmInstructionError { InvalidOperandToInveptInvvpid = 28, } -pub fn check_vm_insruction(rflags: u64, error: String) -> Result<()> { +pub fn check_vm_instruction(rflags: u64, log_error: impl Fn()) -> Result<()> { let rflags = rflags::RFlags::from_bits_truncate(rflags); if rflags.contains(RFlags::FLAGS_CF) { - Err(Error::VmFailInvalid(error)) + log_error(); + Err(Error::VmFailInvalid) } else if rflags.contains(RFlags::FLAGS_ZF) { let errno = unsafe { let value: u64; - llvm_asm!("vmread %rax, %rdx;" - : "={rdx}"(value) - : "{rax}"(vmcs::VmcsField::VmInstructionError as u64) - : "rflags" - : "volatile"); + asm!( + "vmread rdx, rax", + in("rax") vmcs::VmcsField::VmInstructionError as u64, + out("rdx") value, + options(nostack) + ); value }; let vm_error = VmInstructionError::try_from(errno) .unwrap_or(VmInstructionError::UnknownError); - Err(Error::VmFailValid((vm_error, error))) + error!("{:?}", vm_error); + log_error(); + Err(Error::VmFailValid) } else { Ok(()) } @@ -70,40 +73,38 @@ pub fn check_vm_insruction(rflags: u64, error: String) -> Result<()> { #[derive(Debug, PartialEq)] pub enum Error { - Vmcs(String), - VmFailInvalid(String), - VmFailValid((VmInstructionError, String)), - DuplicateMapping(String), - AllocError(String), - MissingDevice(String), - MissingFile(String), - NullPtr(String), + Vmcs, + VmFailInvalid, + VmFailValid, + DuplicateMapping, NotSupported, NotFound, - Exists, Exhausted, - Uefi(String), - InvalidValue(String), - InvalidDevice(String), - NotImplemented(String), - DeviceError(String), + InvalidValue, + InvalidDevice, + NotImplemented, + DeviceError, + MissingDevice, } impl From> for Error { fn from(error: TryFromPrimitiveError) -> Error { - Error::InvalidValue(format!("{}", error)) + error!("{}", error); + Error::InvalidValue } } impl From for Error { fn from(error: TryFromIntError) -> Error { - Error::InvalidValue(format!("{}", error)) + error!("{}", error); + Error::InvalidValue } } impl From for Error { fn from(error: core::str::Utf8Error) -> Error { - Error::InvalidValue(format!("{}", error)) + error!("{}", error); + Error::InvalidValue } } @@ -137,7 +138,7 @@ fn panic_handler(info: &core::panic::PanicInfo) -> ! { loop { unsafe { // Try to at least keep CPU from running at 100% - llvm_asm!("hlt" :::: "volatile"); + asm!("hlt", options(nostack, nomem)); } } } diff --git a/mythril/src/interrupt/idt.rs b/mythril/src/interrupt/idt.rs index 1e25a6c..145c12f 100644 --- a/mythril/src/interrupt/idt.rs +++ b/mythril/src/interrupt/idt.rs @@ -79,43 +79,47 @@ pub struct FaultState { } macro_rules! push_regs { - () => (llvm_asm!( - "push rax - push rbx - push rcx - push rdx - push rdi - push rsi - push r8 - push r9 - push r10 - push r11 - push r12 - push r13 - push r14 - push r15" - : : : : "intel", "volatile" - )); + () => { + #[rustfmt::skip] + asm!( + "push rax", + "push rbx", + "push rcx", + "push rdx", + "push rdi", + "push rsi", + "push r8", + "push r9", + "push r10", + "push r11", + "push r12", + "push r13", + "push r14", + "push r15", + ) + }; } macro_rules! pop_regs { - () => (llvm_asm!( - "pop r15 - pop r14 - pop r13 - pop r12 - pop r11 - pop r10 - pop r9 - pop r8 - pop rsi - pop rdi - pop rdx - pop rcx - pop rbx - pop rax" - : : : : "intel", "volatile" - )); + () => { + #[rustfmt::skip] + asm!( + "pop r15", + "pop r14", + "pop r13", + "pop r12", + "pop r11", + "pop r10", + "pop r9", + "pop r8", + "pop rsi", + "pop rdi", + "pop rdx", + "pop rcx", + "pop rbx", + "pop rax", + ) + }; } macro_rules! interrupt_fn_impl { @@ -129,7 +133,11 @@ macro_rules! interrupt_fn_impl { push_regs!(); let rbp: usize; - llvm_asm!("" : "={rbp}"(rbp) : : : "volatile"); + asm!( + "mov {}, rbp", + out(reg) rbp, + options(nomem, nostack) + ); // Plus usize to skip the old rpb value pushed in the preamble let stack = &*( (rbp + core::mem::size_of::()) as *const $type); @@ -139,10 +147,9 @@ macro_rules! interrupt_fn_impl { // Remove this stack frame before the iretq. This should work // whether the above 'rbp' local variable is stack allocated or not. - llvm_asm!("mov rsp, rbp - pop rbp - iretq" - : : : : "intel", "volatile"); + asm!("mov rsp, rbp", + "pop rbp", + "iretq"); } } } diff --git a/mythril/src/interrupt/mod.rs b/mythril/src/interrupt/mod.rs index 8ea69d5..7b09e36 100644 --- a/mythril/src/interrupt/mod.rs +++ b/mythril/src/interrupt/mod.rs @@ -12,9 +12,9 @@ pub mod gsi { } pub unsafe fn enable_interrupts() { - llvm_asm!("sti" :::: "volatile"); + asm!("sti", options(nomem, nostack)); } pub unsafe fn disable_interrupts() { - llvm_asm!("cli" :::: "volatile"); + asm!("cli", options(nomem, nostack)); } diff --git a/mythril/src/ioapic.rs b/mythril/src/ioapic.rs index 9acd301..99e0894 100644 --- a/mythril/src/ioapic.rs +++ b/mythril/src/ioapic.rs @@ -117,6 +117,7 @@ pub struct IoApic { // IoApics are actually Send/Sync. This will not be correctly derived // because raw pointers are not send (even when protected by a mutex). unsafe impl Send for IoApic {} + unsafe impl Sync for IoApic {} impl IoApic { @@ -170,10 +171,8 @@ impl IoApic { /// See section 3.2.1 of the I/O APIC specification. pub fn set_id(&self, id: u32) -> Result<()> { if id > 0x0f { - Err(Error::InvalidValue(format!( - "I/O APIC ID `0x{:x}` too large", - id - ))) + error!("I/O APIC ID `0x{:x}` too large", id); + Err(Error::InvalidValue) } else { unsafe { self.write_raw(reg::IOAPICID, id << 24); @@ -211,10 +210,8 @@ impl IoApic { /// See section 3.2.3 of the I/O APIC specification. pub fn set_arbitration_id(&self, id: u8) -> Result<()> { if id > 15 { - Err(Error::InvalidValue(format!( - "I/O APIC Arbitration ID `0x{:x}` too large", - id - ))) + error!("I/O APIC Arbitration ID `0x{:x}` too large", id); + Err(Error::InvalidValue) } else { unsafe { self.write_raw(reg::IOAPICARB, (id as u32) << 24); @@ -238,10 +235,8 @@ impl IoApic { /// Read the IO Redirect Table Register for a given id. pub fn read_ioredtbl(&self, id: u8) -> Result { if id > 23 { - Err(Error::InvalidValue(format!( - "I/O APIC IO Redirect Table register`0x{:x}` too large", - id - ))) + error!("I/O APIC IO Redirect Table register`0x{:x}` too large", id); + Err(Error::InvalidValue) } else { let bits = unsafe { self.read_ioredtbl_raw(id) }; @@ -269,15 +264,14 @@ impl IoApic { pub fn write_ioredtbl(&self, id: u8, entry: IoRedTblEntry) -> Result<()> { let val: u64 = entry.into(); if id > 23 { - Err(Error::InvalidValue(format!( - "I/O APIC IO Redirect Table register`0x{:x}` too large", - id - ))) + error!("I/O APIC IO Redirect Table register`0x{:x}` too large", id); + Err(Error::InvalidValue) } else if (val & !IOREDTBL_RW_MASK) != 0 { - Err(Error::InvalidValue(format!( + error!( "Read-only IO Redirect Table Entry bits set: 0x{:x}", val & !IOREDTBL_RW_MASK - ))) + ); + Err(Error::InvalidValue) } else { unsafe { self.write_ioredtbl_raw(id, val); @@ -306,10 +300,13 @@ impl TryFrom for IoApic { gsi_base, .. } => IoApic::new(ioapic_addr, gsi_base), - _ => Err(Error::InvalidValue(format!( - "Attempting to create an IoApic from: {:?}", - value.ics_type() - ))), + _ => { + error!( + "Attempting to create an IoApic from: {:?}", + value.ics_type() + ); + Err(Error::InvalidValue) + } } } } @@ -348,10 +345,10 @@ impl TryFrom for DestinationMode { match value { 0x00 => Ok(DestinationMode::Physical), 0x01 => Ok(DestinationMode::Logical), - _ => Err(Error::InvalidValue(format!( - "Invalid destination mode: 0x{:x}", - value - ))), + _ => { + error!("Invalid destination mode: 0x{:x}", value); + Err(Error::InvalidValue) + } } } } @@ -373,10 +370,10 @@ impl TryFrom for TriggerMode { match value { 0x00 => Ok(TriggerMode::Edge), 0x01 => Ok(TriggerMode::Level), - _ => Err(Error::InvalidValue(format!( - "Invalid trigger mode: 0x{:x}", - value - ))), + _ => { + error!("Invalid trigger mode: 0x{:x}", value); + Err(Error::InvalidValue) + } } } } @@ -398,10 +395,10 @@ impl TryFrom for PinPolarity { match value { 0x00 => Ok(PinPolarity::ActiveHigh), 0x01 => Ok(PinPolarity::ActiveLow), - _ => Err(Error::InvalidValue(format!( - "Invalid pin polarity: 0x{:x}", - value - ))), + _ => { + error!("Invalid pin polarity: 0x{:x}", value); + Err(Error::InvalidValue) + } } } } @@ -453,10 +450,10 @@ impl TryFrom for DeliveryMode { 0b100 => Ok(DeliveryMode::NMI), 0b101 => Ok(DeliveryMode::INIT), 0b111 => Ok(DeliveryMode::ExtINT), - _ => Err(Error::InvalidValue(format!( - "Invalid pin polarity: 0x{:x}", - value - ))), + _ => { + error!("Invalid pin polarity: 0x{:x}", value); + Err(Error::InvalidValue) + } } } } @@ -478,10 +475,10 @@ impl TryFrom for DeliveryStatus { match value { 0x00 => Ok(DeliveryStatus::Idle), 0x01 => Ok(DeliveryStatus::SendPending), - _ => Err(Error::InvalidValue(format!( - "Invalid delivery status: 0x{:x}", - value - ))), + _ => { + error!("Invalid delivery status: 0x{:x}", value); + Err(Error::InvalidValue) + } } } } @@ -548,10 +545,11 @@ impl IoRedTblEntry { if self.trigger_mode == TriggerMode::Level && !self.delivery_mode.valid_for_level_trigger() { - return Err(Error::InvalidValue(format!( + error!( "The delivery mode `0b{:b}` is invalid for level trigger mode", self.delivery_mode as u8 - ))); + ); + return Err(Error::InvalidValue); } // When the physical destination mode is used the address can be only @@ -560,17 +558,19 @@ impl IoRedTblEntry { if self.destination_mode == DestinationMode::Physical && self.destination > 15 { - return Err(Error::InvalidValue(format!( + error!( "Invalid Physical APIC ID destination: 0x{:x}", self.destination - ))); + ); + return Err(Error::InvalidValue); } if self.delivery_mode == DeliveryMode::SMI && self.vector != 0 { - return Err(Error::InvalidValue(format!( + error!( "SMI delivery mode requires an empty vector: 0x{:x}", self.vector - ))); + ); + return Err(Error::InvalidValue); } Ok(()) @@ -643,8 +643,11 @@ impl From for u64 { #[cfg(test)] mod test { + + use log::Level; + extern crate testing_logger; + use super::*; - use alloc::string::ToString; fn get_ioapic(buf: *mut u8) -> Result { let ics = Ics::IoApic { @@ -675,16 +678,22 @@ mod test { #[test] fn ioredtblentry_invalid_trigger_mode() { + testing_logger::setup(); // ExtINT is invalid for level trigger mode. let invalid_for_level = 0x0f000000_00008700; - let err = Error::InvalidValue( - "The delivery mode `0b111` is invalid for level trigger mode" - .to_string(), - ); + let err = Error::InvalidValue; assert_eq!( IoRedTblEntry::try_from(invalid_for_level).unwrap_err(), err ); + testing_logger::validate(|captured_logs| { + assert_eq!(captured_logs.len(), 1); + assert_eq!( + captured_logs[0].body, + "The delivery mode `0b111` is invalid for level trigger mode" + ); + assert_eq!(captured_logs[0].level, Level::Error); + }) } #[test] @@ -700,15 +709,23 @@ mod test { fn ioredtblentry_invalid_dest() { // Destination is a full byte but a physical destination mode // is used. + testing_logger::setup(); let invalid_dest = 0xff000000_0000_0000; - let err = Error::InvalidValue( - "Invalid Physical APIC ID destination: 0xff".to_string(), - ); + let err = Error::InvalidValue; assert_eq!(err, IoRedTblEntry::try_from(invalid_dest).unwrap_err()); + testing_logger::validate(|captured_logs| { + assert_eq!(captured_logs.len(), 1); + assert_eq!( + captured_logs[0].body, + "Invalid Physical APIC ID destination: 0xff" + ); + assert_eq!(captured_logs[0].level, Level::Error); + }) } #[test] fn ioredtblentry_write_ro_bit() { + testing_logger::setup(); let mut buf: [u8; 24] = [ 0x00, 0x00, @@ -739,11 +756,17 @@ mod test { let bits = 0x0f000000_0000_1000; let entry = IoRedTblEntry::try_from(bits).unwrap(); - let err = Error::InvalidValue( - "Read-only IO Redirect Table Entry bits set: 0x1000".to_string(), - ); + let err = Error::InvalidValue; // The delivery status is set, which should be read-only. assert_eq!(err, ioapic.write_ioredtbl(0, entry).unwrap_err()); + testing_logger::validate(|captured_logs| { + assert_eq!(captured_logs.len(), 1); + assert_eq!( + captured_logs[0].body, + "Read-only IO Redirect Table Entry bits set: 0x1000" + ); + assert_eq!(captured_logs[0].level, Level::Error); + }) } #[test] diff --git a/mythril/src/lib.rs b/mythril/src/lib.rs index b8815f4..1bb9a4f 100644 --- a/mythril/src/lib.rs +++ b/mythril/src/lib.rs @@ -1,5 +1,5 @@ #![cfg_attr(not(std), no_std)] -#![feature(llvm_asm)] +#![feature(asm)] #![feature(never_type)] #![feature(const_fn)] #![feature(get_mut_unchecked)] diff --git a/mythril/src/linux.rs b/mythril/src/linux.rs index f7dba14..2055469 100644 --- a/mythril/src/linux.rs +++ b/mythril/src/linux.rs @@ -4,13 +4,45 @@ use crate::virtdev::qemu_fw_cfg::{FwCfgSelector, QemuFwCfgBuilder}; use bitflags::bitflags; use byteorder::{ByteOrder, LittleEndian}; +// These come mostly from https://www.kernel.org/doc/Documentation/x86/boot.txt +mod offsets { + use core::ops::Range; + pub const OLD_CMD_LINE_MAGIC: Range = 0x20..0x22; + pub const SETUP_SECTS: usize = 0x1f1; + pub const OLD_CMD_LINE_OFFSET: Range = 0x22..0x24; + pub const HEADER_MAGIC: Range = 0x202..0x206; + pub const BOOTP_VERSION: Range = 0x206..0x208; + pub const TYPE_OF_LOADER: usize = 0x210; + pub const LOAD_FLAGS: usize = 0x211; + pub const RAMDISK_IMAGE: Range = 0x218..0x21c; + pub const RAMDISK_SIZE: Range = 0x21c..0x220; + pub const HEAP_END_PTR: Range = 0x224..0x226; + pub const CMD_LINE_PTR: Range = 0x228..0x22c; + pub const INITRD_ADDR_MAX: Range = 0x22c..0x230; + pub const XLOAD_FLAGS: Range = 0x236..0x238; +} + +const HEADER_MAGIC_VALUE: u32 = 0x53726448; // "HdrS" +const OLD_CMD_LINE_MAGIC_VALUE: u16 = 0xa33f; +const QEMU_LOADER: u8 = 0xb0; + // This blob is taken from QEMU. See: // https://github.com/qemu/qemu/blob/887adde81d1f1f3897f1688d37ec6851b4fdad86/pc-bios/optionrom/linuxboot_dma.c pub const LINUXBOOT_DMA_ROM: &'static [u8] = include_bytes!("blob/linuxboot_dma.bin"); bitflags! { - pub struct XLoadFlags: u32 { + pub struct LoadFlags: u8 { + const LOADED_HIGH = 1 << 0; + const KASLR = 1 << 1; + const QUIET = 1 << 5; + const KEEP_SEGMENTS = 1 << 6; + const CAN_USE_HEAP = 1 << 7; + } +} + +bitflags! { + pub struct XLoadFlags: u16 { const KERNEL_64 = 1 << 0; const CAN_BE_LOADED_ABOVE_4G = 1 << 1; const EFI_HANDOVER_32 = 1 << 2; @@ -32,60 +64,53 @@ pub fn load_linux( let mut kernel = info .find_module(kernel_name.as_ref()) .ok_or_else(|| { - Error::InvalidValue(format!( - "No such kernel '{}'", - kernel_name.as_ref() - )) + error!("No such kernel '{}'", kernel_name.as_ref()); + Error::InvalidValue })? .data() .to_vec(); let initramfs = info .find_module(initramfs_name.as_ref()) .ok_or_else(|| { - Error::InvalidValue(format!( - "No such initramfs '{}'", - initramfs_name.as_ref() - )) + error!("No such initramfs '{}'", initramfs_name.as_ref()); + Error::InvalidValue })? .data(); if kernel.len() < 8192 { - return Err(Error::InvalidValue(format!( - "Kernel image is too small ({} < 8192)", - kernel.len() - ))); + error!("Kernel image is too small ({} < 8192)", kernel.len()); + return Err(Error::InvalidValue); } - let magic = LittleEndian::read_u32(&kernel[0x202..0x202 + 4]); + let magic = LittleEndian::read_u32(&kernel[offsets::HEADER_MAGIC]); // HdrS - if magic != 0x53726448 { - return Err(Error::InvalidValue(format!( - "Invalid kernel image (bad magic = 0x{:x})", - magic - ))); + if magic != HEADER_MAGIC_VALUE { + error!("Invalid kernel image (bad magic = 0x{:x})", magic); + return Err(Error::InvalidValue); } - let protocol = LittleEndian::read_u16(&kernel[0x206..0x206 + 2]); - let (real_addr, cmdline_addr, prot_addr) = - if protocol < 0x200 || (kernel[0x211] & 0x01) == 0 { - (0x90000, 0x9a000 - cmdline.len() as i32, 0x10000) - } else if protocol < 0x202 { - (0x90000, 0x9a000 - cmdline.len() as i32, 0x100000) - } else { - (0x10000, 0x20000, 0x100000) - }; + let protocol = LittleEndian::read_u16(&kernel[offsets::BOOTP_VERSION]); + let (real_addr, cmdline_addr, prot_addr) = if protocol < 0x200 + || (kernel[offsets::LOAD_FLAGS] & LoadFlags::LOADED_HIGH.bits()) == 0 + { + (0x90000, 0x9a000 - cmdline.len() as i32, 0x10000) + } else if protocol < 0x202 { + (0x90000, 0x9a000 - cmdline.len() as i32, 0x100000) + } else { + (0x10000, 0x20000, 0x100000) + }; info!("Protocol = 0x{:x}", protocol); let mut initrd_max = if protocol >= 0x20c - && (LittleEndian::read_u32(&kernel[0x236..0x236 + 4]) + && (LittleEndian::read_u16(&kernel[offsets::XLOAD_FLAGS]) & XLoadFlags::CAN_BE_LOADED_ABOVE_4G.bits()) != 0 { 0xffffffff } else if protocol >= 0x203 { - LittleEndian::read_u32(&kernel[0x22c..0x22c + 4]) + LittleEndian::read_u32(&kernel[offsets::INITRD_ADDR_MAX]) } else { 0x37ffffff }; @@ -101,11 +126,17 @@ pub fn load_linux( builder.add_i32(FwCfgSelector::CMDLINE_SIZE, cmdline.len() as i32); if protocol >= 0x202 { - LittleEndian::write_i32(&mut kernel[0x228..0x228 + 4], cmdline_addr); + LittleEndian::write_i32( + &mut kernel[offsets::CMD_LINE_PTR], + cmdline_addr, + ); } else { - LittleEndian::write_u16(&mut kernel[0x20..0x20 + 2], 0xa33f); + LittleEndian::write_u16( + &mut kernel[offsets::OLD_CMD_LINE_MAGIC], + OLD_CMD_LINE_MAGIC_VALUE, + ); LittleEndian::write_i16( - &mut kernel[0x22..0x22 + 2], + &mut kernel[offsets::OLD_CMD_LINE_OFFSET], (cmdline_addr - real_addr) as i16, ); } @@ -115,43 +146,43 @@ pub fn load_linux( // loader type // TODO: change this from QEMU probably if protocol >= 0x200 { - kernel[0x210] = 0xB0; + kernel[offsets::TYPE_OF_LOADER] = QEMU_LOADER; } // Heap if protocol >= 0x201 { - kernel[0x211] |= 0x80; + kernel[offsets::LOAD_FLAGS] |= LoadFlags::CAN_USE_HEAP.bits(); LittleEndian::write_i16( - &mut kernel[0x224..0x224 + 2], + &mut kernel[offsets::HEAP_END_PTR], (cmdline_addr - real_addr - 0x200) as i16, ); } if protocol < 0x200 { - return Err(Error::InvalidValue( - "Kernel too old for initrd support".into(), - )); + error!("Kernel too old for initrd support"); + return Err(Error::InvalidValue); } if initramfs.len() as u32 > initrd_max { - return Err(Error::InvalidValue(format!( + error!( "Initramfs too large (0x{:x} bytes > max of 0x{:x})", initramfs.len(), initrd_max - ))); + ); + return Err(Error::InvalidValue); } let initrd_addr = ((initrd_max - initramfs.len() as u32) & !4095) as i32; builder.add_i32(FwCfgSelector::INITRD_ADDR, initrd_addr); builder.add_i32(FwCfgSelector::INITRD_SIZE, initramfs.len() as i32); builder.add_bytes(FwCfgSelector::INITRD_DATA, initramfs); - LittleEndian::write_i32(&mut kernel[0x218..0x218 + 4], initrd_addr); + LittleEndian::write_i32(&mut kernel[offsets::RAMDISK_IMAGE], initrd_addr); LittleEndian::write_i32( - &mut kernel[0x21c..0x21c + 4], + &mut kernel[offsets::RAMDISK_SIZE], initramfs.len() as i32, ); - let setup_size = match kernel[0x1f1] { + let setup_size = match kernel[offsets::SETUP_SECTS] { // For legacy compat, setup size 0 is really 4 sectors 0 => 4 + 1, size => size + 1, @@ -159,9 +190,8 @@ pub fn load_linux( * 512; if setup_size as usize > kernel.len() { - return Err(Error::InvalidValue( - "Invalid kernel header (setup size > kernel size)".into(), - )); + error!("Invalid kernel header (setup size > kernel size)"); + return Err(Error::InvalidValue); } let kernel_size = kernel.len() as i32 - setup_size; diff --git a/mythril/src/lock/mod.rs b/mythril/src/lock/mod.rs index f1596ea..b2ebe28 100644 --- a/mythril/src/lock/mod.rs +++ b/mythril/src/lock/mod.rs @@ -4,6 +4,6 @@ pub mod ro_after_init; #[inline(always)] pub fn relax_cpu() { unsafe { - llvm_asm!("rep; nop" ::: "memory"); + asm!("rep", "nop"); } } diff --git a/mythril/src/logger.rs b/mythril/src/logger.rs index 30d7f47..ec3323a 100644 --- a/mythril/src/logger.rs +++ b/mythril/src/logger.rs @@ -83,11 +83,14 @@ pub unsafe fn raw_write_console(s: impl AsRef) { let len = s.as_ref().len(); let ptr = s.as_ref().as_ptr(); - llvm_asm!("cld; rep outsb" - : - :"{rdx}"(0x3f8), "{rcx}"(len as u64), "{rsi}"(ptr as u64) - : "rflags", "rsi" - : "volatile"); + asm!( + "cld", + "rep outsb", + in("rdx") 0x3f8, + in("rcx") len as u64, + inout("rsi") ptr as u64 => _, + options(nostack) + ); } pub struct VgaWriter { diff --git a/mythril/src/memory.rs b/mythril/src/memory.rs index f178ae3..13c12c4 100644 --- a/mythril/src/memory.rs +++ b/mythril/src/memory.rs @@ -231,9 +231,8 @@ impl HostPhysFrame { pub fn from_start_address(addr: HostPhysAddr) -> Result { if !addr.is_frame_aligned() { - Err(Error::InvalidValue( - "Invalid start address for HostPhysFrame".into(), - )) + error!("Invalid start address for HostPhysFrame"); + Err(Error::InvalidValue) } else { Ok(HostPhysFrame(addr)) } @@ -367,31 +366,27 @@ impl GuestAddressSpace { ) -> Result { let ept_pml4e = &self.root.read()[addr.p4_index()]; if ept_pml4e.is_unused() { - return Err(Error::InvalidValue( - "No PML4 entry for GuestPhysAddr".into(), - )); + error!("No PML4 entry for GuestPhysAddr"); + return Err(Error::InvalidValue); } let ept_pdpt = ept_pml4e.addr().as_u64() as *const EptPageDirectoryPointerTable; let ept_pdpe = unsafe { &(*ept_pdpt)[addr.p3_index()] }; if ept_pdpe.is_unused() { - return Err(Error::InvalidValue( - "No PDP entry for GuestPhysAddr".into(), - )); + error!("No PDP entry for GuestPhysAddr"); + return Err(Error::InvalidValue); } let ept_pdt = ept_pdpe.addr().as_u64() as *const EptPageDirectory; let ept_pde = unsafe { &(*ept_pdt)[addr.p2_index()] }; if ept_pde.is_unused() { - return Err(Error::InvalidValue( - "No PD entry for GuestPhysAddr".into(), - )); + error!("No PD entry for GuestPhysAddr"); + return Err(Error::InvalidValue); } let ept_pt = ept_pde.addr().as_u64() as *const EptPageTable; let ept_pte = unsafe { &(*ept_pt)[addr.p1_index()] }; if ept_pte.is_unused() { - return Err(Error::InvalidValue( - "No PT entry for GuestPhysAddr".into(), - )); + error!("No PT entry for GuestPhysAddr"); + return Err(Error::InvalidValue); } HostPhysFrame::from_start_address(ept_pte.addr()) } @@ -753,10 +748,8 @@ fn map_guest_memory( let ept_pte = unsafe { &mut (*ept_pt)[guest_addr.p1_index()] }; if !ept_pte.is_unused() { - return Err(Error::DuplicateMapping(format!( - "Duplicate mapping for address 0x{:x}", - guest_addr.as_u64() - ))); + error!("Duplicate mapping for address 0x{:x}", guest_addr.as_u64()); + return Err(Error::DuplicateMapping); } let mut page_flags = EptTableFlags::READ_ACCESS diff --git a/mythril/src/physdev/keyboard.rs b/mythril/src/physdev/keyboard.rs index 1369563..68e94d5 100644 --- a/mythril/src/physdev/keyboard.rs +++ b/mythril/src/physdev/keyboard.rs @@ -83,9 +83,8 @@ impl Ps2Controller { controller.write_command_port(Command::TestController); if controller.read_data_port() != 0x55 { - return Err(Error::DeviceError( - "Failed to init Ps2Controller".into(), - )); + error!("Failed to init Ps2Controller"); + return Err(Error::DeviceError); } controller.write_command_port(Command::EnableFirst); @@ -95,14 +94,12 @@ impl Ps2Controller { controller.write_data_port(0xff); if controller.read_data_port() != 0xFA { - return Err(Error::DeviceError( - "Failed to init Ps2Controller".into(), - )); + error!("Failed to init Ps2Controller"); + return Err(Error::DeviceError); } if controller.read_data_port() != 0xAA { - return Err(Error::DeviceError( - "Failed to init Ps2Controller".into(), - )); + error!("Failed to init Ps2Controller"); + return Err(Error::DeviceError); } controller.flush_read("defaults"); diff --git a/mythril/src/physdev/pit.rs b/mythril/src/physdev/pit.rs index c3db39b..6e1ee7a 100644 --- a/mythril/src/physdev/pit.rs +++ b/mythril/src/physdev/pit.rs @@ -35,11 +35,16 @@ pub enum AccessMode { #[derive(Clone, Copy, Debug)] #[repr(u8)] pub enum OperatingMode { - Mode0 = 0b000, // interrupt on terminal count - Mode1 = 0b001, // hardware re-triggerable one-shot - Mode2 = 0b010, // rate generator - Mode3 = 0b011, // square wave generator - Mode4 = 0b100, // software triggered strobe + Mode0 = 0b000, + // interrupt on terminal count + Mode1 = 0b001, + // hardware re-triggerable one-shot + Mode2 = 0b010, + // rate generator + Mode3 = 0b011, + // square wave generator + Mode4 = 0b100, + // software triggered strobe Mode5 = 0b101, // hardware triggered strobe } @@ -55,10 +60,10 @@ impl TryFrom for OperatingMode { 0b101 => Ok(OperatingMode::Mode5), 0b110 => Ok(OperatingMode::Mode2), 0b111 => Ok(OperatingMode::Mode3), - _ => Err(Error::InvalidValue(format!( - "Invalid PIT operating mode: {}", - val - ))), + _ => { + error!("Invalid PIT operating mode: {}", val); + Err(Error::InvalidValue) + } } } } diff --git a/mythril/src/registers.rs b/mythril/src/registers.rs index 6d6a757..c002cde 100644 --- a/mythril/src/registers.rs +++ b/mythril/src/registers.rs @@ -13,11 +13,11 @@ impl IdtrBase { limit: 0, base_addr: 0, }; - llvm_asm!("sidt ($0)" - : - : "r"(&mut info) - : "memory" - : "volatile"); + asm!( + "sidt fword ptr [{0}]", + in(reg) &mut info, + options(nostack) + ); info.base_addr } } @@ -35,11 +35,11 @@ impl GdtrBase { pub fn read() -> u64 { unsafe { let mut info = GdtInfo { size: 0, offset: 0 }; - llvm_asm!("sgdtq ($0)" - : - : "r"(&mut info) - : "memory" - : "volatile"); + asm!( + "sgdt fword ptr [{0}]", + in(reg) &mut info, + options(nostack) + ); info.offset } } diff --git a/mythril/src/tsc.rs b/mythril/src/tsc.rs index d8ad6c1..d6d9c52 100644 --- a/mythril/src/tsc.rs +++ b/mythril/src/tsc.rs @@ -33,7 +33,8 @@ static TSC: RoAfterInit = RoAfterInit::uninitialized(); pub unsafe fn calibrate_tsc() -> Result<&'static dyn TimeSource> { if RoAfterInit::is_initialized(&TSC) { - return Err(Error::InvalidValue("TSC is already calibrated".into())); + error!("TSC is already calibrated"); + return Err(Error::InvalidValue); } let orig: u8 = inb(pit::PIT_PS2_CTRL_B); diff --git a/mythril/src/vcpu.rs b/mythril/src/vcpu.rs index cc89816..090a047 100644 --- a/mythril/src/vcpu.rs +++ b/mythril/src/vcpu.rs @@ -171,7 +171,7 @@ impl VCpu { /// Begin execution in the guest context for this core pub fn launch(&mut self) -> Result { let rflags = unsafe { vmlaunch_wrapper() }; - error::check_vm_insruction(rflags, "Failed to launch vm".into())?; + error::check_vm_instruction(rflags, || error!("Failed to launch vm"))?; unreachable!() } @@ -511,7 +511,8 @@ impl VCpu { .read_field(vmcs::VmcsField::GuestInterruptibilityInfo)?, ) .ok_or_else(|| { - Error::InvalidValue("Invalid interruptibility state".into()) + error!("Invalid interruptibility state"); + Error::InvalidValue })?; let rflags = self.vmcs.read_field(vmcs::VmcsField::GuestRflags)?; @@ -725,9 +726,8 @@ impl VCpu { next_bsp.raw as u8, ) .map_err(|_| { - Error::DeviceError( - "Failed to update console GSI mapping".into(), - ) + error!("Failed to update console GSI mapping"); + Error::DeviceError })?; } virtdev::DeviceEventResponse::GuestUartTransmitted(val) => { diff --git a/mythril/src/virtdev/lapic.rs b/mythril/src/virtdev/lapic.rs index d467d7f..cb46de5 100644 --- a/mythril/src/virtdev/lapic.rs +++ b/mythril/src/virtdev/lapic.rs @@ -68,10 +68,8 @@ impl TryFrom for ApicRegisterOffset { fn try_from(value: u16) -> Result { if value & 0b1111 != 0 { - return Err(Error::InvalidValue(format!( - "APIC register offset not aligned: 0x{:x}", - value - ))); + error!("APIC register offset not aligned: 0x{:x}", value); + return Err(Error::InvalidValue); } if let Ok(simple_reg) = ApicRegisterSimpleOffset::try_from(value) { @@ -92,10 +90,8 @@ impl TryFrom for ApicRegisterOffset { ApicRegisterOffset::InterruptCommand((value - 0x300) >> 4) } offset => { - return Err(Error::InvalidValue(format!( - "Invalid APIC register offset: 0x{:x}", - offset - ))) + error!("Invalid APIC register offset: 0x{:x}", offset); + return Err(Error::InvalidValue); } }; diff --git a/mythril/src/virtdev/mod.rs b/mythril/src/virtdev/mod.rs index 6ad7470..0906f45 100644 --- a/mythril/src/virtdev/mod.rs +++ b/mythril/src/virtdev/mod.rs @@ -23,6 +23,7 @@ pub mod rtc; pub mod vga; const MAX_EVENT_RESPONSES: usize = 8; + pub type ResponseEventArray = ArrayVec<[DeviceEventResponse; MAX_EVENT_RESPONSES]>; pub type Port = u16; @@ -195,10 +196,9 @@ impl<'a> DeviceMap<'a> { .expect("Could not get conflicting device") .0; - return Err(Error::InvalidDevice(format!( - "I/O Port already registered: 0x{:x}-0x{:x} conflicts with existing map of 0x{:x}-0x{:x}", - key.0.start(), key.0.end(), conflict.0.start(), conflict.0.end() - ))); + error!("I/O Port already registered: 0x{:x}-0x{:x} conflicts with existing map of 0x{:x}-0x{:x}", + key.0.start(), key.0.end(), conflict.0.start(), conflict.0.end()); + return Err(Error::InvalidDevice); } self.portio_map.insert(key, dev); } @@ -210,10 +210,9 @@ impl<'a> DeviceMap<'a> { .get_key_value(&key) .expect("Could not get conflicting device") .0; - return Err(Error::InvalidDevice(format!( - "Memory region already registered: 0x{:x}-0x{:x} conflicts with existing map of 0x{:x}-0x{:x}", - key.0.start().as_u64(), key.0.end().as_u64(), conflict.0.start().as_u64(), conflict.0.end().as_u64() - ))); + error!("Memory region already registered: 0x{:x}-0x{:x} conflicts with existing map of 0x{:x}-0x{:x}", + key.0.start().as_u64(), key.0.end().as_u64(), conflict.0.start().as_u64(), conflict.0.end().as_u64()); + return Err(Error::InvalidDevice); } self.memio_map.insert(key, dev.clone()); } @@ -288,10 +287,8 @@ impl<'a> TryFrom<&'a mut [u8]> for PortReadRequest<'a> { &mut *(buff.as_mut_ptr() as *mut [u8; 4]) }), len => { - return Err(Error::InvalidValue(format!( - "Invalid slice length: {}", - len - ))) + error!("Invalid slice length: {}", len); + return Err(Error::InvalidValue); } }; Ok(res) @@ -346,10 +343,8 @@ impl<'a> TryFrom<&'a [u8]> for PortWriteRequest<'a> { Self::FourBytes(unsafe { &*(buff.as_ptr() as *const [u8; 4]) }) } len => { - return Err(Error::InvalidValue(format!( - "Invalid slice length: {}", - len - ))) + error!("Invalid slice length: {}", len); + return Err(Error::InvalidValue); } }; Ok(res) @@ -362,10 +357,10 @@ impl<'a> TryFrom> for u8 { fn try_from(value: PortWriteRequest<'a>) -> Result { match value { PortWriteRequest::OneByte(val) => Ok(val[0]), - val => Err(Error::InvalidValue(format!( - "Value {} cannot be converted to u8", - val - ))), + val => { + error!("Value {} cannot be converted to u8", val); + Err(Error::InvalidValue) + } } } } @@ -376,10 +371,10 @@ impl<'a> TryFrom> for u16 { fn try_from(value: PortWriteRequest<'a>) -> Result { match value { PortWriteRequest::TwoBytes(val) => Ok(u16::from_be_bytes(*val)), - val => Err(Error::InvalidValue(format!( - "Value {} cannot be converted to u16", - val - ))), + val => { + error!("Value {} cannot be converted to u16", val); + Err(Error::InvalidValue) + } } } } @@ -390,10 +385,10 @@ impl<'a> TryFrom> for u32 { fn try_from(value: PortWriteRequest<'a>) -> Result { match value { PortWriteRequest::FourBytes(val) => Ok(u32::from_be_bytes(*val)), - val => Err(Error::InvalidValue(format!( - "Value {} cannot be converted to u32", - val - ))), + val => { + error!("Value {} cannot be converted to u32", val); + Err(Error::InvalidValue) + } } } } @@ -451,10 +446,8 @@ impl<'a> TryInto for MemWriteRequest<'a> { if self.data.len() == 1 { Ok(self.data[0]) } else { - Err(Error::InvalidValue(format!( - "Value {} cannot be converted to u8", - self - ))) + error!("Value {} cannot be converted to u8", self); + Err(Error::InvalidValue) } } } diff --git a/mythril/src/virtdev/pci.rs b/mythril/src/virtdev/pci.rs index e3c4a81..11bf581 100644 --- a/mythril/src/virtdev/pci.rs +++ b/mythril/src/virtdev/pci.rs @@ -233,10 +233,8 @@ impl EmulatedDevice for PciRootComplex { } } _ => { - return Err(Error::InvalidValue(format!( - "Invalid PCI port read 0x{:x}", - port - ))) + error!("Invalid PCI port read 0x{:x}", port); + return Err(Error::InvalidValue); } } } diff --git a/mythril/src/virtdev/pit.rs b/mythril/src/virtdev/pit.rs index f6ea962..8aff77d 100644 --- a/mythril/src/virtdev/pit.rs +++ b/mythril/src/virtdev/pit.rs @@ -111,9 +111,8 @@ impl Pit8254 { let operating = (0b00001110 & val) >> 1; if val & 0b00000001 != 0 { - return Err(Error::InvalidValue( - "PIT BCD mode is not supported".into(), - )); + error!("PIT BCD mode is not supported"); + return Err(Error::InvalidValue); } let operating_state = match operating { @@ -128,10 +127,8 @@ impl Pit8254 { start_time: None, }, value => { - return Err(Error::InvalidValue(format!( - "Invalid PIT operating state '0x{:x}'", - value - ))) + error!("Invalid PIT operating state '0x{:x}'", value); + return Err(Error::InvalidValue); } }; @@ -141,10 +138,8 @@ impl Pit8254 { 0b10 => AccessModeState::HiByte, 0b11 => AccessModeState::Word { lo_byte: None }, value => { - return Err(Error::InvalidValue(format!( - "Invalid PIT access state '0x{:x}'", - value - ))) + error!("Invalid PIT access state '0x{:x}'", value); + return Err(Error::InvalidValue); } }; @@ -157,10 +152,8 @@ impl Pit8254 { 0b00 => &mut self.channel0, 0b10 => &mut self.channel2, value => { - return Err(Error::InvalidValue(format!( - "Invalid PIT channel '0x{:x}'", - value - ))) + error!("Invalid PIT channel '0x{:x}'", value); + return Err(Error::InvalidValue); } }; @@ -179,10 +172,8 @@ impl Pit8254 { let channel_state = match port { PIT_COUNTER_0 => &mut self.channel0, PIT_COUNTER_1 => { - return Err(Error::InvalidValue(format!( - "Invalid PIT port '0x{:x}'", - port - ))) + error!("Invalid PIT port '0x{:x}'", port); + return Err(Error::InvalidValue); } PIT_COUNTER_2 => &mut self.channel2, _ => unreachable!(), diff --git a/mythril/src/virtdev/qemu_fw_cfg.rs b/mythril/src/virtdev/qemu_fw_cfg.rs index 37ddcad..f2f1d08 100644 --- a/mythril/src/virtdev/qemu_fw_cfg.rs +++ b/mythril/src/virtdev/qemu_fw_cfg.rs @@ -191,16 +191,13 @@ impl QemuFwCfgBuilder { data: &[u8], ) -> Result<()> { if name.as_ref().len() > FW_CFG_MAX_FILE_NAME { - return Err(Error::InvalidValue(format!( - "qemu_fw_cfg: file name too long: {}", - name.as_ref() - ))); + error!("qemu_fw_cfg: file name too long: {}", name.as_ref()); + return Err(Error::InvalidValue); } let selector = self.next_file_selector(); if selector > FwCfgSelector::FILE_LAST { - return Err(Error::InvalidValue( - "qemu_fw_cfg: too many files".into(), - )); + error!("qemu_fw_cfg: too many files"); + return Err(Error::InvalidValue); } let name = name.as_ref().as_bytes(); @@ -370,9 +367,8 @@ impl QemuFwCfg { self.data_idx = 0; } Self::FW_CFG_PORT_DATA => { - return Err(Error::NotImplemented( - "Write to QEMU FW CFG data port not yet supported".into(), - )) + error!("Write to QEMU FW CFG data port not yet supported"); + return Err(Error::NotImplemented); } Self::FW_CFG_PORT_DMA_LOW => { let low = u32::from_be(val.try_into()?); diff --git a/mythril/src/virtdev/vga.rs b/mythril/src/virtdev/vga.rs index 0964d64..cdc077e 100644 --- a/mythril/src/virtdev/vga.rs +++ b/mythril/src/virtdev/vga.rs @@ -75,10 +75,11 @@ impl VgaController { val.copy_from_u32(self.registers[self.index as usize] as u32); } _ => { - return Err(Error::NotImplemented(format!( + error!( "Unsupported attempt to read from vga port 0x{:x}", port - ))) + ); + return Err(Error::NotImplemented); } } Ok(()) @@ -105,20 +106,19 @@ impl VgaController { self.registers[self.index as usize] = data; } _ => { - return Err(Error::InvalidValue(format!( + error!( "Invalid port write to VGA index register: {:?}", val - ))) + ); + return Err(Error::InvalidValue); } }, Self::VGA_DATA => { self.registers[self.index as usize] = val.try_into()?; } _ => { - return Err(Error::NotImplemented(format!( - "Unsupported attempt to write to vga port 0x{:x}", - port - ))) + error!("Unsupported attempt to write to vga port 0x{:x}", port); + return Err(Error::NotImplemented); } } Ok(()) diff --git a/mythril/src/vm.rs b/mythril/src/vm.rs index 1cec159..22d4b2f 100644 --- a/mythril/src/vm.rs +++ b/mythril/src/vm.rs @@ -226,10 +226,8 @@ impl VirtualMachineSet { .context_by_core_id(core_id) .ok_or_else(|| Error::NotFound)?; context.msgqueue.write().push_back(msg).map_err(|_| { - Error::InvalidValue(format!( - "RX queue is full for core_id = {}", - core_id - )) + error!("RX queue is full for core_id = {}", core_id); + Error::InvalidValue })?; if !notify { @@ -266,10 +264,8 @@ impl VirtualMachineSet { notify: bool, ) -> Result<()> { let vm_bsp = self.bsp_core_id(vm_id).ok_or_else(|| { - Error::InvalidValue(format!( - "Unable to find BSP for VM id '{}'", - vm_id - )) + error!("Unable to find BSP for VM id '{}'", vm_id); + Error::InvalidValue })?; self.send_msg_core(msg, vm_bsp, notify) } @@ -552,9 +548,10 @@ impl VirtualMachine { // Just ignore writes to unknown ports Ok(()) } - _ => Err(Error::MissingDevice( - "Unable to dispatch event".into(), - )), + _ => { + error!("Unable to dispatch event"); + Err(Error::MissingDevice) + } }; } }; @@ -674,7 +671,8 @@ impl VirtualMachine { let data = info .find_module(image) .ok_or_else(|| { - Error::InvalidValue(format!("No such module '{}'", image)) + error!("No such module '{}'", image); + Error::InvalidValue })? .data(); Self::map_data(data, addr, space) @@ -716,7 +714,7 @@ impl VirtualMachine { ), false, ) { - Ok(_) | Err(Error::DuplicateMapping(_)) => continue, + Ok(_) | Err(Error::DuplicateMapping) => continue, Err(e) => return Err(e), } } diff --git a/mythril/src/vmcs.rs b/mythril/src/vmcs.rs index e0db02c..d0592ed 100644 --- a/mythril/src/vmcs.rs +++ b/mythril/src/vmcs.rs @@ -309,14 +309,13 @@ fn vmcs_write_with_fixed( required_value |= low; /* bit == 1 in low word ==> must be one */ if (value & !required_value) != 0 { - return Err(Error::Vmcs(format!( - "Requested field ({:?}) bit not allowed by MSR (requested=0x{:x} forbidden=0x{:x} required=0x{:x} res=0x{:x})", - field, - value, - high, - low, - required_value - ))); + error!("Requested field ({:?}) bit not allowed by MSR (requested=0x{:x} forbidden=0x{:x} required=0x{:x} res=0x{:x})", + field, + value, + high, + low, + required_value); + return Err(Error::Vmcs); } vmcs_write(field, required_value)?; @@ -326,28 +325,30 @@ fn vmcs_write_with_fixed( fn vmcs_write(field: VmcsField, value: u64) -> Result<()> { let rflags = unsafe { let rflags: u64; - llvm_asm!("vmwrite %rdx, %rax; pushfq; popq $0" - : "=r"(rflags) - : "{rdx}"(value), "{rax}"(field as u64) - : "rflags" - : "volatile"); + asm!( + "vmwrite rax, rdx", + "pushf", + "pop {}", + lateout(reg) rflags, + in("rdx") value, + in("rax") field as u64, + ); rflags }; - error::check_vm_insruction( - rflags, - format!("Failed to write 0x{:x} to field {:?}", value, field), - ) + error::check_vm_instruction(rflags, || { + error!("Failed to write 0x{:x} to field {:?}", value, field) + }) } fn vmcs_read(field: VmcsField) -> Result { let value = unsafe { let value: u64; - llvm_asm!("vmreadq %rdx, %rax" - : "={rax}"(value) - : "{rdx}"(field as u64) - : "rflags" - : "volatile"); + asm!( + "vmread rax, rdx", + out("rax") value, + in("rdx") field as u64 + ); value }; @@ -363,27 +364,32 @@ fn vmcs_activate(vmcs: &mut Vmcs, _vmx: &vmx::Vmx) -> Result<()> { } let rflags = unsafe { let rflags: u64; - llvm_asm!("vmptrld $1; pushfq; popq $0" - : "=r"(rflags) - : "m"(vmcs_region_addr) - : "rflags"); + asm!( + "vmptrld [{}]", + "pushf", + "pop {}", + in(reg) &vmcs_region_addr, + lateout(reg) rflags + ); rflags }; - error::check_vm_insruction(rflags, "Failed to activate VMCS".into()) + error::check_vm_instruction(rflags, || error!("Failed to activate VMCS")) } fn vmcs_clear(vmcs_page: &mut Raw4kPage) -> Result<()> { let rflags = unsafe { let rflags: u64; - llvm_asm!("vmclear $1; pushfq; popq $0" - : "=r"(rflags) - : "m"(vmcs_page as *const _ as u64) - : "rflags" - : "volatile"); + asm!( + "vmclear [{}]", + "pushf", + "pop {}", + in(reg) &(vmcs_page as *const _ as u64), + lateout(reg) rflags + ); rflags }; - error::check_vm_insruction(rflags, "Failed to clear VMCS".into()) + error::check_vm_instruction(rflags, || error!("Failed to clear VMCS")) } pub struct Vmcs { diff --git a/mythril/src/vmexit.rs b/mythril/src/vmexit.rs index 32e5bd5..52e0365 100644 --- a/mythril/src/vmexit.rs +++ b/mythril/src/vmexit.rs @@ -54,7 +54,7 @@ pub extern "C" fn vmexit_handler(state: *mut GuestCpuState) { #[no_mangle] pub extern "C" fn vmresume_failure_handler(rflags: u64) { - error::check_vm_insruction(rflags, "Failed to vmresume".into()) + error::check_vm_instruction(rflags, || error!("Failed to vmresume")) .expect("vmresume failed"); } @@ -221,10 +221,8 @@ impl ExitReason { 63 => ExitInformation::Xsaves, 64 => ExitInformation::Xrstors, reason => { - return Err(Error::InvalidValue(format!( - "Unexpected basic vmexit reason: {}", - reason - ))) + error!("Unexpected basic vmexit reason: {}", reason); + return Err(Error::InvalidValue); } }; Ok(ExitReason { diff --git a/mythril/src/vmx.rs b/mythril/src/vmx.rs index aae45e3..e689143 100644 --- a/mythril/src/vmx.rs +++ b/mythril/src/vmx.rs @@ -20,16 +20,24 @@ impl Vmx { unsafe { // Enable NE in CR0, This is fixed bit in VMX CR0 - llvm_asm!("movq %cr0, %rax; orq %rdx, %rax; movq %rax, %cr0;" - : - : "{rdx}"(0x20) - : "rax"); + asm!( + "mov rax, cr0", + "or rax, rdx", + "mov cr0, rax", + in("rdx") 0x20, + lateout("rax") _, + options(nomem, nostack) + ); // Enable vmx in CR4 - llvm_asm!("movq %cr4, %rax; orq %rdx, %rax; movq %rax, %cr4;" - : - : "{rdx}"(VMX_ENABLE_FLAG) - : "rax"); + asm!( + "mov rax, cr4", + "or rax, rdx", + "mov cr4, rax", + in("rdx") VMX_ENABLE_FLAG, + lateout("rax") _, + options(nomem, nostack) + ); } let revision_id = Self::revision(); @@ -45,14 +53,17 @@ impl Vmx { let rflags = unsafe { let rflags: u64; - llvm_asm!("vmxon $1; pushfq; popq $0" - : "=r"(rflags) - : "m"(vmxon_region_addr) - : "rflags"); + asm!( + "vmxon [{}]", + "pushf", + "pop {}", + in(reg) &vmxon_region_addr, + lateout(reg) rflags, + ); rflags }; - error::check_vm_insruction(rflags, "Failed to enable vmx".into())?; + error::check_vm_instruction(rflags, || error!("Failed to enable vmx"))?; Ok(Vmx { _vmxon_region: vmxon_region, }) @@ -63,14 +74,16 @@ impl Vmx { // was originally activated from let rflags = unsafe { let rflags: u64; - llvm_asm!("vmxoff; pushfq; popq $0" - : "=r"(rflags) - : - : "rflags"); + asm!( + "vmxoff", + "pushf", + "pop {}", + lateout(reg) rflags, + ); rflags }; - error::check_vm_insruction(rflags, "Failed to disable vmx".into()) + error::check_vm_instruction(rflags, || error!("Failed to disable vmx")) } pub fn revision() -> u32 { @@ -85,12 +98,19 @@ impl Vmx { let rflags = unsafe { let rflags: u64; - llvm_asm!("invept $1, $2; pushfq; popq $0" - : "=r"(rflags) - : "m"(val), "r"(t)); + asm!( + "invept {}, [{}]", + "pushfq", + "pop {}", + in(reg) t, + in(reg) &val, + lateout(reg) rflags + ); rflags }; - error::check_vm_insruction(rflags, "Failed to execute invept".into()) + error::check_vm_instruction(rflags, || { + error!("Failed to execute invept") + }) } pub fn invvpid(&self, mode: InvVpidMode) -> Result<()> { @@ -107,12 +127,19 @@ impl Vmx { let rflags = unsafe { let rflags: u64; - llvm_asm!("invvpid $1, $2; pushfq; popq $0" - : "=r"(rflags) - : "m"(val), "r"(t)); + asm!( + "invvpid {}, [{}]", + "pushfq", + "pop {}", + in(reg) t, + in(reg) &val, + lateout(reg) rflags + ); rflags }; - error::check_vm_insruction(rflags, "Failed to execute invvpid".into()) + error::check_vm_instruction(rflags, || { + error!("Failed to execute invvpid") + }) } }