Add readv and writev syscalls for SGX #369

lkatalin · 2020-03-26T13:54:27Z

This is a draft. In particular, it needs some help with casting pointers in a safe and correct way.

Resolves #375 and resolves #376

enarx-keep-sgx-shim/src/handler.rs

npmccallum · 2020-03-26T15:05:11Z

enarx-keep-sgx-shim/src/handler.rs

+        let iovec = unsafe { from_raw_parts_mut(iovec as *mut Iovec, iovcnt) }; // ???
+        let mut bufsize: usize = 0;
+        for i in 0..iovcnt {
+            bufsize += iovec[i].size;
+        }


Suggested change

let iovec = unsafe { from_raw_parts_mut(iovec as *mut Iovec, iovcnt) }; // ???

let mut bufsize: usize = 0;

for i in 0..iovcnt {

bufsize += iovec[i].size;

}

let bufsize = trusted.iter().fold(0, |a, e| a + e.size);

npmccallum · 2020-03-26T15:06:00Z

enarx-keep-sgx-shim/src/handler.rs

+        }
+
+        // Allocate some unencrypted memory.
+        let size = bufsize + size_of::<Iovec>() * iovcnt;


Suggested change

let size = bufsize + size_of::<Iovec>() * iovcnt;

let size = bufsize + size_of::<Iovec>() * trusted.len();

npmccallum · 2020-03-26T15:07:00Z

enarx-keep-sgx-shim/src/handler.rs

+        };
+
+        // Cast beginning of allocated memory as iovec slice of length iovcnt.
+        let map = unsafe { from_raw_parts_mut(map as *mut Iovec, iovcnt) };


Suggested change

let map = unsafe { from_raw_parts_mut(map as *mut Iovec, iovcnt) };

let untrusted = unsafe { from_raw_parts_mut(map as *mut Iovec, iovcnt) };

The general strategy with my naming here is to always communicate to future readers whether the types are trusted or not. We want to avoid accidents...

npmccallum · 2020-03-26T15:09:05Z

enarx-keep-sgx-shim/src/handler.rs

+        let map = unsafe { from_raw_parts_mut(map as *mut Iovec, iovcnt) };
+
+        // Set pointers in unencrypted iovec slice to use the rest of the allocated memory.
+        let mut chunk = map.as_ptr() as usize + size_of::<Iovec>() * iovcnt; // ??? May be unsafe or illegal as per from_raw_parts_mut rules


I'm not sure I understand the question here.

I was thinking of whether I'm allowed to do map.as_ptr() due to: "The memory referenced by the returned slice must not be accessed through any other pointer (not derived from the return value) for the duration of lifetime 'a. Both read and write accesses are forbidden." from https://doc.rust-lang.org/core/slice/fn.from_raw_parts_mut.html. But since the pointer is derived from the original 'map', maybe it's okay.

@lkatalin What it means in this case is don't access the memory in this region. You can still use the base pointer to calculate a new pointer to memory after the region without harm.

@npmccallum Ah okay, thanks as usual for the insights.

npmccallum · 2020-03-26T15:18:28Z

enarx-keep-sgx-shim/src/handler.rs

+
+        // Set pointers in unencrypted iovec slice to use the rest of the allocated memory.
+        let mut chunk = map.as_ptr() as usize + size_of::<Iovec>() * iovcnt; // ??? May be unsafe or illegal as per from_raw_parts_mut rules
+        for i in 0..iovcnt {


Suggested change

for i in 0..iovcnt {

for (t, u) in trusted.iter().zip(untrusted.iter()) {

npmccallum · 2020-03-26T15:20:15Z

enarx-keep-sgx-shim/src/handler.rs

+            map[i].base = chunk as *mut u8;
+            chunk = chunk + iovec[i].size;


Suggested change

map[i].base = chunk as *mut u8;

chunk = chunk + iovec[i].size;

u.base = chunk;

u.size = t.size;

chunk = chunk.add(t.size);

npmccallum · 2020-03-26T15:24:08Z

enarx-keep-sgx-shim/src/handler.rs

+                0,
+                0,
+            ); // ???
+            self.ufree(map.as_ptr() as *mut u8, size as u64); // ???


Don't free this until after you are done copying it...

You mean after copy_nonoverlapping, right? I was confused about this because in read() the ufree is called immediately after the syscall, which I didn't understand.

@lkatalin You probably don't understand it because it probably doesn't work. I was a bad boy and didn't test read() obviously... :)

Fair enough!

npmccallum · 2020-03-26T15:33:18Z

enarx-keep-sgx-shim/src/handler.rs

+                for i in 0..iovcnt {
+                    core::ptr::copy_nonoverlapping(
+                        map[i].base as *mut u8,
+                        iovec[i].base as *mut u8,
+                        ret as _,
+                    );
+                }


Iterate over the slice rather than using an index. However, keep in mind that we handed the whole array of iovecs to the untrusted host. That means that we need to validate that the values haven't been manipulated. For example, the host might replace iovec[0].base with a pointer to enclave memory in order to trick us into copying encrypted memory somewhere else. So, like we did when initializing the array, we need to iterate over both slices. Validate that the untrusted iovec hasn't been modified (and trigger attacked() if it has; that is suspicious behavior). Then do the copy.

Alternatively, we could take advantage of the contiguousness of the map buffer to bypass the untrusted array altogether. This would probably be fewer instructions. However, we'd bypass an opportunity to detect malicious behavior. So I'd prefer to be slightly slower and more security conscious.

For example, the host might replace iovec[0].base with a pointer to enclave memory ... Validate that the untrusted iovec hasn't been modified

@npmccallum For now, I'm assuming this validation means to check that none of the base pointers refer to something inside enclave memory. If there are other checks that should be done here, please let me know. I'll also continue to think on it.

I'm fine with doing the slower and more security-conscious version.

Testing that the base pointers (and base+size pointers) aren't inside the enclave is a weaker version of the testing I had in mind. I wanted to recreate the exact expected base and size that it should be and validate that the results haven't been tampered with in any way.

npmccallum · 2020-03-26T15:34:37Z

enarx-keep-sgx-shim/src/handler.rs

+        let iovec = self.aex.gpr.rsi as *mut u8;
+        let iovcnt = self.aex.gpr.rdx as usize;


Suggested change

let iovec = self.aex.gpr.rsi as *mut u8;

let iovcnt = self.aex.gpr.rdx as usize;

let trusted = unsafe {

from_raw_parts_mut(

self.aex.gpr.rsi as *mut Iovec,

self.aex.gpr.rdx as usize

);

};

npmccallum · 2020-03-26T15:37:05Z

@lkatalin Also note that clippy is giving you really good feedback here.

npmccallum

I'm not at all a fan of casting a pointer through an integer to get rid of the alignment warning. All that does it hide the problem.

I think a better strategy here is to convert the map to a byte slice immediately. Then use split_at_mut to divide the slice into the iovec prefix and the buffer suffix. Then use align_to_mut to convert the prefix from a byte slice into an iovec slice.

lkatalin · 2020-03-27T21:57:23Z

@npmccallum Good point; I've tried to do it better here without casting through an integer.

npmccallum · 2020-03-30T17:34:46Z

enarx-keep-sgx-shim/src/handler.rs

+        let (pre, untrusted, suf) = unsafe { uiovec.align_to_mut::<Iovec>() };
+        if pre != [] || suf != [] {
+            self.attacked();
+        }


Suggested change

let (pre, untrusted, suf) = unsafe { uiovec.align_to_mut::<Iovec>() };

if pre != [] || suf != [] {

self.attacked();

}

let (_, untrusted, _) = unsafe { uiovec.align_to_mut::<Iovec>() };

if untrusted.len() != trusted.len() {

self.attacked();

}

This does make way more sense as a method of checking the same thing. Thank you.

npmccallum · 2020-03-30T17:39:22Z

enarx-keep-sgx-shim/src/handler.rs

+        let mut chunk = ubuffer.as_ptr();
+        for (t, mut u) in trusted.iter_mut().zip(untrusted.iter_mut()) {
+            u.base = chunk as *mut u8;
+            u.size = t.size;
+            chunk = unsafe { chunk.add(t.size) };
+        }


Instead of converting the ubuffer to a pointer and then doing pointer math, instead track the offset and convert a subslice to a pointer. That will remove the untrusted code block.

npmccallum · 2020-03-30T17:47:18Z

enarx-keep-sgx-shim/src/handler.rs

+                let mut chunk = ubuffer.as_ptr();
+                for (t, u) in trusted.iter().zip(untrusted.iter()) {
+                    if u.base != chunk as *mut u8 || u.size != t.size {
+                        self.attacked();
+                    }
+                    core::ptr::copy_nonoverlapping(u.base as *mut u8, t.base as *mut u8, ret as _);
+                    chunk = chunk.add(t.size);
+                }


Same as above.

npmccallum · 2020-03-30T17:47:40Z

enarx-keep-sgx-shim/src/handler.rs

+                self.ufree(map.as_ptr() as *mut u8, size as u64);
+            }
+            ret


Suggested change

self.ufree(map.as_ptr() as *mut u8, size as u64);

}

ret

}

self.ufree(map.as_ptr() as *mut u8, size as u64);

ret

This also makes lots more sense.

However, I can't move this because ret is known only within the unsafe block.

npmccallum · 2020-03-30T20:16:40Z

enarx-keep-sgx-shim/src/handler.rs

+            let ret = self.syscall(
+                SysCall::READV,
+                fd,
+                untrusted.as_ptr() as _,
+                trusted.len() as u64,
+                0,
+                0,
+                0,
+            );


Suggested change

let ret = self.syscall(

SysCall::READV,

fd,

untrusted.as_ptr() as _,

trusted.len() as u64,

0,

0,

0,

);

let ret = unsafe {

self.syscall(

SysCall::READV,

fd,

untrusted.as_ptr() as _,

trusted.len() as u64,

0,

0,

0,

)

};

That enables you to reduce the size of the unsafe block significantly.

Right! Yes, I should have done this.

npmccallum · 2020-03-30T20:17:59Z

enarx-keep-sgx-shim/src/handler.rs

+
+                let mut offset = 0;
+                for (t, u) in trusted.iter().zip(untrusted.iter()) {
+                    if u.base != ubuffer[offset] as *mut u8 || u.size != t.size {


This cast doesn't do what you think it does. It converts the byte at offset to a *const u8.

npmccallum · 2020-03-30T20:18:32Z

enarx-keep-sgx-shim/src/handler.rs

+                    if u.base != ubuffer[offset] as *mut u8 || u.size != t.size {
+                        self.attacked();
+                    }
+                    core::ptr::copy_nonoverlapping(u.base as *mut u8, t.base as *mut u8, ret as _);


Use the slice method copy_from_slice() instead.

Since u.base is a *mut u8, would I not have to turn it into a slice with the unsafe core::slice::from_raw_parts_mut in order to use that method? That would result in the same number unsafe lines here (1). Is there a better way?

You know the offset into the ubuffer, which is already a slice.

npmccallum · 2020-03-31T14:30:57Z

enarx-keep-sgx-shim/src/handler.rs

+        let bufsize = trusted.iter().fold(0, |a, e| a + e.size);
+
+        // Allocate some unencrypted memory.
+        let size = bufsize + size_of::<Iovec>() * trusted.len();


prefix = sizeof * len

npmccallum · 2020-03-31T14:33:13Z

enarx-keep-sgx-shim/src/handler.rs

+                SysCall::READV,
+                fd,
+                untrusted.as_ptr() as _,
+                trusted.len() as u64,


Suggested change

trusted.len() as u64,

untrusted.len() as u64,

These values are the same. But I think this is slightly more readable.

enarx-keep-sgx-shim/src/handler.rs

npmccallum · 2020-03-31T14:42:02Z

enarx-keep-sgx-shim/src/handler.rs

+    base: *mut u8,
+    /// Number of bytes to transfer
+    size: usize,
+}


This struct probably belongs in nolibc.

lkatalin · 2020-03-31T23:06:23Z

@npmccallum I put the Iovec into nolibc. I hope I used the lifetimes correctly. There is probably some room for improvement there.

I also went ahead and included the writev call as well.

I am pretty sure since a Rust slice is a wrapper for a pointer that the way I copy them after converting to a slice from Iovec should work to update the memory pointed to without any further assignments, but it is worth double checking (ex. ln 308).

npmccallum · 2020-04-01T15:29:23Z

nolibc/src/lib.rs

+impl<'a, T> Iovec<'a, T> {
+    pub fn as_slice(&self) -> &[T] {
+        let slice: &[T] = self.into();
+        slice
+    }
+
+    pub fn as_mut_slice(&mut self) -> &mut [T] {
+        let slice: &mut [T] = self.into();
+        slice
+    }
+}


impl AsRef<[u8]> and impl AsMut<[u8]>

npmccallum · 2020-04-02T13:55:29Z

nolibc/src/lib.rs

+/// Buffer used by readv() and writev() syscalls
+#[repr(C)]
+pub struct Iovec<'a, T: 'a> {
+    /// Buffer start address
+    pub base: *mut T,
+
+    /// Number of bytes to transfer
+    pub size: usize,
+
+    phantom: core::marker::PhantomData<&'a T>,
+}


Suggested change

/// Buffer used by readv() and writev() syscalls

#[repr(C)]

pub struct Iovec<'a, T: 'a> {

/// Buffer start address

pub base: *mut T,

/// Number of bytes to transfer

pub size: usize,

phantom: core::marker::PhantomData<&'a T>,

}

/// Buffer used by readv() and writev() syscalls

#[repr(C)]

pub struct Iovec<'a> {

/// Buffer start address

pub base: *mut u8,

/// Number of bytes to transfer

pub size: usize,

phantom: core::marker::PhantomData<&'a T>,

}

That doesn't do what you think it does.

I need to learn more about this, obviously. I thought it would tie the lifetime to the base. For now, Rust won't let me compile it without the T in the struct parameters: pub struct Iovec<'a, T> since it's required in PhantomData. I changed the T in the base to a u8.

npmccallum · 2020-04-02T14:01:05Z

enarx-keep-sgx-shim/src/handler.rs

+        let mut offset = 0;
+        for (t, mut u) in trusted.iter_mut().zip(untrusted.iter_mut()) {
+            u.base = ubuffer[offset..].as_mut_ptr();
+            u.size = t.size;
+            offset += t.size;
+        }
+
+        // Copy the encrypted input into unencrypted memory.
+        let mut offset = 0;
+        for (t, u) in trusted.iter_mut().zip(untrusted.iter_mut()) {
+            if u.base != ubuffer[offset..].as_mut_ptr() || u.size != t.size {
+                self.attacked();
+            }
+
+            u.as_mut_slice().copy_from_slice(t.as_mut_slice());
+            offset += t.size;
+        }


We don't need two loops in this case. Also, we can drop the attacked check.

npmccallum · 2020-04-02T14:01:46Z

enarx-keep-sgx-shim/src/handler.rs

+            self.attacked()
+        }
+
+        unsafe { self.ufree(map.as_ptr() as *mut u8, size as u64) };


Move this to immediately after the syscall.

npmccallum

This mostly looks good. We're almost there!

npmccallum · 2020-04-02T23:01:37Z

nolibc/src/lib.rs

+#[repr(C)]
+pub struct Iovec<'a, T> {
+    /// Buffer start address
+    pub base: *mut u8,
+
+    /// Number of bytes to transfer
+    pub size: usize,
+
+    phantom: core::marker::PhantomData<&'a T>,
+}


Suggested change

#[repr(C)]

pub struct Iovec<'a, T> {

/// Buffer start address

pub base: *mut u8,

/// Number of bytes to transfer

pub size: usize,

phantom: core::marker::PhantomData<&'a T>,

}

#[repr(C)]

pub struct Iovec<'a> {

/// Buffer start address

pub base: *mut u8,

/// Number of bytes to transfer

pub size: usize,

phantom: core::marker::PhantomData<&'a ()>,

}

I didn't realize one could do this. Seems much better!

npmccallum · 2020-04-02T23:03:50Z

enarx-keep-sgx-shim/src/handler.rs

+                    self.attacked();
+                }
+
+                t.as_mut().copy_from_slice(u.as_mut());


Suggested change

t.as_mut().copy_from_slice(u.as_mut());

t.as_mut().copy_from_slice(u.as_ref());

npmccallum · 2020-04-02T23:04:29Z

enarx-keep-sgx-shim/src/handler.rs

+        // Set pointers in unencrypted iovec slice to use the rest of the allocated memory,
+        // then copy the encrypted input into unencrypted memory.
+        // The offset is into the buffer area allocated immediately after the iovec struct
+        // array, measured in bytes.


Properly wrap the paragraph.

npmccallum · 2020-04-02T23:05:15Z

enarx-keep-sgx-shim/src/handler.rs

+        for (t, mut u) in trusted.iter_mut().zip(untrusted.iter_mut()) {
+            u.base = ubuffer[offset..].as_mut_ptr();
+            u.size = t.size;
+            u.as_mut().copy_from_slice(t.as_mut());


Suggested change

u.as_mut().copy_from_slice(t.as_mut());

u.as_mut().copy_from_slice(t.as_ref());

Check that the memory allocated is page-aligned, and that the end of the returned memory is not inside the enclave.

lkatalin requested a review from npmccallum March 26, 2020 13:54

lkatalin self-assigned this Mar 26, 2020

lkatalin added the intel sgx Issues related to Intel SGX label Mar 26, 2020

npmccallum requested changes Mar 26, 2020

View reviewed changes

lkatalin force-pushed the readvwritev branch 2 times, most recently from 587f363 to b3b38ff Compare March 27, 2020 17:34

lkatalin changed the title ~~Readvwritev~~ Add readv syscall for SGX Mar 27, 2020

lkatalin marked this pull request as ready for review March 27, 2020 17:50

lkatalin requested a review from npmccallum March 27, 2020 17:51

npmccallum requested changes Mar 27, 2020

View reviewed changes

lkatalin force-pushed the readvwritev branch from b3b38ff to c4e2b65 Compare March 27, 2020 21:50

lkatalin requested a review from npmccallum March 27, 2020 21:56

npmccallum requested changes Mar 30, 2020

View reviewed changes

lkatalin force-pushed the readvwritev branch from c4e2b65 to 73353c4 Compare March 30, 2020 19:48

npmccallum requested changes Mar 30, 2020

View reviewed changes

lkatalin force-pushed the readvwritev branch from 73353c4 to 5d8d278 Compare March 31, 2020 13:54

npmccallum requested changes Mar 31, 2020

View reviewed changes

lkatalin force-pushed the readvwritev branch 4 times, most recently from 7a5cee7 to f46ddc3 Compare March 31, 2020 22:01

lkatalin changed the title ~~Add readv syscall for SGX~~ Add readv and writev syscalls for SGX Mar 31, 2020

lkatalin force-pushed the readvwritev branch 3 times, most recently from 054b3e2 to dcecb58 Compare March 31, 2020 22:56

lkatalin requested a review from npmccallum March 31, 2020 23:07

lkatalin linked an issue Apr 1, 2020 that may be closed by this pull request

Add readv, writev syscalls for SGX #376

Closed

npmccallum requested changes Apr 2, 2020

View reviewed changes

lkatalin force-pushed the readvwritev branch from dcecb58 to fc5b7fe Compare April 2, 2020 22:33

lkatalin requested a review from npmccallum April 2, 2020 22:40

npmccallum requested changes Apr 2, 2020

View reviewed changes

lkatalin force-pushed the readvwritev branch 2 times, most recently from 5131e47 to 9b839f2 Compare April 2, 2020 23:47

lkatalin added 5 commits April 3, 2020 19:31

Fix parameter order in read() syscall handler

831d448

Add security checks in ualloc()

18b3622

Check that the memory allocated is page-aligned, and that the end of the returned memory is not inside the enclave.

Add Iovec struct to nolibc

fa981c8

Add the readv() syscall handler

79c9162

Add the writev() syscall handler

44ce9f6

lkatalin force-pushed the readvwritev branch from 9b839f2 to 44ce9f6 Compare April 3, 2020 19:53

lkatalin requested a review from npmccallum April 3, 2020 20:04

npmccallum approved these changes Apr 3, 2020

View reviewed changes

npmccallum merged commit 66c452c into enarx:master Apr 3, 2020

	let size = bufsize + size_of::<Iovec>() * iovcnt;
	let size = bufsize + size_of::<Iovec>() * trusted.len();

	let map = unsafe { from_raw_parts_mut(map as *mut Iovec, iovcnt) };
	let untrusted = unsafe { from_raw_parts_mut(map as *mut Iovec, iovcnt) };

	for i in 0..iovcnt {
	for (t, u) in trusted.iter().zip(untrusted.iter()) {

		map[i].base = chunk as *mut u8;
		chunk = chunk + iovec[i].size;

		let iovec = self.aex.gpr.rsi as *mut u8;
		let iovcnt = self.aex.gpr.rdx as usize;

	t.as_mut().copy_from_slice(u.as_mut());
	t.as_mut().copy_from_slice(u.as_ref());

	u.as_mut().copy_from_slice(t.as_mut());
	u.as_mut().copy_from_slice(t.as_ref());

Add readv and writev syscalls for SGX #369

Add readv and writev syscalls for SGX #369

Conversation

lkatalin commented Mar 26, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npmccallum commented Mar 26, 2020

npmccallum left a comment

Choose a reason for hiding this comment

lkatalin commented Mar 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkatalin Mar 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkatalin commented Mar 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npmccallum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkatalin commented Mar 26, 2020 •

edited

lkatalin Mar 30, 2020 •

edited