general/occlum
4
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Refine check for pointers from user space and outside enclave

Browse Source 
Implement the check functions that are empty before and do some
adjustment where they are called.
This commit is contained in:
He Sun 2020-04-27 22:22:38 +08:00 committed by Tate, Hongliang Tian
parent 614ec88242
commit bbb52f6990
5 changed files with 108 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
src/libos/src/entry.rs
View File

@ -135,7 +135,9 @@ fn parse_log_level(level_chars: *const c_char) -> Result<LevelFilter> {
}
let level_string = {
let level_cstring = clone_cstring_safely(level_chars)?;
// level_chars has been guaranteed to be inside enclave
// and null terminated by ECall
let level_cstring = CString::from(unsafe { CStr::from_ptr(level_chars) });
level_cstring
.into_string()
.map_err(|e| errno!(EINVAL, "log_level contains valid utf-8 data"))?
@ -158,7 +160,13 @@ fn parse_arguments(
host_stdio_fds: *const HostStdioFds,
) -> Result<(PathBuf, Vec<CString>, HostStdioFds)> {
let path_buf = {
let path_cstring = clone_cstring_safely(path_ptr)?;
if path_ptr.is_null() {
return_errno!(EINVAL, "empty path");
}
// path_ptr has been guaranteed to be inside enclave
// and null terminated by ECall
let path_cstring = CString::from(unsafe { CStr::from_ptr(path_ptr) });
let path_string = path_cstring
.into_string()
.map_err(|e| errno!(EINVAL, "path contains valid utf-8 data"))?;

4
src/libos/src/fs/syscalls.rs
View File

@ -82,7 +82,7 @@ pub fn do_writev(fd: FileDesc, iov: *const iovec_t, count: i32) -> Result<isize>
count as usize
};
from_user::check_array(iov, count);
from_user::check_array(iov, count)?;
let bufs_vec = {
let mut bufs_vec = Vec::with_capacity(count);
for iov_i in 0..count {
@ -107,7 +107,7 @@ pub fn do_readv(fd: FileDesc, iov: *mut iovec_t, count: i32) -> Result<isize> {
count as usize
};
from_user::check_array(iov, count);
from_user::check_array(iov, count)?;
let mut bufs_vec = {
let mut bufs_vec = Vec::with_capacity(count);
for iov_i in 0..count {

6
src/libos/src/net/syscalls.rs
View File

@ -170,7 +170,11 @@ pub fn do_select(
}
pub fn do_poll(fds: *mut libc::pollfd, nfds: libc::nfds_t, timeout: c_int) -> Result<isize> {
from_user::check_mut_array(fds, nfds as usize)?;
// It behaves like sleep when fds is null and nfds is zero.
if !fds.is_null() || nfds != 0 {
from_user::check_mut_array(fds, nfds as usize)?;
}
let polls = unsafe { std::slice::from_raw_parts_mut(fds, nfds as usize) };
let n = io_multiplexing::do_poll(polls, timeout)?;

42
src/libos/src/syscall/mod.rs
View File

@ -753,13 +753,24 @@ fn do_connect(fd: c_int, addr: *const libc::sockaddr, addr_len: libc::socklen_t)
"connect: fd: {}, addr: {:?}, addr_len: {}",
fd, addr, addr_len
);
// For SOCK_DGRAM sockets not initiated in connection-mode,
// if address is a null address for the protocol,
// the socket's peer address shall be reset.
let need_check: bool = !addr.is_null();
if need_check {
check_array(addr as *const u8, addr_len as usize)?;
}
let file_ref = current!().file(fd as FileDesc)?;
if let Ok(socket) = file_ref.as_socket() {
if need_check {
check_ptr(addr as *const libc::sockaddr_in)?;
}
let ret = try_libc!(libc::ocall::connect(socket.fd(), addr, addr_len));
Ok(ret as isize)
} else if let Ok(unix_socket) = file_ref.as_unix_socket() {
let addr = addr as *const libc::sockaddr_un;
check_ptr(addr)?; // TODO: check addr_len
check_ptr(addr)?;
let path = clone_cstring_safely(unsafe { (&*addr).sun_path.as_ptr() })?
.to_string_lossy()
.into_owned();
@ -788,9 +799,21 @@ fn do_accept4(
"accept4: fd: {}, addr: {:?}, addr_len: {:?}, flags: {:#x}",
fd, addr, addr_len, flags
);
let need_check: bool = !addr.is_null();
if addr.is_null() ^ addr_len.is_null() {
return_errno!(EINVAL, "addr and ddr_len should be both null");
}
if need_check {
check_mut_array(addr as *mut u8, unsafe { *addr_len } as usize)?;
}
let file_ref = current!().file(fd as FileDesc)?;
if let Ok(socket) = file_ref.as_socket() {
let socket = file_ref.as_socket()?;
if need_check {
check_mut_ptr(addr as *mut libc::sockaddr_in)?;
}
let new_socket = socket.accept(addr, addr_len, flags)?;
let new_file_ref: Arc<Box<dyn File>> = Arc::new(Box::new(new_socket));
@ -799,8 +822,10 @@ fn do_accept4(
Ok(new_fd as isize)
} else if let Ok(unix_socket) = file_ref.as_unix_socket() {
let addr = addr as *mut libc::sockaddr_un;
check_mut_ptr(addr)?; // TODO: check addr_len
if need_check {
check_mut_ptr(addr)?;
}
// TODO: handle addr
let new_socket = unix_socket.accept()?;
let new_file_ref: Arc<Box<dyn File>> = Arc::new(Box::new(new_socket));
let new_fd = current!().add_file(new_file_ref, false);
@ -824,14 +849,19 @@ fn do_shutdown(fd: c_int, how: c_int) -> Result<isize> {
fn do_bind(fd: c_int, addr: *const libc::sockaddr, addr_len: libc::socklen_t) -> Result<isize> {
debug!("bind: fd: {}, addr: {:?}, addr_len: {}", fd, addr, addr_len);
if addr.is_null() && addr_len == 0 {
return_errno!(EINVAL, "no address is specified");
}
check_array(addr as *const u8, addr_len as usize)?;
let file_ref = current!().file(fd as FileDesc)?;
if let Ok(socket) = file_ref.as_socket() {
check_ptr(addr)?; // TODO: check addr_len
check_ptr(addr as *const libc::sockaddr_in)?;
let ret = try_libc!(libc::ocall::bind(socket.fd(), addr, addr_len));
Ok(ret as isize)
} else if let Ok(unix_socket) = file_ref.as_unix_socket() {
let addr = addr as *const libc::sockaddr_un;
check_ptr(addr)?; // TODO: check addr_len
check_ptr(addr)?;
let path = clone_cstring_safely(unsafe { (&*addr).sun_path.as_ptr() })?
.to_string_lossy()
.into_owned();

77
src/libos/src/util/mem_util.rs
View File

@ -1,6 +1,8 @@
use super::*;
use std::ffi::{CStr, CString};
use std::mem::size_of;
use std::ptr;
use vm::VMRange;
/// Memory utilities that deals with primitive types passed from user process
/// running inside enclave
@ -9,38 +11,45 @@ pub mod from_user {
/// Check the user pointer is within the readable memory range of the user process
pub fn check_ptr<T>(user_ptr: *const T) -> Result<()> {
if user_ptr.is_null() {
return_errno!(EINVAL, "Address 0 is invalid");
if !is_inside_user_space(user_ptr as *const u8, size_of::<T>()) {
return_errno!(EFAULT, "pointer is not in the user space");
}
Ok(())
}
/// Check the mutable user pointer is within the writable memory of the user process
pub fn check_mut_ptr<T>(user_ptr: *mut T) -> Result<()> {
if user_ptr.is_null() {
return_errno!(EINVAL, "Address 0 is invalid");
}
Ok(())
// The user space is both readable and writable on SGX1.
// TODO: Fine-tune the checking on SGX2.
check_ptr(user_ptr)
}
/// Check the readonly array is within the readable memory of the user process
pub fn check_array<T>(user_buf: *const T, count: usize) -> Result<()> {
check_ptr(user_buf);
if !is_inside_user_space(user_buf as *const u8, count * size_of::<T>()) {
return_errno!(EFAULT, "the whole buffer is not in the user space");
}
Ok(())
}
/// Check the mutable array is within the writable memory of the user process
pub fn check_mut_array<T>(user_buf: *mut T, count: usize) -> Result<()> {
check_mut_ptr(user_buf);
Ok(())
// The user space is both readable and writable on SGX1.
// TODO: Fine-tune the checking on SGX2.
check_array(user_buf, count)
}
/// Clone a C-string from the user process safely
pub fn clone_cstring_safely(out_ptr: *const c_char) -> Result<CString> {
check_ptr(out_ptr)?;
// TODO: using from_ptr directly is not safe
if out_ptr.is_null() {
return_errno!(EINVAL, "NULL address is invalid");
}
let cstr = unsafe { CStr::from_ptr(out_ptr) };
let cstring = CString::from(cstr);
if !is_inside_user_space(out_ptr as *const u8, cstring.as_bytes().len()) {
return_errno!(EFAULT, "the whole buffer is not in the user space");
}
Ok(cstring)
}
@ -55,13 +64,13 @@ pub mod from_user {
let mut user_ptr = user_ptr;
loop {
check_ptr(user_ptr);
check_ptr(user_ptr)?;
let cstr_ptr = {
let cstr_ptr = unsafe { *user_ptr };
if cstr_ptr == ptr::null() {
break;
}
check_ptr(cstr_ptr);
cstr_ptr
};
let cstring = clone_cstring_safely(cstr_ptr)?;
@ -71,6 +80,20 @@ pub mod from_user {
}
Ok(cstrings)
}
/// Check if the provided buffer is within the current user space
///
/// addr: the start address
/// len: the length in byte
fn is_inside_user_space(addr: *const u8, len: usize) -> bool {
let current = current!();
let current_vm = current.vm().lock().unwrap();
let user_range = current_vm.get_process_range();
let ur_start = user_range.start();
let ur_end = user_range.end();
let addr_start = addr as usize;
addr_start >= ur_start && addr_start < ur_end && ur_end - addr_start >= len
}
}
/// Memory utilities that deals with primitive types passed from outside the enclave
@ -78,34 +101,44 @@ pub mod from_untrusted {
use super::*;
/// Check the untrusted pointer is outside the enclave
// TODO: implement this!
pub fn check_ptr<T>(out_ptr: *const T) -> Result<()> {
if !sgx_trts::trts::rsgx_raw_is_outside_enclave(out_ptr as *const u8, size_of::<T>()) {
return_errno!(EFAULT, "the pointer is not outside enclave");
}
Ok(())
}
/// Check the untrusted array is outside the enclave
// TODO: implement this!
pub fn check_array<T>(out_ptr: *const T, count: usize) -> Result<()> {
if !sgx_trts::trts::rsgx_raw_is_outside_enclave(
out_ptr as *const u8,
count * size_of::<T>(),
) {
return_errno!(EFAULT, "the whole buffer is not outside enclave");
}
Ok(())
}
/// Clone a C-string from outside the enclave
// TODO: strict check!
pub fn clone_cstring_safely(out_ptr: *const c_char) -> Result<CString> {
check_ptr(out_ptr)?;
if out_ptr.is_null() {
return_errno!(EINVAL, "null ptr");
return_errno!(EINVAL, "NULL address is invalid");
}
// TODO: using from_ptr directly is not safe
let cstr = unsafe { CStr::from_ptr(out_ptr) };
let cstring = CString::from(cstr);
if !sgx_trts::trts::rsgx_raw_is_outside_enclave(
out_ptr as *const u8,
cstring.as_bytes().len(),
) {
return_errno!(EFAULT, "the string is not outside enclave");
}
Ok(cstring)
}
/// Clone a C-string array (const char*[]) from outside the enclave
///
/// This array must be ended with a NULL pointer.
// TODO: strict check!
pub fn clone_cstrings_safely(out_ptr: *const *const c_char) -> Result<Vec<CString>> {
let mut cstrings = Vec::new();
if out_ptr == ptr::null() {
@ -114,13 +147,13 @@ pub mod from_untrusted {
let mut out_ptr = out_ptr;
loop {
check_ptr(out_ptr);
check_ptr(out_ptr)?;
let cstr_ptr = {
let cstr_ptr = unsafe { *out_ptr };
if cstr_ptr == ptr::null() {
break;
}
check_ptr(cstr_ptr);
cstr_ptr
};
let cstring = clone_cstring_safely(cstr_ptr)?;