From 738554386bc15b81b17ba43059a7d5b31ce9114a Mon Sep 17 00:00:00 2001 From: WangRunji Date: Sun, 3 Mar 2019 01:27:30 +0800 Subject: [PATCH] introduce VMError to simplify EFAULT error handling --- crate/memory/src/lib.rs | 6 +++ crate/memory/src/memory_set/mod.rs | 28 ++++++++----- kernel/src/syscall/fs.rs | 65 ++++++++++-------------------- kernel/src/syscall/mod.rs | 7 ++++ kernel/src/syscall/net.rs | 13 ++---- kernel/src/syscall/proc.rs | 10 ++--- kernel/src/syscall/time.rs | 4 +- 7 files changed, 59 insertions(+), 74 deletions(-) diff --git a/crate/memory/src/lib.rs b/crate/memory/src/lib.rs index 96d3fd1..a162b20 100644 --- a/crate/memory/src/lib.rs +++ b/crate/memory/src/lib.rs @@ -14,3 +14,9 @@ mod addr; pub mod no_mmu; pub use crate::addr::*; + +pub enum VMError { + InvalidPtr +} + +pub type VMResult = Result; \ No newline at end of file diff --git a/crate/memory/src/memory_set/mod.rs b/crate/memory/src/memory_set/mod.rs index 35bf2bc..5ff5d87 100644 --- a/crate/memory/src/memory_set/mod.rs +++ b/crate/memory/src/memory_set/mod.rs @@ -192,29 +192,37 @@ impl MemorySet { } } /// Check the pointer is within the readable memory - pub fn check_ptr(&self, ptr: *const S) -> bool { - self.areas.iter().find(|area| area.check_ptr(ptr)).is_some() + pub fn check_ptr(&self, ptr: *const S) -> VMResult<()> { + self.areas.iter() + .find(|area| area.check_ptr(ptr)) + .map(|_|()).ok_or(VMError::InvalidPtr) } /// Check the pointer is within the writable memory - pub fn check_mut_ptr(&self, ptr: *mut S) -> bool { - self.areas.iter().find(|area| area.check_mut_ptr(ptr)).is_some() + pub fn check_mut_ptr(&self, ptr: *mut S) -> VMResult<()> { + self.areas.iter() + .find(|area| area.check_mut_ptr(ptr)) + .map(|_|()).ok_or(VMError::InvalidPtr) } /// Check the array is within the readable memory - pub fn check_array(&self, ptr: *const S, count: usize) -> bool { - self.areas.iter().find(|area| area.check_array(ptr, count)).is_some() + pub fn check_array(&self, ptr: *const S, count: usize) -> VMResult<()> { + self.areas.iter() + .find(|area| area.check_array(ptr, count)) + .map(|_|()).ok_or(VMError::InvalidPtr) } /// Check the array is within the writable memory - pub fn check_mut_array(&self, ptr: *mut S, count: usize) -> bool { - self.areas.iter().find(|area| area.check_mut_array(ptr, count)).is_some() + pub fn check_mut_array(&self, ptr: *mut S, count: usize) -> VMResult<()> { + self.areas.iter() + .find(|area| area.check_mut_array(ptr, count)) + .map(|_|()).ok_or(VMError::InvalidPtr) } /// Check the null-end C string is within the readable memory, and is valid. /// If so, clone it to a String. /// /// Unsafe: the page table must be active. - pub unsafe fn check_and_clone_cstr(&self, ptr: *const u8) -> Option { + pub unsafe fn check_and_clone_cstr(&self, ptr: *const u8) -> VMResult { self.areas.iter() .filter_map(|area| area.check_and_clone_cstr(ptr)) - .next() + .next().ok_or(VMError::InvalidPtr) } /// Find a free area with hint address `addr_hint` and length `len`. /// Return the start address of found free area. diff --git a/kernel/src/syscall/fs.rs b/kernel/src/syscall/fs.rs index 343295d..dd20e26 100644 --- a/kernel/src/syscall/fs.rs +++ b/kernel/src/syscall/fs.rs @@ -11,9 +11,7 @@ use super::net::*; pub fn sys_read(fd: usize, base: *mut u8, len: usize) -> SysResult { info!("read: fd: {}, base: {:?}, len: {:#x}", fd, base, len); let mut proc = process(); - if !proc.memory_set.check_mut_array(base, len) { - return Err(SysError::EFAULT); - } + proc.memory_set.check_mut_array(base, len)?; let slice = unsafe { slice::from_raw_parts_mut(base, len) }; let len = proc.get_file(fd)?.read(slice)?; Ok(len as isize) @@ -22,9 +20,7 @@ pub fn sys_read(fd: usize, base: *mut u8, len: usize) -> SysResult { pub fn sys_write(fd: usize, base: *const u8, len: usize) -> SysResult { info!("write: fd: {}, base: {:?}, len: {:#x}", fd, base, len); let mut proc = process(); - if !proc.memory_set.check_array(base, len) { - return Err(SysError::EFAULT); - } + proc.memory_set.check_array(base, len)?; match proc.files.get(&fd) { Some(FileLike::File(_)) => sys_write_file(&mut proc, fd, base, len), Some(FileLike::Socket(_)) => sys_write_socket(&mut proc, fd, base, len), @@ -65,8 +61,7 @@ pub fn sys_writev(fd: usize, iov_ptr: *const IoVec, iov_count: usize) -> SysResu pub fn sys_open(path: *const u8, flags: usize, mode: usize) -> SysResult { let mut proc = process(); - let path = unsafe { proc.memory_set.check_and_clone_cstr(path) } - .ok_or(SysError::EFAULT)?; + let path = unsafe { proc.memory_set.check_and_clone_cstr(path)? }; let flags = OpenFlags::from_bits_truncate(flags); info!("open: path: {:?}, flags: {:?}, mode: {:#o}", path, flags, mode); @@ -115,9 +110,7 @@ pub fn sys_close(fd: usize) -> SysResult { pub fn sys_getcwd(buf: *mut u8, len: usize) -> SysResult { info!("getcwd: buf: {:?}, len: {:#x}", buf, len); let mut proc = process(); - if !proc.memory_set.check_mut_array(buf, len) { - return Err(SysError::EFAULT); - } + proc.memory_set.check_mut_array(buf, len)?; if proc.cwd.len() + 1 > len { return Err(SysError::ERANGE); } @@ -135,9 +128,7 @@ pub fn sys_stat(path: *const u8, stat_ptr: *mut Stat) -> SysResult { pub fn sys_fstat(fd: usize, stat_ptr: *mut Stat) -> SysResult { info!("fstat: fd: {}", fd); let mut proc = process(); - if !proc.memory_set.check_mut_ptr(stat_ptr) { - return Err(SysError::EFAULT); - } + proc.memory_set.check_mut_ptr(stat_ptr)?; let file = proc.get_file(fd)?; let stat = Stat::from(file.metadata()?); // TODO: handle symlink @@ -147,11 +138,8 @@ pub fn sys_fstat(fd: usize, stat_ptr: *mut Stat) -> SysResult { pub fn sys_lstat(path: *const u8, stat_ptr: *mut Stat) -> SysResult { let mut proc = process(); - let path = unsafe { proc.memory_set.check_and_clone_cstr(path) } - .ok_or(SysError::EFAULT)?; - if !proc.memory_set.check_mut_ptr(stat_ptr) { - return Err(SysError::EFAULT); - } + let path = unsafe { proc.memory_set.check_and_clone_cstr(path)? }; + proc.memory_set.check_mut_ptr(stat_ptr)?; info!("lstat: path: {}", path); let inode = proc.lookup_inode(&path)?; @@ -189,8 +177,7 @@ pub fn sys_fdatasync(fd: usize) -> SysResult { pub fn sys_truncate(path: *const u8, len: usize) -> SysResult { let mut proc = process(); - let path = unsafe { proc.memory_set.check_and_clone_cstr(path) } - .ok_or(SysError::EFAULT)?; + let path = unsafe { proc.memory_set.check_and_clone_cstr(path)? }; info!("truncate: path: {:?}, len: {}", path, len); proc.lookup_inode(&path)?.resize(len)?; Ok(0) @@ -205,9 +192,7 @@ pub fn sys_ftruncate(fd: usize, len: usize) -> SysResult { pub fn sys_getdents64(fd: usize, buf: *mut LinuxDirent64, buf_size: usize) -> SysResult { info!("getdents64: fd: {}, ptr: {:?}, buf_size: {}", fd, buf, buf_size); let mut proc = process(); - if !proc.memory_set.check_mut_array(buf as *mut u8, buf_size) { - return Err(SysError::EFAULT); - } + proc.memory_set.check_mut_array(buf as *mut u8, buf_size)?; let file = proc.get_file(fd)?; let info = file.metadata()?; if info.type_ != FileType::Dir { @@ -239,8 +224,7 @@ pub fn sys_dup2(fd1: usize, fd2: usize) -> SysResult { pub fn sys_chdir(path: *const u8) -> SysResult { let mut proc = process(); - let path = unsafe { proc.memory_set.check_and_clone_cstr(path) } - .ok_or(SysError::EFAULT)?; + let path = unsafe { proc.memory_set.check_and_clone_cstr(path)? }; info!("chdir: path: {:?}", path); let inode = proc.lookup_inode(&path)?; @@ -255,10 +239,8 @@ pub fn sys_chdir(path: *const u8) -> SysResult { pub fn sys_rename(oldpath: *const u8, newpath: *const u8) -> SysResult { let mut proc = process(); - let oldpath = unsafe { proc.memory_set.check_and_clone_cstr(oldpath) } - .ok_or(SysError::EFAULT)?; - let newpath = unsafe { proc.memory_set.check_and_clone_cstr(newpath) } - .ok_or(SysError::EFAULT)?; + let oldpath = unsafe { proc.memory_set.check_and_clone_cstr(oldpath)? }; + let newpath = unsafe { proc.memory_set.check_and_clone_cstr(newpath)? }; info!("rename: oldpath: {:?}, newpath: {:?}", oldpath, newpath); let (old_dir_path, old_file_name) = split_path(&oldpath); @@ -276,8 +258,7 @@ pub fn sys_rename(oldpath: *const u8, newpath: *const u8) -> SysResult { pub fn sys_mkdir(path: *const u8, mode: usize) -> SysResult { let mut proc = process(); - let path = unsafe { proc.memory_set.check_and_clone_cstr(path) } - .ok_or(SysError::EFAULT)?; + let path = unsafe { proc.memory_set.check_and_clone_cstr(path)? }; // TODO: check pathname info!("mkdir: path: {:?}, mode: {:#o}", path, mode); @@ -292,10 +273,8 @@ pub fn sys_mkdir(path: *const u8, mode: usize) -> SysResult { pub fn sys_link(oldpath: *const u8, newpath: *const u8) -> SysResult { let mut proc = process(); - let oldpath = unsafe { proc.memory_set.check_and_clone_cstr(oldpath) } - .ok_or(SysError::EFAULT)?; - let newpath = unsafe { proc.memory_set.check_and_clone_cstr(newpath) } - .ok_or(SysError::EFAULT)?; + let oldpath = unsafe { proc.memory_set.check_and_clone_cstr(oldpath)? }; + let newpath = unsafe { proc.memory_set.check_and_clone_cstr(newpath)? }; info!("link: oldpath: {:?}, newpath: {:?}", oldpath, newpath); let (new_dir_path, new_file_name) = split_path(&newpath); @@ -307,8 +286,7 @@ pub fn sys_link(oldpath: *const u8, newpath: *const u8) -> SysResult { pub fn sys_unlink(path: *const u8) -> SysResult { let mut proc = process(); - let path = unsafe { proc.memory_set.check_and_clone_cstr(path) } - .ok_or(SysError::EFAULT)?; + let path = unsafe { proc.memory_set.check_and_clone_cstr(path)? }; info!("unlink: path: {:?}", path); let (dir_path, file_name) = split_path(&path); @@ -597,15 +575,14 @@ struct IoVecs(Vec<&'static mut [u8]>); impl IoVecs { fn check_and_new(iov_ptr: *const IoVec, iov_count: usize, vm: &MemorySet, readv: bool) -> Result { - if !vm.check_array(iov_ptr, iov_count) { - return Err(SysError::EFAULT); - } + vm.check_array(iov_ptr, iov_count)?; let iovs = unsafe { slice::from_raw_parts(iov_ptr, iov_count) }.to_vec(); // check all bufs in iov for iov in iovs.iter() { - if readv && !vm.check_mut_array(iov.base, iov.len as usize) - || !readv && !vm.check_array(iov.base, iov.len as usize) { - return Err(SysError::EFAULT); + if readv { + vm.check_mut_array(iov.base, iov.len as usize)?; + } else { + vm.check_array(iov.base, iov.len as usize)?; } } let slices = iovs.iter().map(|iov| unsafe { slice::from_raw_parts_mut(iov.base, iov.len as usize) }).collect(); diff --git a/kernel/src/syscall/mod.rs b/kernel/src/syscall/mod.rs index 6bb6bc5..80f6d40 100644 --- a/kernel/src/syscall/mod.rs +++ b/kernel/src/syscall/mod.rs @@ -4,6 +4,7 @@ use alloc::{string::String, sync::Arc, vec::Vec}; use core::{slice, str, fmt}; use bitflags::bitflags; +use rcore_memory::VMError; use rcore_fs::vfs::{FileType, FsError, INode, Metadata}; use spin::{Mutex, MutexGuard}; @@ -270,3 +271,9 @@ impl fmt::Display for SysError { ) } } + +impl From for SysError { + fn from(_: VMError) -> Self { + SysError::EFAULT + } +} diff --git a/kernel/src/syscall/net.rs b/kernel/src/syscall/net.rs index 1529482..c28db71 100644 --- a/kernel/src/syscall/net.rs +++ b/kernel/src/syscall/net.rs @@ -139,9 +139,7 @@ pub fn sys_connect(fd: usize, addr: *const u8, addrlen: usize) -> SysResult { ); let mut proc = process(); - if !proc.memory_set.check_ptr(addr) { - return Err(SysError::EFAULT); - } + proc.memory_set.check_ptr(addr)?; let mut dest = None; let mut port = 0; @@ -234,13 +232,8 @@ pub fn sys_sendto( fd, buffer, len, addr, addr_len ); let mut proc = process(); - if !proc.memory_set.check_ptr(addr) { - return Err(SysError::EFAULT); - } - - if !proc.memory_set.check_array(buffer, len) { - return Err(SysError::EINVAL); - } + proc.memory_set.check_ptr(addr)?; + proc.memory_set.check_array(buffer, len)?; // little hack: kick it forward let iface = &mut *NET_DRIVERS.lock()[0]; diff --git a/kernel/src/syscall/proc.rs b/kernel/src/syscall/proc.rs index de43834..7c42037 100644 --- a/kernel/src/syscall/proc.rs +++ b/kernel/src/syscall/proc.rs @@ -13,9 +13,7 @@ pub fn sys_fork(tf: &TrapFrame) -> SysResult { /// Wait the process exit. /// Return the PID. Store exit code to `code` if it's not null. pub fn sys_wait(pid: usize, code: *mut i32) -> SysResult { - if !process().memory_set.check_mut_ptr(code) { - return Err(SysError::EFAULT); - } + process().memory_set.check_mut_ptr(code)?; loop { use alloc::vec; let wait_procs = match pid { @@ -53,8 +51,7 @@ pub fn sys_wait(pid: usize, code: *mut i32) -> SysResult { pub fn sys_exec(name: *const u8, argc: usize, argv: *const *const u8, tf: &mut TrapFrame) -> SysResult { let proc = process(); let name = if name.is_null() { String::from("") } else { - unsafe { proc.memory_set.check_and_clone_cstr(name) } - .ok_or(SysError::EFAULT)? + unsafe { proc.memory_set.check_and_clone_cstr(name)? } }; if argc <= 0 { return Err(SysError::EINVAL); @@ -63,8 +60,7 @@ pub fn sys_exec(name: *const u8, argc: usize, argv: *const *const u8, tf: &mut T let mut args = Vec::new(); unsafe { for &ptr in slice::from_raw_parts(argv, argc) { - let arg = proc.memory_set.check_and_clone_cstr(ptr) - .ok_or(SysError::EFAULT)?; + let arg = proc.memory_set.check_and_clone_cstr(ptr)?; args.push(arg); } } diff --git a/kernel/src/syscall/time.rs b/kernel/src/syscall/time.rs index dd91554..e686fe4 100644 --- a/kernel/src/syscall/time.rs +++ b/kernel/src/syscall/time.rs @@ -10,9 +10,7 @@ pub fn sys_time(time: *mut u64) -> SysResult { let t = unsafe { crate::trap::TICK }; if time as usize != 0 { let mut proc = process(); - if !proc.memory_set.check_mut_ptr(time) { - return Err(SysError::EFAULT); - } + proc.memory_set.check_mut_ptr(time)?; unsafe { time.write(t as u64); }