From d5efca74a4ba9a9c52b69b0e250441150ecc1bc2 Mon Sep 17 00:00:00 2001 From: Mael Guillossou Date: Thu, 14 Jan 2021 12:23:38 +0100 Subject: [PATCH 01/24] Use new asm macro in src/interrupt/ --- mythril/src/interrupt/idt.rs | 71 +++++++++++++++++------------------- mythril/src/interrupt/mod.rs | 4 +- mythril/src/lib.rs | 1 + 3 files changed, 37 insertions(+), 39 deletions(-) diff --git a/mythril/src/interrupt/idt.rs b/mythril/src/interrupt/idt.rs index 1e25a6c..4059192 100644 --- a/mythril/src/interrupt/idt.rs +++ b/mythril/src/interrupt/idt.rs @@ -79,42 +79,40 @@ 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" + () => (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" + () => (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", )); } @@ -129,7 +127,7 @@ macro_rules! interrupt_fn_impl { push_regs!(); let rbp: usize; - llvm_asm!("" : "={rbp}"(rbp) : : : "volatile"); + asm!("mov rax, rbp", out("eax") rbp); // Plus usize to skip the old rpb value pushed in the preamble let stack = &*( (rbp + core::mem::size_of::()) as *const $type); @@ -139,10 +137,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 c83e1f3..199e73e 100644 --- a/mythril/src/interrupt/mod.rs +++ b/mythril/src/interrupt/mod.rs @@ -5,9 +5,9 @@ pub const TIMER_VECTOR: u8 = 48; pub const IPC_VECTOR: u8 = 49; pub unsafe fn enable_interrupts() { - llvm_asm!("sti" :::: "volatile"); + asm!("sti"); } pub unsafe fn disable_interrupts() { - llvm_asm!("cli" :::: "volatile"); + asm!("cli"); } diff --git a/mythril/src/lib.rs b/mythril/src/lib.rs index 43fbe01..1187274 100644 --- a/mythril/src/lib.rs +++ b/mythril/src/lib.rs @@ -1,5 +1,6 @@ #![cfg_attr(not(std), no_std)] #![feature(llvm_asm)] +#![feature(asm)] #![feature(never_type)] #![feature(const_fn)] #![feature(get_mut_unchecked)] From dc5313530edde29230784b71cbb23f319755cee6 Mon Sep 17 00:00:00 2001 From: Mael Guillossou Date: Thu, 14 Jan 2021 12:31:19 +0100 Subject: [PATCH 02/24] Use asm macro in error.rs --- mythril/src/error.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/mythril/src/error.rs b/mythril/src/error.rs index 873d80c..4d1a780 100644 --- a/mythril/src/error.rs +++ b/mythril/src/error.rs @@ -52,11 +52,9 @@ pub fn check_vm_insruction(rflags: u64, error: String) -> Result<()> { } 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); value }; let vm_error = VmInstructionError::try_from(errno) @@ -137,7 +135,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"); } } } From 53d250b4a99a2e222ee9f97f580bed964c843de1 Mon Sep 17 00:00:00 2001 From: antoine Date: Fri, 15 Jan 2021 10:15:26 +0100 Subject: [PATCH 03/24] Changes some llvm_asm to asm macro --- mythril/src/registers.rs | 18 ++++++++---------- mythril/src/time.rs | 2 +- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/mythril/src/registers.rs b/mythril/src/registers.rs index 6d6a757..74b381a 100644 --- a/mythril/src/registers.rs +++ b/mythril/src/registers.rs @@ -13,11 +13,10 @@ 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 + ); info.base_addr } } @@ -35,11 +34,10 @@ 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 + ); info.offset } } diff --git a/mythril/src/time.rs b/mythril/src/time.rs index 9c8af19..a2d5054 100644 --- a/mythril/src/time.rs +++ b/mythril/src/time.rs @@ -384,7 +384,7 @@ pub fn busy_wait(duration: core::time::Duration) { while now() < start + duration { unsafe { // Relax the cpu - llvm_asm!("rep; nop" ::: "memory"); + asm!("rep", "nop"); } } } From 3cb0d1fa15cb508892b8f44ae64d8e032e20dc6b Mon Sep 17 00:00:00 2001 From: Paul Cacheux Date: Fri, 15 Jan 2021 10:47:09 +0100 Subject: [PATCH 04/24] Change logger asm output code to new `asm!` macro --- mythril/src/logger.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/mythril/src/logger.rs b/mythril/src/logger.rs index 30d7f47..4217dee 100644 --- a/mythril/src/logger.rs +++ b/mythril/src/logger.rs @@ -83,11 +83,13 @@ 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 => _, + ); } pub struct VgaWriter { From 075cfc7b2343ab84a34cf1a7d8a22f10356fc56e Mon Sep 17 00:00:00 2001 From: Mael Guillossou Date: Fri, 15 Jan 2021 11:19:56 +0100 Subject: [PATCH 05/24] Change asm macro in percore.rs --- mythril/src/percore.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/mythril/src/percore.rs b/mythril/src/percore.rs index 98b9343..9efff66 100644 --- a/mythril/src/percore.rs +++ b/mythril/src/percore.rs @@ -81,9 +81,8 @@ impl fmt::Display for CoreId { pub fn read_core_id() -> CoreId { unsafe { let value: u64; - llvm_asm!("mov [%fs], %rax" - : "={rax}"(value) - ::: "volatile"); + asm!("mov rax, fs", + out("rax") value); ((value >> 3) as u32).into() // Shift away the RPL and TI bits (they will always be 0) } } From 2319303a75272b556900ad665ab1cf28e0a1f670 Mon Sep 17 00:00:00 2001 From: Paul Cacheux Date: Fri, 15 Jan 2021 11:05:07 +0100 Subject: [PATCH 06/24] Change VMX enable method to use new `asm!` macro --- mythril/src/vmx.rs | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/mythril/src/vmx.rs b/mythril/src/vmx.rs index aae45e3..cabc748 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) + ); // 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) + ); } let revision_id = Self::revision(); From 8d3c866987c37cbf16986811893d37bcb3a61a55 Mon Sep 17 00:00:00 2001 From: Paul Cacheux Date: Fri, 15 Jan 2021 11:09:47 +0100 Subject: [PATCH 07/24] Fix formatting in {push,pop}_regs macros --- mythril/src/interrupt/idt.rs | 70 +++++++++++++++++++----------------- 1 file changed, 38 insertions(+), 32 deletions(-) diff --git a/mythril/src/interrupt/idt.rs b/mythril/src/interrupt/idt.rs index 4059192..fb22922 100644 --- a/mythril/src/interrupt/idt.rs +++ b/mythril/src/interrupt/idt.rs @@ -79,41 +79,47 @@ pub struct FaultState { } macro_rules! push_regs { - () => (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", - )); + () => { + #[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 { - () => (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", - )); + () => { + #[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 { From d672ec6f5e18a8d3be80f165a9cfaae72f64b88d Mon Sep 17 00:00:00 2001 From: Paul Cacheux Date: Fri, 15 Jan 2021 12:02:00 +0100 Subject: [PATCH 08/24] Change VMXON and VMXOFF methods to use new `asm!` macro --- mythril/src/vmx.rs | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/mythril/src/vmx.rs b/mythril/src/vmx.rs index cabc748..af0c9ab 100644 --- a/mythril/src/vmx.rs +++ b/mythril/src/vmx.rs @@ -53,10 +53,13 @@ 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 }; @@ -71,10 +74,12 @@ 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 }; From ee5ce751e075c48d03bb49c8a904460b7332afd1 Mon Sep 17 00:00:00 2001 From: Paul Cacheux Date: Thu, 21 Jan 2021 11:06:07 +0100 Subject: [PATCH 09/24] Last ASM macro changes in vmx.rs --- mythril/src/vmx.rs | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/mythril/src/vmx.rs b/mythril/src/vmx.rs index af0c9ab..4748fcb 100644 --- a/mythril/src/vmx.rs +++ b/mythril/src/vmx.rs @@ -98,9 +98,14 @@ 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()) @@ -120,9 +125,14 @@ 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()) From 62cc2fbd2140d127b586bc2030b73e4d98c58b55 Mon Sep 17 00:00:00 2001 From: antoine Date: Thu, 21 Jan 2021 11:35:43 +0100 Subject: [PATCH 10/24] Changes the vmcs file to use asm macro --- mythril/src/vmcs.rs | 46 ++++++++++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/mythril/src/vmcs.rs b/mythril/src/vmcs.rs index a10ebb9..ce96d94 100644 --- a/mythril/src/vmcs.rs +++ b/mythril/src/vmcs.rs @@ -316,11 +316,14 @@ 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 }; @@ -333,11 +336,11 @@ fn vmcs_write(field: VmcsField, value: u64) -> Result<()> { 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 }; @@ -353,10 +356,13 @@ 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 }; @@ -366,11 +372,13 @@ fn vmcs_activate(vmcs: &mut Vmcs, _vmx: &vmx::Vmx) -> Result<()> { 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()) From ec3b7331569c7222567047a9098ee426ba79bf14 Mon Sep 17 00:00:00 2001 From: Paul Cacheux Date: Thu, 21 Jan 2021 14:57:32 +0100 Subject: [PATCH 11/24] Fix asm! macro use in idt.rs --- mythril/src/interrupt/idt.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/mythril/src/interrupt/idt.rs b/mythril/src/interrupt/idt.rs index fb22922..8c2ac9f 100644 --- a/mythril/src/interrupt/idt.rs +++ b/mythril/src/interrupt/idt.rs @@ -133,7 +133,11 @@ macro_rules! interrupt_fn_impl { push_regs!(); let rbp: usize; - asm!("mov rax, rbp", out("eax") rbp); + asm!( + "mov rax, rbp", + out("rax") 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); From 550238470548e43f4e4bfde5d86ff929f40ed79f Mon Sep 17 00:00:00 2001 From: Paul Cacheux Date: Thu, 21 Jan 2021 14:58:45 +0100 Subject: [PATCH 12/24] Remove unused llvm_asm feature --- mythril/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/mythril/src/lib.rs b/mythril/src/lib.rs index 1187274..57362d6 100644 --- a/mythril/src/lib.rs +++ b/mythril/src/lib.rs @@ -1,5 +1,4 @@ #![cfg_attr(not(std), no_std)] -#![feature(llvm_asm)] #![feature(asm)] #![feature(never_type)] #![feature(const_fn)] From ed4c65488ddaf880b3ba772e07a5bbc3245d82a1 Mon Sep 17 00:00:00 2001 From: Paul Cacheux Date: Thu, 21 Jan 2021 15:11:36 +0100 Subject: [PATCH 13/24] Fix asm! macro options --- mythril/src/error.rs | 11 +++++++---- mythril/src/interrupt/mod.rs | 4 ++-- mythril/src/logger.rs | 1 + mythril/src/percore.rs | 7 +++++-- mythril/src/registers.rs | 6 ++++-- mythril/src/vmx.rs | 4 ++-- 6 files changed, 21 insertions(+), 12 deletions(-) diff --git a/mythril/src/error.rs b/mythril/src/error.rs index 4d1a780..cffb5db 100644 --- a/mythril/src/error.rs +++ b/mythril/src/error.rs @@ -52,9 +52,12 @@ pub fn check_vm_insruction(rflags: u64, error: String) -> Result<()> { } else if rflags.contains(RFlags::FLAGS_ZF) { let errno = unsafe { let value: u64; - asm!("vmread rdx, rax", - in("rax") vmcs::VmcsField::VmInstructionError as u64, - out("rdx") value); + asm!( + "vmread rdx, rax", + in("rax") vmcs::VmcsField::VmInstructionError as u64, + out("rdx") value, + options(nostack) + ); value }; let vm_error = VmInstructionError::try_from(errno) @@ -135,7 +138,7 @@ fn panic_handler(info: &core::panic::PanicInfo) -> ! { loop { unsafe { // Try to at least keep CPU from running at 100% - asm!("hlt"); + asm!("hlt", options(nostack, nomem)); } } } diff --git a/mythril/src/interrupt/mod.rs b/mythril/src/interrupt/mod.rs index 199e73e..97665e6 100644 --- a/mythril/src/interrupt/mod.rs +++ b/mythril/src/interrupt/mod.rs @@ -5,9 +5,9 @@ pub const TIMER_VECTOR: u8 = 48; pub const IPC_VECTOR: u8 = 49; pub unsafe fn enable_interrupts() { - asm!("sti"); + asm!("sti", options(nomem, nostack)); } pub unsafe fn disable_interrupts() { - asm!("cli"); + asm!("cli", options(nomem, nostack)); } diff --git a/mythril/src/logger.rs b/mythril/src/logger.rs index 4217dee..ec3323a 100644 --- a/mythril/src/logger.rs +++ b/mythril/src/logger.rs @@ -89,6 +89,7 @@ pub unsafe fn raw_write_console(s: impl AsRef) { in("rdx") 0x3f8, in("rcx") len as u64, inout("rsi") ptr as u64 => _, + options(nostack) ); } diff --git a/mythril/src/percore.rs b/mythril/src/percore.rs index 9efff66..a6364a0 100644 --- a/mythril/src/percore.rs +++ b/mythril/src/percore.rs @@ -81,8 +81,11 @@ impl fmt::Display for CoreId { pub fn read_core_id() -> CoreId { unsafe { let value: u64; - asm!("mov rax, fs", - out("rax") value); + asm!( + "mov rax, fs", + lateout("rax") value, + options(nomem, nostack) + ); ((value >> 3) as u32).into() // Shift away the RPL and TI bits (they will always be 0) } } diff --git a/mythril/src/registers.rs b/mythril/src/registers.rs index 74b381a..c002cde 100644 --- a/mythril/src/registers.rs +++ b/mythril/src/registers.rs @@ -15,7 +15,8 @@ impl IdtrBase { }; asm!( "sidt fword ptr [{0}]", - in(reg) &mut info + in(reg) &mut info, + options(nostack) ); info.base_addr } @@ -36,7 +37,8 @@ impl GdtrBase { let mut info = GdtInfo { size: 0, offset: 0 }; asm!( "sgdt fword ptr [{0}]", - in(reg) &mut info + in(reg) &mut info, + options(nostack) ); info.offset } diff --git a/mythril/src/vmx.rs b/mythril/src/vmx.rs index 4748fcb..853c8a9 100644 --- a/mythril/src/vmx.rs +++ b/mythril/src/vmx.rs @@ -26,7 +26,7 @@ impl Vmx { "mov cr0, rax", in("rdx") 0x20, lateout("rax") _, - options(nomem) + options(nomem, nostack) ); // Enable vmx in CR4 @@ -36,7 +36,7 @@ impl Vmx { "mov cr4, rax", in("rdx") VMX_ENABLE_FLAG, lateout("rax") _, - options(nomem) + options(nomem, nostack) ); } From 67118875cecb526c62a07c92a426ebff104f7e13 Mon Sep 17 00:00:00 2001 From: Mael Guillossou Date: Thu, 21 Jan 2021 15:52:10 +0100 Subject: [PATCH 14/24] Remove hardcoded register --- mythril/src/interrupt/idt.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mythril/src/interrupt/idt.rs b/mythril/src/interrupt/idt.rs index 8c2ac9f..145c12f 100644 --- a/mythril/src/interrupt/idt.rs +++ b/mythril/src/interrupt/idt.rs @@ -134,8 +134,8 @@ macro_rules! interrupt_fn_impl { let rbp: usize; asm!( - "mov rax, rbp", - out("rax") rbp, + "mov {}, rbp", + out(reg) rbp, options(nomem, nostack) ); From 1925e0a28a5f9e7dae2256595e1b98d56fb0430f Mon Sep 17 00:00:00 2001 From: Benjamin Fogle Date: Fri, 22 Jan 2021 14:22:50 -0500 Subject: [PATCH 15/24] Replace magic numbers with constants in linux.rs --- mythril/src/linux.rs | 65 +++++++++++++++++++++++++++++++++----------- 1 file changed, 49 insertions(+), 16 deletions(-) diff --git a/mythril/src/linux.rs b/mythril/src/linux.rs index f7dba14..d31fa89 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; @@ -56,19 +88,19 @@ pub fn load_linux( ))); } - let magic = LittleEndian::read_u32(&kernel[0x202..0x202 + 4]); + let magic = LittleEndian::read_u32(&kernel[offsets::HEADER_MAGIC]); // HdrS - if magic != 0x53726448 { + if magic != HEADER_MAGIC_VALUE { return Err(Error::InvalidValue(format!( "Invalid kernel image (bad magic = 0x{:x})", magic ))); } - let protocol = LittleEndian::read_u16(&kernel[0x206..0x206 + 2]); + let protocol = LittleEndian::read_u16(&kernel[offsets::BOOTP_VERSION]); let (real_addr, cmdline_addr, prot_addr) = - if protocol < 0x200 || (kernel[0x211] & 0x01) == 0 { + 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) @@ -79,13 +111,13 @@ pub fn load_linux( 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 +133,12 @@ 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,14 +148,14 @@ 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, ); } @@ -145,13 +178,13 @@ pub fn load_linux( 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, From e30e2038e817ff76670fa34afda782af54b6c48c Mon Sep 17 00:00:00 2001 From: Benjamin Fogle Date: Fri, 22 Jan 2021 14:28:47 -0500 Subject: [PATCH 16/24] Format cleanup --- mythril/src/linux.rs | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/mythril/src/linux.rs b/mythril/src/linux.rs index d31fa89..d21d191 100644 --- a/mythril/src/linux.rs +++ b/mythril/src/linux.rs @@ -99,14 +99,15 @@ pub fn load_linux( } 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) - }; + 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); @@ -133,10 +134,15 @@ pub fn load_linux( builder.add_i32(FwCfgSelector::CMDLINE_SIZE, cmdline.len() as i32); if protocol >= 0x202 { - LittleEndian::write_i32(&mut kernel[offsets::CMD_LINE_PTR], cmdline_addr); + LittleEndian::write_i32( + &mut kernel[offsets::CMD_LINE_PTR], + cmdline_addr, + ); } else { - LittleEndian::write_u16(&mut kernel[offsets::OLD_CMD_LINE_MAGIC], - OLD_CMD_LINE_MAGIC_VALUE); + LittleEndian::write_u16( + &mut kernel[offsets::OLD_CMD_LINE_MAGIC], + OLD_CMD_LINE_MAGIC_VALUE, + ); LittleEndian::write_i16( &mut kernel[offsets::OLD_CMD_LINE_OFFSET], (cmdline_addr - real_addr) as i16, From ced6a827d6a8438173745a631fb0ddfb991cf615 Mon Sep 17 00:00:00 2001 From: Francis Nixon Date: Fri, 22 Jan 2021 11:27:36 -0500 Subject: [PATCH 17/24] Removal of strings in Error --- mythril/src/acpi/madt.rs | 61 +++++++------ mythril/src/acpi/mod.rs | 18 ++-- mythril/src/acpi/rsdp.rs | 34 ++++---- mythril/src/acpi/rsdt.rs | 16 ++-- mythril/src/emulate/controlreg.rs | 5 +- mythril/src/emulate/memio.rs | 27 +++--- mythril/src/error.rs | 38 ++++---- mythril/src/ioapic.rs | 135 +++++++++++++++-------------- mythril/src/linux.rs | 49 +++++------ mythril/src/memory.rs | 32 +++---- mythril/src/physdev/keyboard.rs | 15 ++-- mythril/src/physdev/pit.rs | 24 +++-- mythril/src/tsc.rs | 3 +- mythril/src/vcpu.rs | 8 +- mythril/src/virtdev/lapic.rs | 14 ++- mythril/src/virtdev/mod.rs | 65 +++++++------- mythril/src/virtdev/pci.rs | 7 +- mythril/src/virtdev/pit.rs | 33 +++---- mythril/src/virtdev/qemu_fw_cfg.rs | 17 ++-- mythril/src/virtdev/vga.rs | 21 ++--- mythril/src/vm.rs | 19 ++-- mythril/src/vmcs.rs | 15 ++-- mythril/src/vmexit.rs | 7 +- 23 files changed, 313 insertions(+), 350 deletions(-) diff --git a/mythril/src/acpi/madt.rs b/mythril/src/acpi/madt.rs index a29e61a..aeb19ca 100644 --- a/mythril/src/acpi/madt.rs +++ b/mythril/src/acpi/madt.rs @@ -16,6 +16,7 @@ use num_enum::TryFromPrimitive; /// SDT (the end of the Creator Revision at offset 36). mod offsets { use super::*; + /// 32-bit physical address at which each processor can access its /// local APIC. pub const LOCAL_INT_CTRL_ADDR: Range = 0..4; @@ -80,10 +81,9 @@ 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,16 +254,17 @@ 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) + } } } /// Encode into the byte sequence - pub fn encode>( + pub fn encode>( &self, buffer: &mut ArrayVec, ) -> Result<()> { @@ -294,10 +295,9 @@ impl Ics { NativeEndian::write_u32(&mut tmp_buf[8..12], gsi_base); } _ => { - return Err(Error::NotImplemented(format!( - "The ICS Type {:?} has not been implemented", - self.ics_type() - ))) + error!("The ICS Type {:?} has not been implemented", + self.ics_type()); + return Err(Error::NotImplemented); } } buffer.try_extend_from_slice( @@ -393,26 +393,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!( - "Payload for type=0x{:x} and len={} to big for buffer len={}", - ty as u8, - len, - self.bytes.len() - )))); + 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]; @@ -437,7 +434,7 @@ pub struct MADTBuilder { structures: ArrayVec, } -impl> MADTBuilder { +impl> MADTBuilder { /// Create a new builder for the MADT SDT. pub fn new() -> MADTBuilder { MADTBuilder { @@ -465,8 +462,8 @@ impl> MADTBuilder { } impl SDTBuilder for MADTBuilder -where - U: Array, + where + U: Array, { const SIGNATURE: [u8; 4] = [b'A', b'P', b'I', b'C']; @@ -475,7 +472,7 @@ where 5u8 } - fn encode_table>( + fn encode_table>( &mut self, buffer: &mut ArrayVec, ) -> Result<()> { diff --git a/mythril/src/acpi/mod.rs b/mythril/src/acpi/mod.rs index 4b157ff..a92068e 100644 --- a/mythril/src/acpi/mod.rs +++ b/mythril/src/acpi/mod.rs @@ -48,11 +48,8 @@ pub(self) fn verify_checksum(bytes: &[u8], cksum_idx: usize) -> Result<()> { if (result & 0xff) == 0x00 { Ok(()) } else { - Err(Error::InvalidValue(format!( - "Checksum mismatch checksum={:x} {:x} != 0x00", - bytes[cksum_idx], - result & 0xff, - ))) + error!("Checksum mismatch checksum={:x} {:x} != 0x00", bytes[cksum_idx], result & 0xff); + Err(Error::InvalidValue) } } @@ -137,11 +134,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 +166,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..6680acd 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,10 @@ 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 +178,12 @@ 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,11 @@ 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) + } } } @@ -220,11 +222,11 @@ impl RSDP { } /// Builder structure for the RSDP -pub struct RSDPBuilder<'a, T: Array> { +pub struct RSDPBuilder<'a, T: Array> { builder: RSDTBuilder<'a, T>, } -impl<'a, T: Array> RSDPBuilder<'a, T> { +impl<'a, T: Array> RSDPBuilder<'a, T> { /// Create a new RSDP Builder. pub fn new( map: ManagedMap<'a, [u8; 4], (ArrayVec, usize)>, diff --git a/mythril/src/acpi/rsdt.rs b/mythril/src/acpi/rsdt.rs index 8a57114..36e75f5 100644 --- a/mythril/src/acpi/rsdt.rs +++ b/mythril/src/acpi/rsdt.rs @@ -246,11 +246,10 @@ 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!( - "Buffer length should be at least `{}` but was `{}`", - sdt_len, - buffer.len() - ))); + 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 +338,9 @@ impl<'a, T: Array> RSDTBuilder<'a, T> { Ok(()) } } else { - Err(Error::InvalidValue(format!( - "The key `{}` already exists", - str::from_utf8(&U::SIGNATURE).unwrap() - ))) + error!("The key `{}` already exists", + str::from_utf8(&U::SIGNATURE).unwrap()); + Err(Error::InvalidValue) } } diff --git a/mythril/src/emulate/controlreg.rs b/mythril/src/emulate/controlreg.rs index b683bbd..cff1cfe 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..4c63d1f 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,9 @@ 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 +431,11 @@ fn process_memio_op( { do_mmio_read(addr, vcpu, guest_cpu, responses, instr, on_read)?; } else { - return Err(Error::InvalidValue(format!( - "Unsupported mmio instruction: {:?} (rip=0x{:x}, bytes={:?})", - instr.code(), - ip, - bytes, - ))); + error!("Unsupported mmio instruction: {:?} (rip=0x{:x}, bytes={:?})", + instr.code(), + ip, + bytes); + return Err(Error::InvalidValue); } Ok(()) } diff --git a/mythril/src/error.rs b/mythril/src/error.rs index cffb5db..226d320 100644 --- a/mythril/src/error.rs +++ b/mythril/src/error.rs @@ -48,7 +48,8 @@ pub fn check_vm_insruction(rflags: u64, error: String) -> Result<()> { let rflags = rflags::RFlags::from_bits_truncate(rflags); if rflags.contains(RFlags::FLAGS_CF) { - Err(Error::VmFailInvalid(error)) + error!("{}",error); + Err(Error::VmFailInvalid) } else if rflags.contains(RFlags::FLAGS_ZF) { let errno = unsafe { let value: u64; @@ -63,7 +64,9 @@ pub fn check_vm_insruction(rflags: u64, error: String) -> Result<()> { let vm_error = VmInstructionError::try_from(errno) .unwrap_or(VmInstructionError::UnknownError); - Err(Error::VmFailValid((vm_error, error))) + error!("{:?}",vm_error); + error!("{}",error); + Err(Error::VmFailValid) } else { Ok(()) } @@ -71,40 +74,37 @@ 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, } 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 } } diff --git a/mythril/src/ioapic.rs b/mythril/src/ioapic.rs index 9acd301..978a1a9 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,9 @@ 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 +211,9 @@ 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 +237,9 @@ 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 +267,13 @@ 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!( - "Read-only IO Redirect Table Entry bits set: 0x{:x}", - val & !IOREDTBL_RW_MASK - ))) + 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 +302,11 @@ 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,11 @@ 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 +371,11 @@ 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 +397,11 @@ 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 +453,11 @@ 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 +479,11 @@ 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 +550,9 @@ impl IoRedTblEntry { if self.trigger_mode == TriggerMode::Level && !self.delivery_mode.valid_for_level_trigger() { - return Err(Error::InvalidValue(format!( - "The delivery mode `0b{:b}` is invalid for level trigger mode", - self.delivery_mode as u8 - ))); + 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 +561,15 @@ impl IoRedTblEntry { if self.destination_mode == DestinationMode::Physical && self.destination > 15 { - return Err(Error::InvalidValue(format!( - "Invalid Physical APIC ID destination: 0x{:x}", - self.destination - ))); + 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!( - "SMI delivery mode requires an empty vector: 0x{:x}", - self.vector - ))); + error!("SMI delivery mode requires an empty vector: 0x{:x}", + self.vector); + return Err(Error::InvalidValue); } Ok(()) @@ -644,7 +643,6 @@ impl From for u64 { #[cfg(test)] mod test { use super::*; - use alloc::string::ToString; fn get_ioapic(buf: *mut u8) -> Result { let ics = Ics::IoApic { @@ -677,10 +675,11 @@ mod test { fn ioredtblentry_invalid_trigger_mode() { // 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; + // ( + // "The delivery mode `0b111` is invalid for level trigger mode" + // .to_string(), + // ); assert_eq!( IoRedTblEntry::try_from(invalid_for_level).unwrap_err(), err @@ -701,9 +700,10 @@ mod test { // Destination is a full byte but a physical destination mode // is used. let invalid_dest = 0xff000000_0000_0000; - let err = Error::InvalidValue( - "Invalid Physical APIC ID destination: 0xff".to_string(), - ); + let err = Error::InvalidValue; + // ( + // "Invalid Physical APIC ID destination: 0xff".to_string(), + // ); assert_eq!(err, IoRedTblEntry::try_from(invalid_dest).unwrap_err()); } @@ -739,9 +739,10 @@ 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; + // ( + // "Read-only IO Redirect Table Entry bits set: 0x1000".to_string(), + // ); // The delivery status is set, which should be read-only. assert_eq!(err, ioapic.write_ioredtbl(0, entry).unwrap_err()); } diff --git a/mythril/src/linux.rs b/mythril/src/linux.rs index d21d191..beeb87b 100644 --- a/mythril/src/linux.rs +++ b/mythril/src/linux.rs @@ -64,38 +64,34 @@ 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[offsets::HEADER_MAGIC]); // HdrS - if magic != HEADER_MAGIC_VALUE { - return Err(Error::InvalidValue(format!( - "Invalid kernel image (bad magic = 0x{:x})", - magic - ))); + if magic != 0x53726448 { + error!("Invalid kernel image (bad magic = 0x{:x})", + magic); + return Err(Error::InvalidValue); } let protocol = LittleEndian::read_u16(&kernel[offsets::BOOTP_VERSION]); @@ -167,17 +163,15 @@ pub fn load_linux( } 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!( - "Initramfs too large (0x{:x} bytes > max of 0x{:x})", - initramfs.len(), - initrd_max - ))); + 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; @@ -198,9 +192,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/memory.rs b/mythril/src/memory.rs index f178ae3..dd8376e 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,9 @@ 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..dd77efb 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,11 @@ 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/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..03d3af7 100644 --- a/mythril/src/vcpu.rs +++ b/mythril/src/vcpu.rs @@ -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..7ab01c5 100644 --- a/mythril/src/virtdev/lapic.rs +++ b/mythril/src/virtdev/lapic.rs @@ -68,10 +68,9 @@ 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 +91,9 @@ 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..1dd5d50 100644 --- a/mythril/src/virtdev/mod.rs +++ b/mythril/src/virtdev/mod.rs @@ -23,8 +23,9 @@ pub mod rtc; pub mod vga; const MAX_EVENT_RESPONSES: usize = 8; + pub type ResponseEventArray = - ArrayVec<[DeviceEventResponse; MAX_EVENT_RESPONSES]>; +ArrayVec<[DeviceEventResponse; MAX_EVENT_RESPONSES]>; pub type Port = u16; /// Dynamic virtual devices are devices that are not part of the architectural @@ -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,9 @@ 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 +344,9 @@ 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 +359,11 @@ 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 +374,11 @@ 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 +389,11 @@ 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 +451,9 @@ 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..d05dd27 100644 --- a/mythril/src/virtdev/pci.rs +++ b/mythril/src/virtdev/pci.rs @@ -233,10 +233,9 @@ 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..8103f86 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,9 @@ 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 +139,9 @@ 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 +154,9 @@ 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 +175,9 @@ 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..db198e4 100644 --- a/mythril/src/virtdev/qemu_fw_cfg.rs +++ b/mythril/src/virtdev/qemu_fw_cfg.rs @@ -191,16 +191,14 @@ 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 +368,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..d3563de 100644 --- a/mythril/src/virtdev/vga.rs +++ b/mythril/src/virtdev/vga.rs @@ -75,10 +75,9 @@ impl VgaController { val.copy_from_u32(self.registers[self.index as usize] as u32); } _ => { - return Err(Error::NotImplemented(format!( - "Unsupported attempt to read from vga port 0x{:x}", - port - ))) + error!("Unsupported attempt to read from vga port 0x{:x}", + port); + return Err(Error::NotImplemented) } } Ok(()) @@ -105,20 +104,18 @@ impl VgaController { self.registers[self.index as usize] = data; } _ => { - return Err(Error::InvalidValue(format!( - "Invalid port write to VGA index register: {:?}", - val - ))) + 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..1feeb64 100644 --- a/mythril/src/vm.rs +++ b/mythril/src/vm.rs @@ -226,10 +226,9 @@ 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 +265,9 @@ 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) } @@ -674,7 +672,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 +715,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 51d907b..c6fce5f 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)?; diff --git a/mythril/src/vmexit.rs b/mythril/src/vmexit.rs index 32e5bd5..c568775 100644 --- a/mythril/src/vmexit.rs +++ b/mythril/src/vmexit.rs @@ -221,10 +221,9 @@ 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 { From 6782d9ade579a67e05bbe2d61d8da64583884762 Mon Sep 17 00:00:00 2001 From: Francis Nixon Date: Fri, 22 Jan 2021 12:40:30 -0500 Subject: [PATCH 18/24] removed table_name format!() allocation. --- mythril/src/acpi/rsdt.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/mythril/src/acpi/rsdt.rs b/mythril/src/acpi/rsdt.rs index 36e75f5..3c21340 100644 --- a/mythril/src/acpi/rsdt.rs +++ b/mythril/src/acpi/rsdt.rs @@ -371,16 +371,21 @@ 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 LEN_OF_ETC_MYTHRIL :usize = 12; + const LEN_OF_NAME: usize = 4; + let mut table_name_bytes = [0u8;LEN_OF_ETC_MYTHRIL + LEN_OF_NAME]; + table_name_bytes[0..LEN_OF_ETC_MYTHRIL].copy_from_slice("etc/mythril/".as_bytes()); + table_name_bytes[LEN_OF_ETC_MYTHRIL..].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, From 197837f310293f433aaa11d52a1a034cbc2b588e Mon Sep 17 00:00:00 2001 From: Francis Nixon Date: Fri, 22 Jan 2021 12:52:08 -0500 Subject: [PATCH 19/24] got rid of check_vm_instruction related allocations. --- mythril/src/acpi/madt.rs | 44 +++++++++++---------- mythril/src/acpi/mod.rs | 6 ++- mythril/src/acpi/rsdp.rs | 21 +++++----- mythril/src/acpi/rsdt.rs | 21 ++++++---- mythril/src/emulate/controlreg.rs | 2 +- mythril/src/emulate/memio.rs | 17 ++++---- mythril/src/error.rs | 15 ++++--- mythril/src/ioapic.rs | 63 +++++++++++++++--------------- mythril/src/linux.rs | 20 +++++----- mythril/src/memory.rs | 3 +- mythril/src/physdev/pit.rs | 3 +- mythril/src/vcpu.rs | 2 +- mythril/src/virtdev/lapic.rs | 8 ++-- mythril/src/virtdev/mod.rs | 22 ++++------- mythril/src/virtdev/pci.rs | 5 +-- mythril/src/virtdev/pit.rs | 20 ++++------ mythril/src/virtdev/qemu_fw_cfg.rs | 5 +-- mythril/src/virtdev/vga.rs | 21 +++++----- mythril/src/vm.rs | 6 +-- mythril/src/vmcs.rs | 8 ++-- mythril/src/vmexit.rs | 7 ++-- mythril/src/vmx.rs | 8 ++-- 22 files changed, 160 insertions(+), 167 deletions(-) diff --git a/mythril/src/acpi/madt.rs b/mythril/src/acpi/madt.rs index aeb19ca..9ebd51d 100644 --- a/mythril/src/acpi/madt.rs +++ b/mythril/src/acpi/madt.rs @@ -16,7 +16,6 @@ use num_enum::TryFromPrimitive; /// SDT (the end of the Creator Revision at offset 36). mod offsets { use super::*; - /// 32-bit physical address at which each processor can access its /// local APIC. pub const LOCAL_INT_CTRL_ADDR: Range = 0..4; @@ -81,8 +80,7 @@ impl IcsType { if length == self.expected_len() as usize - 2 { Ok(()) } else { - error!("Invalid length={} for type=0x{:x}", - *self as u8, length); + error!("Invalid length={} for type=0x{:x}", *self as u8, length); Err(Error::InvalidValue) } } @@ -255,16 +253,18 @@ impl Ics { apic_proc_uid: NativeEndian::read_u32(&bytes[10..14]), }), _ => { - error!("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) } } } /// Encode into the byte sequence - pub fn encode>( + pub fn encode>( &self, buffer: &mut ArrayVec, ) -> Result<()> { @@ -295,8 +295,10 @@ impl Ics { NativeEndian::write_u32(&mut tmp_buf[8..12], gsi_base); } _ => { - error!("The ICS Type {:?} has not been implemented", - self.ics_type()); + error!( + "The ICS Type {:?} has not been implemented", + self.ics_type() + ); return Err(Error::NotImplemented); } } @@ -393,22 +395,22 @@ impl<'a> Iterator for IcsIterator<'a> { let ty = match IcsType::try_from(self.bytes[0]) { Ok(ty) => ty, _ => { - error!("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() { - error!("Payload for type=0x{:x} and len={} to big for buffer len={}", - ty as u8, - len, - self.bytes.len()); + 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 { - error!("length `{}` provided is too small", - len); + error!("length `{}` provided is too small", len); return Some(Err(Error::InvalidValue)); } @@ -434,7 +436,7 @@ pub struct MADTBuilder { structures: ArrayVec, } -impl> MADTBuilder { +impl> MADTBuilder { /// Create a new builder for the MADT SDT. pub fn new() -> MADTBuilder { MADTBuilder { @@ -462,8 +464,8 @@ impl> MADTBuilder { } impl SDTBuilder for MADTBuilder - where - U: Array, +where + U: Array, { const SIGNATURE: [u8; 4] = [b'A', b'P', b'I', b'C']; @@ -472,7 +474,7 @@ impl SDTBuilder for MADTBuilder 5u8 } - fn encode_table>( + fn encode_table>( &mut self, buffer: &mut ArrayVec, ) -> Result<()> { diff --git a/mythril/src/acpi/mod.rs b/mythril/src/acpi/mod.rs index a92068e..05983e7 100644 --- a/mythril/src/acpi/mod.rs +++ b/mythril/src/acpi/mod.rs @@ -48,7 +48,11 @@ pub(self) fn verify_checksum(bytes: &[u8], cksum_idx: usize) -> Result<()> { if (result & 0xff) == 0x00 { Ok(()) } else { - error!("Checksum mismatch checksum={:x} {:x} != 0x00", bytes[cksum_idx], result & 0xff); + error!( + "Checksum mismatch checksum={:x} {:x} != 0x00", + bytes[cksum_idx], + result & 0xff + ); Err(Error::InvalidValue) } } diff --git a/mythril/src/acpi/rsdp.rs b/mythril/src/acpi/rsdp.rs index 6680acd..62fd08e 100644 --- a/mythril/src/acpi/rsdp.rs +++ b/mythril/src/acpi/rsdp.rs @@ -117,10 +117,8 @@ impl RSDP { xsdt_addr: NativeEndian::read_u64(&bytes[offsets::XSDT_ADDR]), }, _ => { - error!("Invalid RSDP revision: {}", - bytes[offsets::REVISION]); - return Err(Error::InvalidValue) - .into(); + error!("Invalid RSDP revision: {}", bytes[offsets::REVISION]); + return Err(Error::InvalidValue).into(); } }; @@ -179,9 +177,11 @@ impl RSDP { Ok(&range[i..rsdp_v2_end]) } _ => { - error!("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) } }; @@ -205,8 +205,7 @@ impl RSDP { offsets::EXT_CHECKSUM, ), _ => { - error!("Invalid RSDP revision: {}", - bytes[offsets::REVISION]); + error!("Invalid RSDP revision: {}", bytes[offsets::REVISION]); Err(Error::InvalidValue) } } @@ -222,11 +221,11 @@ impl RSDP { } /// Builder structure for the RSDP -pub struct RSDPBuilder<'a, T: Array> { +pub struct RSDPBuilder<'a, T: Array> { builder: RSDTBuilder<'a, T>, } -impl<'a, T: Array> RSDPBuilder<'a, T> { +impl<'a, T: Array> RSDPBuilder<'a, T> { /// Create a new RSDP Builder. pub fn new( map: ManagedMap<'a, [u8; 4], (ArrayVec, usize)>, diff --git a/mythril/src/acpi/rsdt.rs b/mythril/src/acpi/rsdt.rs index 3c21340..a63013f 100644 --- a/mythril/src/acpi/rsdt.rs +++ b/mythril/src/acpi/rsdt.rs @@ -246,9 +246,11 @@ fn write_sdt_header( // The SDT length value is the value of the entire SDT including // the header. if buffer.len() < sdt_len { - error!("Buffer length should be at least `{}` but was `{}`", - sdt_len, - buffer.len()); + 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 @@ -338,8 +340,10 @@ impl<'a, T: Array> RSDTBuilder<'a, T> { Ok(()) } } else { - error!("The key `{}` already exists", - str::from_utf8(&U::SIGNATURE).unwrap()); + error!( + "The key `{}` already exists", + str::from_utf8(&U::SIGNATURE).unwrap() + ); Err(Error::InvalidValue) } } @@ -371,10 +375,11 @@ impl<'a, T: Array> RSDTBuilder<'a, T> { })?; for (i, (name, (sdt, size))) in self.map.iter().enumerate() { - const LEN_OF_ETC_MYTHRIL :usize = 12; + const LEN_OF_ETC_MYTHRIL: usize = 12; const LEN_OF_NAME: usize = 4; - let mut table_name_bytes = [0u8;LEN_OF_ETC_MYTHRIL + LEN_OF_NAME]; - table_name_bytes[0..LEN_OF_ETC_MYTHRIL].copy_from_slice("etc/mythril/".as_bytes()); + let mut table_name_bytes = [0u8; LEN_OF_ETC_MYTHRIL + LEN_OF_NAME]; + table_name_bytes[0..LEN_OF_ETC_MYTHRIL] + .copy_from_slice("etc/mythril/".as_bytes()); table_name_bytes[LEN_OF_ETC_MYTHRIL..].copy_from_slice(name); let table_name = str::from_utf8(&table_name_bytes)?; diff --git a/mythril/src/emulate/controlreg.rs b/mythril/src/emulate/controlreg.rs index cff1cfe..2b7348f 100644 --- a/mythril/src/emulate/controlreg.rs +++ b/mythril/src/emulate/controlreg.rs @@ -61,7 +61,7 @@ pub fn emulate_access( }, _ => { error!("Unsupported CR number access"); - return Err(Error::InvalidValue) + return Err(Error::InvalidValue); } } Ok(()) diff --git a/mythril/src/emulate/memio.rs b/mythril/src/emulate/memio.rs index 4c63d1f..1f3804f 100644 --- a/mythril/src/emulate/memio.rs +++ b/mythril/src/emulate/memio.rs @@ -145,7 +145,7 @@ fn read_register_value( _ => { error!("Invalid register '{:?}'", register); - return Err(Error::InvalidValue) + return Err(Error::InvalidValue); } } @@ -351,9 +351,8 @@ fn do_mmio_read( } register => { - error!("mmio read into invalid register '{:?}'", - register); - return Err(Error::InvalidValue) + error!("mmio read into invalid register '{:?}'", register); + return Err(Error::InvalidValue); } }, _ => return Err(Error::NotSupported), @@ -431,10 +430,12 @@ fn process_memio_op( { do_mmio_read(addr, vcpu, guest_cpu, responses, instr, on_read)?; } else { - error!("Unsupported mmio instruction: {:?} (rip=0x{:x}, bytes={:?})", - instr.code(), - ip, - bytes); + error!( + "Unsupported mmio instruction: {:?} (rip=0x{:x}, bytes={:?})", + instr.code(), + ip, + bytes + ); return Err(Error::InvalidValue); } Ok(()) diff --git a/mythril/src/error.rs b/mythril/src/error.rs index 226d320..f68a047 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,11 +43,11 @@ 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) { - error!("{}",error); + log_error(); Err(Error::VmFailInvalid) } else if rflags.contains(RFlags::FLAGS_ZF) { let errno = unsafe { @@ -64,8 +63,8 @@ pub fn check_vm_insruction(rflags: u64, error: String) -> Result<()> { let vm_error = VmInstructionError::try_from(errno) .unwrap_or(VmInstructionError::UnknownError); - error!("{:?}",vm_error); - error!("{}",error); + error!("{:?}", vm_error); + log_error(); Err(Error::VmFailValid) } else { Ok(()) @@ -89,21 +88,21 @@ pub enum Error { impl From> for Error { fn from(error: TryFromPrimitiveError) -> Error { - error!("{}",error); + error!("{}", error); Error::InvalidValue } } impl From for Error { fn from(error: TryFromIntError) -> Error { - error!("{}",error); + error!("{}", error); Error::InvalidValue } } impl From for Error { fn from(error: core::str::Utf8Error) -> Error { - error!("{}",error); + error!("{}", error); Error::InvalidValue } } diff --git a/mythril/src/ioapic.rs b/mythril/src/ioapic.rs index 978a1a9..434fda6 100644 --- a/mythril/src/ioapic.rs +++ b/mythril/src/ioapic.rs @@ -171,8 +171,7 @@ impl IoApic { /// See section 3.2.1 of the I/O APIC specification. pub fn set_id(&self, id: u32) -> Result<()> { if id > 0x0f { - error!("I/O APIC ID `0x{:x}` too large", - id); + error!("I/O APIC ID `0x{:x}` too large", id); Err(Error::InvalidValue) } else { unsafe { @@ -211,8 +210,7 @@ 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 { - error!("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 { @@ -237,8 +235,7 @@ impl IoApic { /// Read the IO Redirect Table Register for a given id. pub fn read_ioredtbl(&self, id: u8) -> Result { if id > 23 { - error!("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) }; @@ -267,12 +264,13 @@ impl IoApic { pub fn write_ioredtbl(&self, id: u8, entry: IoRedTblEntry) -> Result<()> { let val: u64 = entry.into(); if id > 23 { - error!("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 { - error!("Read-only IO Redirect Table Entry bits set: 0x{:x}", - val & !IOREDTBL_RW_MASK); + error!( + "Read-only IO Redirect Table Entry bits set: 0x{:x}", + val & !IOREDTBL_RW_MASK + ); Err(Error::InvalidValue) } else { unsafe { @@ -303,8 +301,10 @@ impl TryFrom for IoApic { .. } => IoApic::new(ioapic_addr, gsi_base), _ => { - error!("Attempting to create an IoApic from: {:?}", - value.ics_type()); + error!( + "Attempting to create an IoApic from: {:?}", + value.ics_type() + ); Err(Error::InvalidValue) } } @@ -346,8 +346,7 @@ impl TryFrom for DestinationMode { 0x00 => Ok(DestinationMode::Physical), 0x01 => Ok(DestinationMode::Logical), _ => { - error!("Invalid destination mode: 0x{:x}", - value); + error!("Invalid destination mode: 0x{:x}", value); Err(Error::InvalidValue) } } @@ -372,8 +371,7 @@ impl TryFrom for TriggerMode { 0x00 => Ok(TriggerMode::Edge), 0x01 => Ok(TriggerMode::Level), _ => { - error!("Invalid trigger mode: 0x{:x}", - value); + error!("Invalid trigger mode: 0x{:x}", value); Err(Error::InvalidValue) } } @@ -398,8 +396,7 @@ impl TryFrom for PinPolarity { 0x00 => Ok(PinPolarity::ActiveHigh), 0x01 => Ok(PinPolarity::ActiveLow), _ => { - error!("Invalid pin polarity: 0x{:x}", - value); + error!("Invalid pin polarity: 0x{:x}", value); Err(Error::InvalidValue) } } @@ -454,10 +451,9 @@ impl TryFrom for DeliveryMode { 0b101 => Ok(DeliveryMode::INIT), 0b111 => Ok(DeliveryMode::ExtINT), _ => { - error!("Invalid pin polarity: 0x{:x}", - value); + error!("Invalid pin polarity: 0x{:x}", value); Err(Error::InvalidValue) - }, + } } } } @@ -480,8 +476,7 @@ impl TryFrom for DeliveryStatus { 0x00 => Ok(DeliveryStatus::Idle), 0x01 => Ok(DeliveryStatus::SendPending), _ => { - error!("Invalid delivery status: 0x{:x}", - value); + error!("Invalid delivery status: 0x{:x}", value); Err(Error::InvalidValue) } } @@ -550,8 +545,10 @@ impl IoRedTblEntry { if self.trigger_mode == TriggerMode::Level && !self.delivery_mode.valid_for_level_trigger() { - error!("The delivery mode `0b{:b}` is invalid for level trigger mode", - self.delivery_mode as u8); + error!( + "The delivery mode `0b{:b}` is invalid for level trigger mode", + self.delivery_mode as u8 + ); return Err(Error::InvalidValue); } @@ -561,14 +558,18 @@ impl IoRedTblEntry { if self.destination_mode == DestinationMode::Physical && self.destination > 15 { - error!("Invalid Physical APIC ID destination: 0x{:x}", - self.destination); + error!( + "Invalid Physical APIC ID destination: 0x{:x}", + self.destination + ); return Err(Error::InvalidValue); } if self.delivery_mode == DeliveryMode::SMI && self.vector != 0 { - error!("SMI delivery mode requires an empty vector: 0x{:x}", - self.vector); + error!( + "SMI delivery mode requires an empty vector: 0x{:x}", + self.vector + ); return Err(Error::InvalidValue); } @@ -701,8 +702,8 @@ mod test { // is used. let invalid_dest = 0xff000000_0000_0000; let err = Error::InvalidValue; - // ( - // "Invalid Physical APIC ID destination: 0xff".to_string(), + // ( + // "Invalid Physical APIC ID destination: 0xff".to_string(), // ); assert_eq!(err, IoRedTblEntry::try_from(invalid_dest).unwrap_err()); } diff --git a/mythril/src/linux.rs b/mythril/src/linux.rs index beeb87b..ed0c8f3 100644 --- a/mythril/src/linux.rs +++ b/mythril/src/linux.rs @@ -64,8 +64,7 @@ pub fn load_linux( let mut kernel = info .find_module(kernel_name.as_ref()) .ok_or_else(|| { - error!("No such kernel '{}'", - kernel_name.as_ref()); + error!("No such kernel '{}'", kernel_name.as_ref()); Error::InvalidValue })? .data() @@ -73,15 +72,13 @@ pub fn load_linux( let initramfs = info .find_module(initramfs_name.as_ref()) .ok_or_else(|| { - error!("No such initramfs '{}'", - initramfs_name.as_ref()); + error!("No such initramfs '{}'", initramfs_name.as_ref()); Error::InvalidValue })? .data(); if kernel.len() < 8192 { - error!("Kernel image is too small ({} < 8192)", - kernel.len()); + error!("Kernel image is too small ({} < 8192)", kernel.len()); return Err(Error::InvalidValue); } @@ -89,8 +86,7 @@ pub fn load_linux( // HdrS if magic != 0x53726448 { - error!("Invalid kernel image (bad magic = 0x{:x})", - magic); + error!("Invalid kernel image (bad magic = 0x{:x})", magic); return Err(Error::InvalidValue); } @@ -168,9 +164,11 @@ pub fn load_linux( } if initramfs.len() as u32 > initrd_max { - error!("Initramfs too large (0x{:x} bytes > max of 0x{:x})", - initramfs.len(), - initrd_max); + error!( + "Initramfs too large (0x{:x} bytes > max of 0x{:x})", + initramfs.len(), + initrd_max + ); return Err(Error::InvalidValue); } diff --git a/mythril/src/memory.rs b/mythril/src/memory.rs index dd8376e..13c12c4 100644 --- a/mythril/src/memory.rs +++ b/mythril/src/memory.rs @@ -748,8 +748,7 @@ fn map_guest_memory( let ept_pte = unsafe { &mut (*ept_pt)[guest_addr.p1_index()] }; if !ept_pte.is_unused() { - error!("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); } diff --git a/mythril/src/physdev/pit.rs b/mythril/src/physdev/pit.rs index dd77efb..6e1ee7a 100644 --- a/mythril/src/physdev/pit.rs +++ b/mythril/src/physdev/pit.rs @@ -61,8 +61,7 @@ impl TryFrom for OperatingMode { 0b110 => Ok(OperatingMode::Mode2), 0b111 => Ok(OperatingMode::Mode3), _ => { - error!("Invalid PIT operating mode: {}", - val); + error!("Invalid PIT operating mode: {}", val); Err(Error::InvalidValue) } } diff --git a/mythril/src/vcpu.rs b/mythril/src/vcpu.rs index 03d3af7..2ea94bd 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!() } diff --git a/mythril/src/virtdev/lapic.rs b/mythril/src/virtdev/lapic.rs index 7ab01c5..cb46de5 100644 --- a/mythril/src/virtdev/lapic.rs +++ b/mythril/src/virtdev/lapic.rs @@ -68,8 +68,7 @@ impl TryFrom for ApicRegisterOffset { fn try_from(value: u16) -> Result { if value & 0b1111 != 0 { - error!("APIC register offset not aligned: 0x{:x}", - value); + error!("APIC register offset not aligned: 0x{:x}", value); return Err(Error::InvalidValue); } @@ -91,9 +90,8 @@ impl TryFrom for ApicRegisterOffset { ApicRegisterOffset::InterruptCommand((value - 0x300) >> 4) } offset => { - error!("Invalid APIC register offset: 0x{:x}", - offset); - return Err(Error::InvalidValue) + 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 1dd5d50..0906f45 100644 --- a/mythril/src/virtdev/mod.rs +++ b/mythril/src/virtdev/mod.rs @@ -25,7 +25,7 @@ pub mod vga; const MAX_EVENT_RESPONSES: usize = 8; pub type ResponseEventArray = -ArrayVec<[DeviceEventResponse; MAX_EVENT_RESPONSES]>; + ArrayVec<[DeviceEventResponse; MAX_EVENT_RESPONSES]>; pub type Port = u16; /// Dynamic virtual devices are devices that are not part of the architectural @@ -287,8 +287,7 @@ impl<'a> TryFrom<&'a mut [u8]> for PortReadRequest<'a> { &mut *(buff.as_mut_ptr() as *mut [u8; 4]) }), len => { - error!("Invalid slice length: {}", - len); + error!("Invalid slice length: {}", len); return Err(Error::InvalidValue); } }; @@ -344,8 +343,7 @@ impl<'a> TryFrom<&'a [u8]> for PortWriteRequest<'a> { Self::FourBytes(unsafe { &*(buff.as_ptr() as *const [u8; 4]) }) } len => { - error!("Invalid slice length: {}", - len); + error!("Invalid slice length: {}", len); return Err(Error::InvalidValue); } }; @@ -360,10 +358,9 @@ impl<'a> TryFrom> for u8 { match value { PortWriteRequest::OneByte(val) => Ok(val[0]), val => { - error!("Value {} cannot be converted to u8", - val); + error!("Value {} cannot be converted to u8", val); Err(Error::InvalidValue) - }, + } } } } @@ -375,8 +372,7 @@ impl<'a> TryFrom> for u16 { match value { PortWriteRequest::TwoBytes(val) => Ok(u16::from_be_bytes(*val)), val => { - error!("Value {} cannot be converted to u16", - val); + error!("Value {} cannot be converted to u16", val); Err(Error::InvalidValue) } } @@ -390,8 +386,7 @@ impl<'a> TryFrom> for u32 { match value { PortWriteRequest::FourBytes(val) => Ok(u32::from_be_bytes(*val)), val => { - error!("Value {} cannot be converted to u32", - val); + error!("Value {} cannot be converted to u32", val); Err(Error::InvalidValue) } } @@ -451,8 +446,7 @@ impl<'a> TryInto for MemWriteRequest<'a> { if self.data.len() == 1 { Ok(self.data[0]) } else { - error!("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 d05dd27..11bf581 100644 --- a/mythril/src/virtdev/pci.rs +++ b/mythril/src/virtdev/pci.rs @@ -233,9 +233,8 @@ impl EmulatedDevice for PciRootComplex { } } _ => { - error!("Invalid PCI port read 0x{:x}", - port); - return Err(Error::InvalidValue) + 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 8103f86..8aff77d 100644 --- a/mythril/src/virtdev/pit.rs +++ b/mythril/src/virtdev/pit.rs @@ -127,9 +127,8 @@ impl Pit8254 { start_time: None, }, value => { - error!("Invalid PIT operating state '0x{:x}'", - value); - return Err(Error::InvalidValue) + error!("Invalid PIT operating state '0x{:x}'", value); + return Err(Error::InvalidValue); } }; @@ -139,9 +138,8 @@ impl Pit8254 { 0b10 => AccessModeState::HiByte, 0b11 => AccessModeState::Word { lo_byte: None }, value => { - error!("Invalid PIT access state '0x{:x}'", - value); - return Err(Error::InvalidValue) + error!("Invalid PIT access state '0x{:x}'", value); + return Err(Error::InvalidValue); } }; @@ -154,9 +152,8 @@ impl Pit8254 { 0b00 => &mut self.channel0, 0b10 => &mut self.channel2, value => { - error!("Invalid PIT channel '0x{:x}'", - value); - return Err(Error::InvalidValue) + error!("Invalid PIT channel '0x{:x}'", value); + return Err(Error::InvalidValue); } }; @@ -175,9 +172,8 @@ impl Pit8254 { let channel_state = match port { PIT_COUNTER_0 => &mut self.channel0, PIT_COUNTER_1 => { - error!("Invalid PIT port '0x{:x}'", - port); - return Err(Error::InvalidValue) + 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 db198e4..f2f1d08 100644 --- a/mythril/src/virtdev/qemu_fw_cfg.rs +++ b/mythril/src/virtdev/qemu_fw_cfg.rs @@ -191,8 +191,7 @@ impl QemuFwCfgBuilder { data: &[u8], ) -> Result<()> { if name.as_ref().len() > FW_CFG_MAX_FILE_NAME { - error!("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(); @@ -369,7 +368,7 @@ impl QemuFwCfg { } Self::FW_CFG_PORT_DATA => { error!("Write to QEMU FW CFG data port not yet supported"); - return Err(Error::NotImplemented) + 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 d3563de..cdc077e 100644 --- a/mythril/src/virtdev/vga.rs +++ b/mythril/src/virtdev/vga.rs @@ -75,9 +75,11 @@ impl VgaController { val.copy_from_u32(self.registers[self.index as usize] as u32); } _ => { - error!("Unsupported attempt to read from vga port 0x{:x}", - port); - return Err(Error::NotImplemented) + error!( + "Unsupported attempt to read from vga port 0x{:x}", + port + ); + return Err(Error::NotImplemented); } } Ok(()) @@ -104,18 +106,19 @@ impl VgaController { self.registers[self.index as usize] = data; } _ => { - error!("Invalid port write to VGA index register: {:?}", - val); - return Err(Error::InvalidValue) + 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()?; } _ => { - error!("Unsupported attempt to write to vga port 0x{:x}", - port); - return Err(Error::NotImplemented) + 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 1feeb64..081a9fd 100644 --- a/mythril/src/vm.rs +++ b/mythril/src/vm.rs @@ -226,8 +226,7 @@ impl VirtualMachineSet { .context_by_core_id(core_id) .ok_or_else(|| Error::NotFound)?; context.msgqueue.write().push_back(msg).map_err(|_| { - error!("RX queue is full for core_id = {}", - core_id); + error!("RX queue is full for core_id = {}", core_id); Error::InvalidValue })?; @@ -265,8 +264,7 @@ impl VirtualMachineSet { notify: bool, ) -> Result<()> { let vm_bsp = self.bsp_core_id(vm_id).ok_or_else(|| { - error!("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) diff --git a/mythril/src/vmcs.rs b/mythril/src/vmcs.rs index c6fce5f..72e7fce 100644 --- a/mythril/src/vmcs.rs +++ b/mythril/src/vmcs.rs @@ -336,9 +336,9 @@ fn vmcs_write(field: VmcsField, value: u64) -> Result<()> { rflags }; - error::check_vm_insruction( + error::check_vm_instruction( rflags, - format!("Failed to write 0x{:x} to field {:?}", value, field), + ||error!("Failed to write 0x{:x} to field {:?}", value, field), ) } @@ -375,7 +375,7 @@ fn vmcs_activate(vmcs: &mut Vmcs, _vmx: &vmx::Vmx) -> Result<()> { 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<()> { @@ -390,7 +390,7 @@ fn vmcs_clear(vmcs_page: &mut Raw4kPage) -> Result<()> { ); 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 c568775..a4e6c9e 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,9 +221,8 @@ impl ExitReason { 63 => ExitInformation::Xsaves, 64 => ExitInformation::Xrstors, reason => { - error!("Unexpected basic vmexit reason: {}", - reason); - return Err(Error::InvalidValue) + 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 853c8a9..78f7546 100644 --- a/mythril/src/vmx.rs +++ b/mythril/src/vmx.rs @@ -63,7 +63,7 @@ impl Vmx { 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, }) @@ -83,7 +83,7 @@ impl Vmx { 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 { @@ -108,7 +108,7 @@ impl Vmx { ); 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<()> { @@ -135,7 +135,7 @@ impl Vmx { ); rflags }; - error::check_vm_insruction(rflags, "Failed to execute invvpid".into()) + error::check_vm_instruction(rflags, ||error!("Failed to execute invvpid")) } } From 16361917426d122eea9eb3cb5305343813797f8c Mon Sep 17 00:00:00 2001 From: Francis Nixon Date: Fri, 22 Jan 2021 15:47:24 -0500 Subject: [PATCH 20/24] handle testing of log outputs. --- mythril/Cargo.lock | 10 ++++++++++ mythril/Cargo.toml | 3 +++ mythril/src/ioapic.rs | 41 +++++++++++++++++++++++++++++++---------- mythril/src/vcpu.rs | 2 +- mythril/src/vmcs.rs | 11 +++++------ mythril/src/vmexit.rs | 2 +- mythril/src/vmx.rs | 12 ++++++++---- 7 files changed, 59 insertions(+), 22 deletions(-) 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/ioapic.rs b/mythril/src/ioapic.rs index 434fda6..99e0894 100644 --- a/mythril/src/ioapic.rs +++ b/mythril/src/ioapic.rs @@ -643,6 +643,10 @@ impl From for u64 { #[cfg(test)] mod test { + + use log::Level; + extern crate testing_logger; + use super::*; fn get_ioapic(buf: *mut u8) -> Result { @@ -674,17 +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(), - // ); 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,16 +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(), - // ); 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, @@ -741,11 +757,16 @@ mod test { let entry = IoRedTblEntry::try_from(bits).unwrap(); let err = Error::InvalidValue; - // ( - // "Read-only IO Redirect Table Entry bits set: 0x1000".to_string(), - // ); // 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/vcpu.rs b/mythril/src/vcpu.rs index 2ea94bd..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_instruction(rflags, ||error!("Failed to launch vm"))?; + error::check_vm_instruction(rflags, || error!("Failed to launch vm"))?; unreachable!() } diff --git a/mythril/src/vmcs.rs b/mythril/src/vmcs.rs index 72e7fce..d0592ed 100644 --- a/mythril/src/vmcs.rs +++ b/mythril/src/vmcs.rs @@ -336,10 +336,9 @@ fn vmcs_write(field: VmcsField, value: u64) -> Result<()> { rflags }; - error::check_vm_instruction( - rflags, - ||error!("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 { @@ -375,7 +374,7 @@ fn vmcs_activate(vmcs: &mut Vmcs, _vmx: &vmx::Vmx) -> Result<()> { rflags }; - error::check_vm_instruction(rflags, ||error!("Failed to activate VMCS")) + error::check_vm_instruction(rflags, || error!("Failed to activate VMCS")) } fn vmcs_clear(vmcs_page: &mut Raw4kPage) -> Result<()> { @@ -390,7 +389,7 @@ fn vmcs_clear(vmcs_page: &mut Raw4kPage) -> Result<()> { ); rflags }; - error::check_vm_instruction(rflags, ||error!("Failed to clear VMCS")) + 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 a4e6c9e..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_instruction(rflags,|| error!("Failed to vmresume")) + error::check_vm_instruction(rflags, || error!("Failed to vmresume")) .expect("vmresume failed"); } diff --git a/mythril/src/vmx.rs b/mythril/src/vmx.rs index 78f7546..e689143 100644 --- a/mythril/src/vmx.rs +++ b/mythril/src/vmx.rs @@ -63,7 +63,7 @@ impl Vmx { rflags }; - error::check_vm_instruction(rflags, ||error!("Failed to enable vmx"))?; + error::check_vm_instruction(rflags, || error!("Failed to enable vmx"))?; Ok(Vmx { _vmxon_region: vmxon_region, }) @@ -83,7 +83,7 @@ impl Vmx { rflags }; - error::check_vm_instruction(rflags, ||error!("Failed to disable vmx")) + error::check_vm_instruction(rflags, || error!("Failed to disable vmx")) } pub fn revision() -> u32 { @@ -108,7 +108,9 @@ impl Vmx { ); rflags }; - error::check_vm_instruction(rflags, ||error!("Failed to execute invept")) + error::check_vm_instruction(rflags, || { + error!("Failed to execute invept") + }) } pub fn invvpid(&self, mode: InvVpidMode) -> Result<()> { @@ -135,7 +137,9 @@ impl Vmx { ); rflags }; - error::check_vm_instruction(rflags, ||error!("Failed to execute invvpid")) + error::check_vm_instruction(rflags, || { + error!("Failed to execute invvpid") + }) } } From bc3969bdf330ef678c75b4a69851c929c9183d37 Mon Sep 17 00:00:00 2001 From: Francis Nixon Date: Fri, 22 Jan 2021 15:57:02 -0500 Subject: [PATCH 21/24] change Makefile slightly to not fail clean if already clean --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 21be92a..a0ce76b 100644 --- a/Makefile +++ b/Makefile @@ -98,9 +98,9 @@ test: test_common .PHONY: clean clean: $(CARGO) clean $(CARGO_MANIFEST) - rm $(seabios_blob) + rm $(seabios_blob) -f make -C seabios clean - rm $(GUEST_ASSETS) + rm $(GUEST_ASSETS) -f .PHONY: dev-init dev-init: install-git-hooks From e9015eb6a21a749d1789332eef28a861ddff6b3c Mon Sep 17 00:00:00 2001 From: Francis Nixon Date: Fri, 26 Feb 2021 10:09:58 -0500 Subject: [PATCH 22/24] put flag before dir. --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index a0ce76b..4874774 100644 --- a/Makefile +++ b/Makefile @@ -98,9 +98,9 @@ test: test_common .PHONY: clean clean: $(CARGO) clean $(CARGO_MANIFEST) - rm $(seabios_blob) -f + rm -f $(seabios_blob) make -C seabios clean - rm $(GUEST_ASSETS) -f + rm -f $(GUEST_ASSETS) .PHONY: dev-init dev-init: install-git-hooks From a479042e66dfdb695ad18601ad3a411dccab59a6 Mon Sep 17 00:00:00 2001 From: Francis Nixon Date: Fri, 26 Feb 2021 10:11:29 -0500 Subject: [PATCH 23/24] fix LEN_OF_ETC_MYTHRIL --- mythril/src/acpi/rsdt.rs | 10 +++++----- mythril/src/linux.rs | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/mythril/src/acpi/rsdt.rs b/mythril/src/acpi/rsdt.rs index a63013f..993971e 100644 --- a/mythril/src/acpi/rsdt.rs +++ b/mythril/src/acpi/rsdt.rs @@ -375,12 +375,12 @@ impl<'a, T: Array> RSDTBuilder<'a, T> { })?; for (i, (name, (sdt, size))) in self.map.iter().enumerate() { - const LEN_OF_ETC_MYTHRIL: usize = 12; + const ETC_MYTHRIL: &'static str = "etc/mythril/"; const LEN_OF_NAME: usize = 4; - let mut table_name_bytes = [0u8; LEN_OF_ETC_MYTHRIL + LEN_OF_NAME]; - table_name_bytes[0..LEN_OF_ETC_MYTHRIL] - .copy_from_slice("etc/mythril/".as_bytes()); - table_name_bytes[LEN_OF_ETC_MYTHRIL..].copy_from_slice(name); + 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 { diff --git a/mythril/src/linux.rs b/mythril/src/linux.rs index ed0c8f3..2055469 100644 --- a/mythril/src/linux.rs +++ b/mythril/src/linux.rs @@ -85,7 +85,7 @@ pub fn load_linux( let magic = LittleEndian::read_u32(&kernel[offsets::HEADER_MAGIC]); // HdrS - if magic != 0x53726448 { + if magic != HEADER_MAGIC_VALUE { error!("Invalid kernel image (bad magic = 0x{:x})", magic); return Err(Error::InvalidValue); } From dc6423ac94862c63e7b6aafe24926fcf17f68b64 Mon Sep 17 00:00:00 2001 From: Francis Nixon Date: Fri, 26 Feb 2021 11:19:43 -0500 Subject: [PATCH 24/24] removed some dynamic allocations added since this MR was opened. formatting. --- mythril/src/error.rs | 1 + mythril/src/vm.rs | 7 ++++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/mythril/src/error.rs b/mythril/src/error.rs index f68a047..9e43aba 100644 --- a/mythril/src/error.rs +++ b/mythril/src/error.rs @@ -84,6 +84,7 @@ pub enum Error { InvalidDevice, NotImplemented, DeviceError, + MissingDevice, } impl From> for Error { diff --git a/mythril/src/vm.rs b/mythril/src/vm.rs index 081a9fd..22d4b2f 100644 --- a/mythril/src/vm.rs +++ b/mythril/src/vm.rs @@ -548,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) + } }; } };