From 9e9d025bb57d5b832854f310c0f8a34070c5faf9 Mon Sep 17 00:00:00 2001 From: 4lDO2 <4lDO2@protonmail.com> Date: Thu, 8 Jul 2021 13:28:46 +0200 Subject: [PATCH 1/4] Add support for FUTEX_WAIT64. --- rmm | 2 +- src/arch/x86_64/mod.rs | 2 + src/syscall/futex.rs | 96 +++++++++++++++++++++++++---------------- src/syscall/mod.rs | 2 +- src/syscall/validate.rs | 19 +++++++- syscall | 2 +- 6 files changed, 83 insertions(+), 40 deletions(-) diff --git a/rmm b/rmm index c66956c..32caee3 160000 --- a/rmm +++ b/rmm @@ -1 +1 @@ -Subproject commit c66956ca2ac1b4df3904afbafe53321a4b6c50af +Subproject commit 32caee3095e54a8d9181841fe48d9ea8d43f4e6c diff --git a/src/arch/x86_64/mod.rs b/src/arch/x86_64/mod.rs index 7343d45..88e5073 100644 --- a/src/arch/x86_64/mod.rs +++ b/src/arch/x86_64/mod.rs @@ -41,6 +41,8 @@ pub mod start; /// Stop function pub mod stop; +pub use ::rmm::X8664Arch as CurrentRmmArch; + // Flags pub mod flags { pub const FLAG_SINGLESTEP: usize = 1 << 8; diff --git a/src/syscall/futex.rs b/src/syscall/futex.rs index 8d3582e..a130e12 100644 --- a/src/syscall/futex.rs +++ b/src/syscall/futex.rs @@ -7,14 +7,16 @@ use alloc::collections::VecDeque; use core::intrinsics; use spin::{Once, RwLock, RwLockReadGuard, RwLockWriteGuard}; +use rmm::Arch; + use crate::context::{self, Context}; use crate::time; use crate::memory::PhysicalAddress; use crate::paging::{ActivePageTable, TableKind, VirtualAddress}; use crate::syscall::data::TimeSpec; use crate::syscall::error::{Error, Result, ESRCH, EAGAIN, EFAULT, EINVAL}; -use crate::syscall::flag::{FUTEX_WAIT, FUTEX_WAKE, FUTEX_REQUEUE}; -use crate::syscall::validate::{validate_slice, validate_slice_mut}; +use crate::syscall::flag::{FUTEX_WAIT, FUTEX_WAIT64, FUTEX_WAKE, FUTEX_REQUEUE}; +use crate::syscall::validate::{validate_array, validate_slice, validate_slice_mut}; type FutexList = VecDeque; @@ -41,23 +43,28 @@ pub fn futexes_mut() -> RwLockWriteGuard<'static, FutexList> { FUTEXES.call_once(init_futexes).write() } -// FIXME: Don't take a mutable reference to the addr, since rustc can make assumptions that the -// pointee cannot be changed by another thread, which could make atomic ops useless. -pub fn futex(addr: &mut i32, op: usize, val: i32, val2: usize, addr2: *mut i32) -> Result { +pub fn futex(addr: usize, op: usize, val: usize, val2: usize, addr2: usize) -> Result { let target_physaddr = unsafe { let active_table = ActivePageTable::new(TableKind::User); - let virtual_address = VirtualAddress::new(addr as *mut i32 as usize); + let virtual_address = VirtualAddress::new(addr); + + if !crate::CurrentRmmArch::virt_is_valid(virtual_address) || crate::CurrentRmmArch::virt_kind(virtual_address) == TableKind::Kernel { + return Err(Error::new(EFAULT)); + } - // FIXME: Already validated in syscall/mod.rs active_table.translate(virtual_address).ok_or(Error::new(EFAULT))? }; match op { - FUTEX_WAIT => { - let timeout_opt = if val2 != 0 { - Some(validate_slice(val2 as *const TimeSpec, 1).map(|req| &req[0])?) - } else { + // TODO: FUTEX_WAIT_MULTIPLE? + FUTEX_WAIT | FUTEX_WAIT64 => { + let timeout_ptr = val2 as *const TimeSpec; + + let timeout_opt = if timeout_ptr.is_null() { None + } else { + let [timeout] = unsafe { *validate_array(timeout_ptr)? }; + Some(timeout) }; { @@ -69,7 +76,22 @@ pub fn futex(addr: &mut i32, op: usize, val: i32, val2: usize, addr2: *mut i32) Arc::clone(&context_lock) }; - if unsafe { intrinsics::atomic_load(addr) != val } { + // TODO: Is the implicit SeqCst ordering too strong here? + let (fetched, expected) = if op == FUTEX_WAIT { + // Must be aligned, otherwise it could cross a page boundary and mess up the + // (simpler) validation we did in the first place. + if addr % 4 != 0 { + return Err(Error::new(EINVAL)); + } + (u64::from(unsafe { intrinsics::atomic_load::(addr as *const u32) }), u64::from(val as u32)) + } else { + // op == FUTEX_WAIT64 + if addr % 8 != 0 { + return Err(Error::new(EINVAL)); + } + (unsafe { intrinsics::atomic_load::(addr as *const u64) }, val as u64) + }; + if fetched != expected { return Err(Error::new(EAGAIN)); } @@ -116,15 +138,17 @@ pub fn futex(addr: &mut i32, op: usize, val: i32, val2: usize, addr2: *mut i32) let mut futexes = futexes_mut(); let mut i = 0; - while i < futexes.len() && (woken as i32) < val { - if futexes[i].target_physaddr == target_physaddr { - if let Some(futex) = futexes.swap_remove_back(i) { - let mut context_guard = futex.context_lock.write(); - context_guard.unblock(); - woken += 1; - } - } else { + + // TODO: Use retain, once it allows the closure to tell it to stop iterating... + while i < futexes.len() && woken < val { + if futexes[i].target_physaddr != target_physaddr { i += 1; + continue; + } + if let Some(futex) = futexes.swap_remove_back(i) { + let mut context_guard = futex.context_lock.write(); + context_guard.unblock(); + woken += 1; } } } @@ -133,12 +157,13 @@ pub fn futex(addr: &mut i32, op: usize, val: i32, val2: usize, addr2: *mut i32) }, FUTEX_REQUEUE => { let addr2_physaddr = unsafe { + let addr2_virt = VirtualAddress::new(addr2); + + if !crate::CurrentRmmArch::virt_is_valid(addr2_virt) || crate::CurrentRmmArch::virt_kind(addr2_virt) == TableKind::Kernel { + return Err(Error::new(EFAULT)); + } + let active_table = ActivePageTable::new(TableKind::User); - - let addr2_safe = validate_slice_mut(addr2, 1).map(|addr2_safe| &mut addr2_safe[0])?; - let addr2_virt = VirtualAddress::new(addr2_safe as *mut i32 as usize); - - // FIXME: Already validated. active_table.translate(addr2_virt).ok_or(Error::new(EFAULT))? }; @@ -149,22 +174,21 @@ pub fn futex(addr: &mut i32, op: usize, val: i32, val2: usize, addr2: *mut i32) let mut futexes = futexes_mut(); let mut i = 0; - while i < futexes.len() && (woken as i32) < val { - if futexes[i].target_physaddr == target_physaddr { - if let Some(futex) = futexes.swap_remove_back(i) { - futex.context_lock.write().unblock(); - woken += 1; - } - } else { + while i < futexes.len() && woken < val { + if futexes[i].target_physaddr != target_physaddr { i += 1; } + if let Some(futex) = futexes.swap_remove_back(i) { + futex.context_lock.write().unblock(); + woken += 1; + } } while i < futexes.len() && requeued < val2 { - if futexes[i].target_physaddr == target_physaddr { - futexes[i].target_physaddr = addr2_physaddr; - requeued += 1; + if futexes[i].target_physaddr != target_physaddr { + i += 1; } - i += 1; + futexes[i].target_physaddr = addr2_physaddr; + requeued += 1; } } diff --git a/src/syscall/mod.rs b/src/syscall/mod.rs index 5792cd8..b90dd54 100644 --- a/src/syscall/mod.rs +++ b/src/syscall/mod.rs @@ -117,7 +117,7 @@ pub fn syscall(a: usize, b: usize, c: usize, d: usize, e: usize, f: usize, bp: u } ), SYS_CLOCK_GETTIME => clock_gettime(b, validate_slice_mut(c as *mut TimeSpec, 1).map(|time| &mut time[0])?), - SYS_FUTEX => futex(validate_slice_mut(b as *mut i32, 1).map(|uaddr| &mut uaddr[0])?, c, d as i32, e, f as *mut i32), + SYS_FUTEX => futex(b, c, d, e, f), SYS_GETPID => getpid().map(ContextId::into), SYS_GETPGID => getpgid(ContextId::from(b)).map(ContextId::into), SYS_GETPPID => getppid().map(ContextId::into), diff --git a/src/syscall/validate.rs b/src/syscall/validate.rs index ae99a95..883f01c 100644 --- a/src/syscall/validate.rs +++ b/src/syscall/validate.rs @@ -41,9 +41,26 @@ pub fn validate_slice(ptr: *const T, len: usize) -> Result<&'static [T]> { Ok(unsafe { slice::from_raw_parts(ptr, len) }) } } +/// Convert a pointer with fixed static length to a reference to an array, if valid. +// TODO: This is probably also quite unsafe, mainly because we have no idea unless we do very +// careful checking, that this upholds the rules that LLVM relies with shared references, namely +// that the value cannot change by others. Atomic volatile. +pub unsafe fn validate_array<'a, T, const N: usize>(ptr: *const T) -> Result<&'a [T; N]> { + validate(ptr as usize, mem::size_of::() * N, false)?; + Ok(&*ptr.cast::<[T; N]>()) +} +pub unsafe fn validate_array_mut<'a, T, const N: usize>(ptr: *mut T) -> Result<&'a mut [T; N]> { + validate(ptr as usize, mem::size_of::() * N, true)?; + Ok(&mut *ptr.cast::<[T; N]>()) +} /// Convert a pointer and length to slice, if valid -//TODO: Mark unsafe +// TODO: Mark unsafe +// +// FIXME: This is probably never ever safe, except under very special circumstances. Any &mut +// reference will allow LLVM to assume that nobody else will ever modify this value, which is +// certainly not the case for multithreaded userspace programs. Instead, we will want something +// like atomic volatile. pub fn validate_slice_mut(ptr: *mut T, len: usize) -> Result<&'static mut [T]> { if len == 0 { Ok(&mut []) diff --git a/syscall b/syscall index 52fcd23..fa5bc90 160000 --- a/syscall +++ b/syscall @@ -1 +1 @@ -Subproject commit 52fcd238db87354b6556eac9b05bb8ab2658426a +Subproject commit fa5bc90eadda43d72d3fe8e00415ec63a7cfc2ea From bcfd7b175e45e027eb176bf2acc5754da3f1a093 Mon Sep 17 00:00:00 2001 From: 4lDO2 <4lDO2@protonmail.com> Date: Thu, 8 Jul 2021 13:47:16 +0200 Subject: [PATCH 2/4] Update rmm. --- rmm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rmm b/rmm index 32caee3..b75c329 160000 --- a/rmm +++ b/rmm @@ -1 +1 @@ -Subproject commit 32caee3095e54a8d9181841fe48d9ea8d43f4e6c +Subproject commit b75c329a273d194e313bc36a1ceab5362fd5f8e2 From bbe6b4650ad9522886e2292acc1c800b39a4def6 Mon Sep 17 00:00:00 2001 From: 4lDO2 <4lDO2@protonmail.com> Date: Thu, 8 Jul 2021 16:08:02 +0200 Subject: [PATCH 3/4] Futex: check for lower-half addrs manually. --- rmm | 2 +- src/syscall/futex.rs | 16 ++++++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/rmm b/rmm index b75c329..6bc59e7 160000 --- a/rmm +++ b/rmm @@ -1 +1 @@ -Subproject commit b75c329a273d194e313bc36a1ceab5362fd5f8e2 +Subproject commit 6bc59e70131135984a41216c45e4dc3a6395a30a diff --git a/src/syscall/futex.rs b/src/syscall/futex.rs index a130e12..2600b2e 100644 --- a/src/syscall/futex.rs +++ b/src/syscall/futex.rs @@ -48,7 +48,13 @@ pub fn futex(addr: usize, op: usize, val: usize, val2: usize, addr2: usize) -> R let active_table = ActivePageTable::new(TableKind::User); let virtual_address = VirtualAddress::new(addr); - if !crate::CurrentRmmArch::virt_is_valid(virtual_address) || crate::CurrentRmmArch::virt_kind(virtual_address) == TableKind::Kernel { + if !crate::CurrentRmmArch::virt_is_valid(virtual_address) { + return Err(Error::new(EFAULT)); + } + // TODO: Use this all over the code, making sure that no user pointers that are higher half + // can get to the page table walking procedure. + #[cfg(any(target_arch = "x86_64", target_arch = "aarch64"))] + if virtual_address.data() & (1 << 63) == (1 << 63) { return Err(Error::new(EFAULT)); } @@ -159,7 +165,13 @@ pub fn futex(addr: usize, op: usize, val: usize, val2: usize, addr2: usize) -> R let addr2_physaddr = unsafe { let addr2_virt = VirtualAddress::new(addr2); - if !crate::CurrentRmmArch::virt_is_valid(addr2_virt) || crate::CurrentRmmArch::virt_kind(addr2_virt) == TableKind::Kernel { + if !crate::CurrentRmmArch::virt_is_valid(addr2_virt) { + return Err(Error::new(EFAULT)); + } + + // TODO + #[cfg(any(target_arch = "x86_64", target_arch = "aarch64"))] + if addr2_virt.data() & (1 << 63) == (1 << 63) { return Err(Error::new(EFAULT)); } From 6993c98e0f7fc5b0f94d551a149b4d366854cf4c Mon Sep 17 00:00:00 2001 From: 4lDO2 <4lDO2@protonmail.com> Date: Mon, 12 Jul 2021 00:10:48 +0200 Subject: [PATCH 4/4] Update rmm and syscall. --- rmm | 2 +- syscall | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/rmm b/rmm index 6bc59e7..37e88ff 160000 --- a/rmm +++ b/rmm @@ -1 +1 @@ -Subproject commit 6bc59e70131135984a41216c45e4dc3a6395a30a +Subproject commit 37e88ff4d3254b512e0bdf0faa5059ef08ac74dd diff --git a/syscall b/syscall index fa5bc90..841b5f4 160000 --- a/syscall +++ b/syscall @@ -1 +1 @@ -Subproject commit fa5bc90eadda43d72d3fe8e00415ec63a7cfc2ea +Subproject commit 841b5f42216782ce2aee6201c55b849ce5d7ab71