Fix two bugs of ioctl

1. Add length of the argument to the ioctl ocall to guard sgx sdk to do
the copy.
2. Handle non-zero return value of ioctl.
This commit is contained in:
He Sun 2020-05-11 15:11:44 +08:00
parent 6d72e10fc1
commit 9252a1a993
12 changed files with 125 additions and 35 deletions

@ -162,5 +162,12 @@ enclave {
void occlum_ocall_print_log(uint32_t level, [in, string] const char* msg);
void occlum_ocall_flush_log(void);
int occlum_ocall_ioctl(
int fd,
int request,
[in, out, size=len] void *arg,
size_t len
) propagate_errno;
};
};

@ -11,7 +11,7 @@ use util::sgx::*;
pub struct DevSgx;
impl File for DevSgx {
fn ioctl(&self, cmd: &mut IoctlCmd) -> Result<()> {
fn ioctl(&self, cmd: &mut IoctlCmd) -> Result<i32> {
let nonbuiltin_cmd = match cmd {
IoctlCmd::NonBuiltin(nonbuiltin_cmd) => nonbuiltin_cmd,
_ => return_errno!(EINVAL, "unknown ioctl cmd for /dev/sgx"),
@ -92,7 +92,7 @@ impl File for DevSgx {
return_errno!(ENOSYS, "unknown ioctl cmd for /dev/sgx");
}
}
Ok(())
Ok(0)
}
fn as_any(&self) -> &dyn Any {

@ -67,7 +67,7 @@ pub trait File: Debug + Sync + Send + Any {
Ok(())
}
fn ioctl(&self, cmd: &mut IoctlCmd) -> Result<()> {
fn ioctl(&self, cmd: &mut IoctlCmd) -> Result<i32> {
return_op_unsupported_error!("ioctl")
}

@ -112,6 +112,15 @@ macro_rules! impl_ioctl_cmds {
}
}
pub fn arg_len(&self) -> usize {
match self {
$(
IoctlCmd::$ioctl_name(_) => get_arg_len!($($ioctl_type_tt)*),
)*
IoctlCmd::NonBuiltin(inner) => inner.arg_len(),
}
}
pub fn cmd_num(&self) -> u32 {
match self {
$(
@ -142,6 +151,18 @@ macro_rules! get_arg_type {
};
}
macro_rules! get_arg_len {
(()) => {
0
};
(mut $type: ty) => {
std::mem::size_of::<$type>()
};
($type: ty) => {
std::mem::size_of::<$type>()
};
}
macro_rules! get_arg {
((), $arg_ptr: ident) => {
()

@ -45,7 +45,7 @@ impl_ioctl_nums_and_cmds! {
/// Sanity checks are mostly useful when the argument values are returned from
/// the untrusted host OS.
impl<'a> IoctlCmd<'a> {
pub fn validate_arg_val(&self) -> Result<()> {
pub fn validate_arg_and_ret_vals(&self, ret: i32) -> Result<()> {
match self {
IoctlCmd::TIOCGWINSZ(winsize_ref) => {
// ws_row and ws_col are usually not zeros
@ -63,12 +63,27 @@ impl<'a> IoctlCmd<'a> {
}
_ => {}
}
// Current ioctl commands all return zero
if ret != 0 {
return_errno!(EINVAL, "return value should be zero");
}
Ok(())
}
}
pub fn do_ioctl(fd: FileDesc, cmd: &mut IoctlCmd) -> Result<()> {
pub fn do_ioctl(fd: FileDesc, cmd: &mut IoctlCmd) -> Result<i32> {
debug!("ioctl: fd: {}, cmd: {:?}", fd, cmd);
let file_ref = current!().file(fd)?;
file_ref.ioctl(cmd)
}
extern "C" {
pub fn occlum_ocall_ioctl(
ret: *mut i32,
fd: c_int,
request: c_int,
arg: *mut c_void,
len: size_t,
) -> sgx_status_t;
}

@ -65,6 +65,10 @@ impl<'a> NonBuiltinIoctlCmd<'a> {
.as_ref()
.map_or(std::ptr::null(), |arg_slice| arg_slice.as_ptr())
}
pub fn arg_len(&self) -> usize {
self.cmd_num.arg_size()
}
}
#[derive(Debug, Copy, Clone, PartialEq)]

@ -13,7 +13,9 @@ pub use self::fcntl::{do_fcntl, FcntlCmd};
pub use self::file_flags::{AccessMode, CreationFlags, StatusFlags};
pub use self::flock::{Flock, FlockType};
pub use self::fsync::{do_fdatasync, do_fsync};
pub use self::ioctl::{do_ioctl, IoctlCmd, StructuredIoctlArgType, StructuredIoctlNum};
pub use self::ioctl::{
do_ioctl, occlum_ocall_ioctl, IoctlCmd, StructuredIoctlArgType, StructuredIoctlNum,
};
pub use self::link::do_link;
pub use self::lseek::do_lseek;
pub use self::mkdir::do_mkdir;

@ -13,7 +13,9 @@ use untrusted::{SliceAsMutPtrAndLen, SliceAsPtrAndLen};
pub use self::dev_fs::AsDevRandom;
pub use self::event_file::{AsEvent, EventFile};
pub use self::file::{File, FileRef};
pub use self::file_ops::{AccessMode, CreationFlags, FileMode, Stat, StatusFlags};
pub use self::file_ops::{
occlum_ocall_ioctl, AccessMode, CreationFlags, FileMode, Stat, StatusFlags,
};
pub use self::file_ops::{Flock, FlockType};
pub use self::file_ops::{IoctlCmd, StructuredIoctlArgType, StructuredIoctlNum};
pub use self::file_table::{FileDesc, FileTable};

@ -169,7 +169,7 @@ impl File for StdoutFile {
Ok(())
}
fn ioctl(&self, cmd: &mut IoctlCmd) -> Result<()> {
fn ioctl(&self, cmd: &mut IoctlCmd) -> Result<i32> {
let can_delegate_to_host = match cmd {
IoctlCmd::TIOCGWINSZ(_) => true,
IoctlCmd::TIOCSWINSZ(_) => true,
@ -180,16 +180,24 @@ impl File for StdoutFile {
}
let cmd_bits = cmd.cmd_num() as c_int;
let cmd_arg_ptr = cmd.arg_ptr() as *mut c_int;
let cmd_arg_ptr = cmd.arg_ptr() as *mut c_void;
let host_stdout_fd = self.get_host_fd() as i32;
try_libc!(libc::ocall::ioctl_arg1(
let cmd_arg_len = cmd.arg_len();
let ret = try_libc!({
let mut retval: i32 = 0;
let status = occlum_ocall_ioctl(
&mut retval as *mut i32,
host_stdout_fd,
cmd_bits,
cmd_arg_ptr
));
cmd.validate_arg_val()?;
cmd_arg_ptr,
cmd_arg_len,
);
assert!(status == sgx_status_t::SGX_SUCCESS);
retval
});
cmd.validate_arg_and_ret_vals(ret)?;
Ok(())
Ok(ret)
}
fn as_any(&self) -> &dyn Any {
@ -310,7 +318,7 @@ impl File for StdinFile {
})
}
fn ioctl(&self, cmd: &mut IoctlCmd) -> Result<()> {
fn ioctl(&self, cmd: &mut IoctlCmd) -> Result<i32> {
let can_delegate_to_host = match cmd {
IoctlCmd::TIOCGWINSZ(_) => true,
IoctlCmd::TIOCSWINSZ(_) => true,
@ -321,16 +329,24 @@ impl File for StdinFile {
}
let cmd_bits = cmd.cmd_num() as c_int;
let cmd_arg_ptr = cmd.arg_ptr() as *mut c_int;
let cmd_arg_ptr = cmd.arg_ptr() as *mut c_void;
let host_stdin_fd = self.get_host_fd() as i32;
try_libc!(libc::ocall::ioctl_arg1(
let cmd_arg_len = cmd.arg_len();
let ret = try_libc!({
let mut retval: i32 = 0;
let status = occlum_ocall_ioctl(
&mut retval as *mut i32,
host_stdin_fd,
cmd_bits,
cmd_arg_ptr
));
cmd.validate_arg_val()?;
cmd_arg_ptr,
cmd_arg_len,
);
assert!(status == sgx_status_t::SGX_SUCCESS);
retval
});
cmd.validate_arg_and_ret_vals(ret)?;
Ok(())
Ok(ret)
}
fn as_any(&self) -> &dyn Any {

@ -3,7 +3,7 @@ use super::*;
mod recv;
mod send;
use fs::{AccessMode, CreationFlags, File, FileRef, IoctlCmd, StatusFlags};
use fs::{occlum_ocall_ioctl, AccessMode, CreationFlags, File, FileRef, IoctlCmd, StatusFlags};
use std::any::Any;
use std::io::{Read, Seek, SeekFrom, Write};
@ -106,13 +106,24 @@ impl File for SocketFile {
return_errno!(ESPIPE, "Socket does not support seek")
}
fn ioctl(&self, cmd: &mut IoctlCmd) -> Result<()> {
fn ioctl(&self, cmd: &mut IoctlCmd) -> Result<i32> {
let cmd_num = cmd.cmd_num() as c_int;
let cmd_arg_ptr = cmd.arg_ptr() as *mut c_int;
try_libc!(libc::ocall::ioctl_arg1(self.fd(), cmd_num, cmd_arg_ptr));
let cmd_arg_ptr = cmd.arg_ptr() as *mut c_void;
let ret = try_libc!({
let mut retval: i32 = 0;
let status = occlum_ocall_ioctl(
&mut retval as *mut i32,
self.fd(),
cmd_num,
cmd_arg_ptr,
cmd.arg_len(),
);
assert!(status == sgx_status_t::SGX_SUCCESS);
retval
});
// FIXME: add sanity checks for results returned for socket-related ioctls
cmd.validate_arg_val()?;
Ok(())
cmd.validate_arg_and_ret_vals(ret)?;
Ok(ret)
}
fn get_access_mode(&self) -> Result<AccessMode> {

@ -80,7 +80,7 @@ impl File for UnixSocketFile {
})
}
fn ioctl(&self, cmd: &mut IoctlCmd) -> Result<()> {
fn ioctl(&self, cmd: &mut IoctlCmd) -> Result<i32> {
let mut inner = self.inner.lock().unwrap();
inner.ioctl(cmd)
}
@ -260,7 +260,7 @@ impl UnixSocket {
Ok((r, w, false))
}
pub fn ioctl(&self, cmd: &mut IoctlCmd) -> Result<()> {
pub fn ioctl(&self, cmd: &mut IoctlCmd) -> Result<i32> {
match cmd {
IoctlCmd::FIONREAD(arg) => {
let bytes_to_read = self
@ -272,7 +272,7 @@ impl UnixSocket {
}
_ => return_errno!(EINVAL, "unknown ioctl cmd for unix socket"),
}
Ok(())
Ok(0)
}
fn channel(&self) -> Result<&Channel> {

@ -1,6 +1,8 @@
#include <unistd.h>
#include "ocalls.h"
#include <errno.h>
#include <unistd.h>
#include <sys/eventfd.h>
#include <sys/ioctl.h>
void occlum_ocall_sync(void) {
sync();
@ -9,3 +11,13 @@ void occlum_ocall_sync(void) {
int occlum_ocall_eventfd(unsigned int initval, int flags) {
return eventfd(initval, flags);
}
int occlum_ocall_ioctl(int fd, int request, void *arg, size_t len) {
if (((arg == NULL) ^ (len == 0)) == 1) {
printf("invalid ioctl parameters\n");
errno = EINVAL;
return -1;
}
return ioctl(fd, request, arg);
}