teaclave-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From din...@apache.org
Subject [incubator-teaclave-sgx-sdk] branch master updated: Make mutex unlock errors fatal rather than ignored
Date Sun, 05 Jan 2020 00:57:20 GMT
This is an automated email from the ASF dual-hosted git repository.

dingyu pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-teaclave-sgx-sdk.git


The following commit(s) were added to refs/heads/master by this push:
     new 9082556  Make mutex unlock errors fatal rather than ignored
     new 15b8470  Merge pull request #191 from cbeck88/mutex_unlock_errors
9082556 is described below

commit 9082556911bdbcb3c9467865c5becd577ffc748e
Author: Chris Beck <beck.ct@gmail.com>
AuthorDate: Thu Jan 2 16:03:07 2020 -0800

    Make mutex unlock errors fatal rather than ignored
    
    In the paper "Remote Attestation is not Sufficient" by Yogesh Swami:
    https://www.blackhat.com/docs/us-17/thursday/us-17-Swami-SGX-Remote-Attestation-Is-Not-Sufficient-wp.pdf
    
    it is described that one way an adversary can attack an enclave is by
    maliciously interrupting it during its execution, re-entering ECALLs,
    not returning from OCALLs, etc. If they can cause undefined behavior in
    the enclave this way, then they may be able to mount an attack.
    
    We have been considering a patch like this in our internal fork of rust-sgx-sdk,
    the motivation is the following scenario
    
    ```
    {
       let _ = mutex.lock();
       my_ocall();
    }
    ```
    
    We would like to be able to say that if untrusted tries to mess with control flow
    and return from `my_ocall` unexpectedly, or on the "wrong" thread, then a panic
    will ensue, rather than the failure to properly unlock the mutex being silently
    ignored.
    
    I thought I would send you a patch and ask you what you think about this, please let me
know!
    
    ---
    
    For comparison:
    
    In POSIX, unlocking a mutex that you don't own is "illegal", and
    undefined behavior in general, but not necessarily a hard error:
    
    https://stackoverflow.com/questions/1778780/if-you-unlock-an-already-unlocked-mutex-is-the-behavior-undefined
    
    In the Intel tlibcxx C++ code, they have the following:
    
    https://github.com/intel/linux-sgx/blob/master/sdk/tlibcxx/src/mutex.cpp#L51
    
    ```
    void
    mutex::unlock() _NOEXCEPT
    {
        int ec = __libcpp_mutex_unlock(&__m_);
        (void)ec;
        assert(ec == 0);
    }
    ```
    
    That is, when compiling in debug mode, it's an assertion failure,
    but in release mode the error code is ignored.
    
    The Intel SGX SDK documentation defines `sgx_thread_mutex_unlock` and specifies
    
    ```
    Return value
    0
    The mutex is unlocked successfully.
    EINVAL
    The trusted mutex object is invalid or it is not locked by any thread.
    EPERM
    The mutex is locked by another thread.
    ```
    
    and you wrap this as `rsgx_thread_mutex_unlock` with a rust `Result` object.
    
    However, before this commit, we ultimately drop / ignore the result in the
    `SgxMutexGuard` destructor. This commit makes us panic instead.
    
    In rust `libstd`, we don't have this panic in any configuration, the error is
    always ignored
    
    https://github.com/rust-lang/rust/blob/master/src/libstd/sync/mutex.rs#L445
    
    so in particular, fortanix sgx folks have either not thought of this or chose
    not to do it.
    
    My instinct is that this probably doesn't do any measurable harm and helps to
    "fail fast" in a security critical environment which is usually good. OTOH the
    motivating example is somewhat handwaving. Maybe there is a better one, or some
    other reason not to worry about this, I'm not really sure.
---
 sgx_tstd/src/sync/mutex.rs | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sgx_tstd/src/sync/mutex.rs b/sgx_tstd/src/sync/mutex.rs
index 8cb756e..576f624 100644
--- a/sgx_tstd/src/sync/mutex.rs
+++ b/sgx_tstd/src/sync/mutex.rs
@@ -801,9 +801,12 @@ impl<T: ?Sized> DerefMut for SgxMutexGuard<'_, T> {
 impl<T: ?Sized> Drop for SgxMutexGuard<'_, T> {
     #[inline]
     fn drop(&mut self) {
-        unsafe {
+        let result = unsafe {
             self.__lock.poison.done(&self.__poison);
-            self.__lock.inner.unlock();
+            self.__lock.inner.unlock()
+        };
+        if let Err(err) = result {
+            panic!("Error when unlocking an SgxMutex: {}", err);
         }
     }
 }


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@teaclave.apache.org
For additional commands, e-mail: commits-help@teaclave.apache.org


Mime
View raw message