From fc2abe0d160890a33d8a83946f2a1ade78dd86bc Mon Sep 17 00:00:00 2001 From: Dmitrii Kuvaiskii Date: Wed, 12 May 2021 07:14:36 -0700 Subject: [PATCH 1/3] [Pal/Linux-SGX] Fix refcounting on open/close of Protected Files Previously, `file_open()` and `file_close()` on a Protected File had weird semnatics: if the same PF was opened twice, there was only one PF context created, but there were several underlying host FDs. Even weirder, the PF context tied itself to the host FD (the very first opened one) using the pointer to this FD: `&pal_handle->file.fd`. This resulted in bugs and cumbersome workarounds: the associated PAL handle could be closed and freed by our common PAL code, and the PF context would contain a dangling pointer. And for `file_attrquery()`, there was a workaround to destroy the PAL context if it seemed to be created only for this attrquery flow. This commit fixes these bugs: the PF context stores the underlying host FD directly, and doesn't open new FDs on subsequent `file_open()`. So, there is always only one host FD for one PF context. As a side effect, the logic of `file_open()` and `file_close()` is refactored. A new LibOS regression test `protected_file` is added for GDB debugging because the dedicated `fs/` tests are harder to debug. Signed-off-by: Dmitrii Kuvaiskii --- LibOS/shim/test/regression/.gitignore | 2 + LibOS/shim/test/regression/Makefile | 1 + LibOS/shim/test/regression/manifest.template | 4 + LibOS/shim/test/regression/protected_file.c | 84 ++++ LibOS/shim/test/regression/test_libos.py | 6 + Pal/src/host/Linux-SGX/db_files.c | 380 ++++++++++-------- Pal/src/host/Linux-SGX/enclave_pf.c | 2 +- Pal/src/host/Linux-SGX/pal_linux.h | 4 +- .../protected-files/protected_files.c | 17 + .../protected-files/protected_files.h | 18 + 10 files changed, 345 insertions(+), 173 deletions(-) create mode 100644 LibOS/shim/test/regression/protected_file.c diff --git a/LibOS/shim/test/regression/.gitignore b/LibOS/shim/test/regression/.gitignore index 42039e4b54..15d51a88e0 100644 --- a/LibOS/shim/test/regression/.gitignore +++ b/LibOS/shim/test/regression/.gitignore @@ -1,5 +1,6 @@ /*.manifest /*.xml +/*.dat /.cache /abort @@ -76,6 +77,7 @@ /proc_common /proc_cpuinfo /proc_path +/protected_file /pselect /pthread_set_get_affinity /rdtsc diff --git a/LibOS/shim/test/regression/Makefile b/LibOS/shim/test/regression/Makefile index e1d366718e..ce0132c7a9 100644 --- a/LibOS/shim/test/regression/Makefile +++ b/LibOS/shim/test/regression/Makefile @@ -67,6 +67,7 @@ c_executables = \ proc_common \ proc_cpuinfo \ proc_path \ + protected_file \ pselect \ pthread_set_get_affinity \ readdir \ diff --git a/LibOS/shim/test/regression/manifest.template b/LibOS/shim/test/regression/manifest.template index e8da3ea70c..9a47ed6333 100644 --- a/LibOS/shim/test/regression/manifest.template +++ b/LibOS/shim/test/regression/manifest.template @@ -40,3 +40,7 @@ sgx.allowed_files.testfile = "file:testfile" # for mmap_file test sgx.thread_num = 16 sgx.nonpie_binary = 1 + +# for protected_file test +sgx.protected_files.pf1 = "file:protected_file_1.dat" +sgx.protected_files_key = "ffeeddccbbaa99887766554433221100" diff --git a/LibOS/shim/test/regression/protected_file.c b/LibOS/shim/test/regression/protected_file.c new file mode 100644 index 0000000000..57d11d1309 --- /dev/null +++ b/LibOS/shim/test/regression/protected_file.c @@ -0,0 +1,84 @@ +/* This test opens the same file twice, reads from one FD, reads from another FD, and closes both + * FDs. This test exists mainly to test Protected Files Linux-SGX feature. */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define BUF_LENGTH 256 +#define STRING "Hello World" + +static ssize_t rw_file(int fd, char* buf, size_t bytes, bool write_flag) { + ssize_t rv = 0; + ssize_t ret; + + while (bytes > rv) { + if (write_flag) + ret = write(fd, buf + rv, bytes - rv); + else + ret = read(fd, buf + rv, bytes - rv); + + if (ret > 0) { + rv += ret; + } else { + if (ret < 0 && (errno == EAGAIN || errno == EINTR)) { + continue; + } else { + fprintf(stderr, "%s failed:%s\n", write_flag ? "write" : "read", strerror(errno)); + return ret; + } + } + } + + return rv; +} + +int main(int argc, char** argv) { + int ret; + char buf[BUF_LENGTH] = {0}; + ssize_t bytes; + + if (argc != 2) { + fprintf(stderr, "Usage: %s protected_file_path\n", argv[0]); + return 1; + } + + char* protected_file_path = argv[1]; + int fd1 = open(protected_file_path, O_CREAT | O_RDONLY, 0644); + if (fd1 < 0) + err(1, "open of first fd"); + + int fd2 = open(protected_file_path, O_CREAT | O_RDWR, 0644); + if (fd2 < 0) + err(1, "open of second fd"); + + bytes = rw_file(fd2, STRING, sizeof(STRING), /*write_flag=*/true); + if (bytes != sizeof(STRING)) + errx(1, "writing '" STRING "' to second fd failed"); + + bytes = rw_file(fd1, buf, sizeof(STRING), /*write_flag=*/false); + if (bytes < 0) + errx(1, "reading '" STRING "' from first fd failed"); + + buf[bytes - 1] = '\0'; + + if (strcmp(STRING, buf)) + errx(1, "unexpected '%s' was read", buf); + + ret = close(fd2); + if (ret < 0) + err(1, "close of second fd"); + + ret = close(fd1); + if (ret < 0) + err(1, "close of first fd"); + + puts("TEST OK"); + return 0; +} diff --git a/LibOS/shim/test/regression/test_libos.py b/LibOS/shim/test/regression/test_libos.py index f7671073d0..be697d80e1 100644 --- a/LibOS/shim/test/regression/test_libos.py +++ b/LibOS/shim/test/regression/test_libos.py @@ -381,6 +381,12 @@ def test_032_file_size(self): stdout, _ = self.run_binary(['file_size']) self.assertIn('test completed successfully', stdout) + def test_033_protected_file(self): + if os.path.exists("protected_file_1.dat"): + os.remove("protected_file_1.dat") + stdout, _ = self.run_binary(['protected_file', 'protected_file_1.dat']) + self.assertIn('TEST OK', stdout) + def test_040_futex_bitset(self): stdout, _ = self.run_binary(['futex_bitset']) diff --git a/Pal/src/host/Linux-SGX/db_files.c b/Pal/src/host/Linux-SGX/db_files.c index 6c9372178c..3f55c6a422 100644 --- a/Pal/src/host/Linux-SGX/db_files.c +++ b/Pal/src/host/Linux-SGX/db_files.c @@ -29,125 +29,182 @@ * GBs in size, and a pread OCALL could fail with -ENOMEM, so we cap to reasonably small size) */ #define MAX_READ_SIZE (PRESET_PAGESIZE * 1024L * 32) -/* 'open' operation for file streams */ +static int pf_file_open(struct protected_file* pf, const char* path, PAL_HANDLE handle, int access, + int share, int create, int options) { + int ret; + int fd = -1; + pf_status_t pfs; + + /* whether to re-initialize the PF */ + bool pf_create = (create & PAL_CREATE_ALWAYS) || (create & PAL_CREATE_TRY); + + pf_file_mode_t pf_mode = 0; + if ((access & PAL_ACCESS_RDWR) == PAL_ACCESS_RDWR) + pf_mode = PF_FILE_MODE_READ | PF_FILE_MODE_WRITE; + else if ((access & PAL_ACCESS_WRONLY) == PAL_ACCESS_WRONLY) + pf_mode = PF_FILE_MODE_WRITE; + else + pf_mode = PF_FILE_MODE_READ; + + if (!pf->context) { + /* there is no associated PF context (and thus no underlying host file descriptor), open a + * file at host level and register a PF context */ + assert(pf->refcount == 0); + assert(pf->host_fd == -1); + + /* always open underlying host file as read-write; PF logic restricts the actual allowed + * operations based on pf_mode */ + fd = ocall_open(path, O_RDWR | + PAL_CREATE_TO_LINUX_OPEN(create) | + PAL_OPTION_TO_LINUX_OPEN(options), + share); + if (fd < 0) { + ret = unix_to_pal_error(fd); + goto fail; + } + + pf->host_fd = fd; + + /* get real file size */ + struct stat st; + ret = ocall_fstat(fd, &st); + if (ret < 0) { + log_error("file_open(%s): fstat failed: %d\n", path, ret); + ret = unix_to_pal_error(ret); + goto fail; + } + + struct protected_file* ret_pf = load_protected_file(path, &pf->host_fd, st.st_size, pf_mode, + pf_create, pf); + if (!ret_pf) { + log_error("load_protected_file(%s, %d) failed\n", path, pf->host_fd); + ret = -PAL_ERROR_DENIED; + goto fail; + } + } else { + /* there is already an associated PF context (and thus an already-opened host file + * descriptor), we only need to check and extend the PF access mode if needed */ + assert(pf->refcount > 0); + + pf_file_mode_t old_pf_mode; + pfs = pf_get_mode(pf->context, &old_pf_mode); + if (PF_FAILURE(pfs)) { + log_error("pf_file_open(%s): pf_get_mode failed\n", path); + ret = -PAL_ERROR_DENIED; + goto fail; + } + + /* disallow opening more than one writable handle to a PF */ + if ((pf_mode & PF_FILE_MODE_WRITE) && (old_pf_mode & PF_FILE_MODE_WRITE)) { + log_error("pf_file_open(%s): disallowing concurrent writable handle\n", path); + ret = -PAL_ERROR_DENIED; + goto fail; + } + + pfs = pf_set_mode(pf->context, old_pf_mode | pf_mode); + __UNUSED(pfs); + assert(PF_SUCCESS(pfs)); + } + + + if (handle) { + handle->file.seekable = true; + handle->file.realpath = (PAL_STR)path; + handle->file.fd = pf->host_fd; + } + + pf->refcount++; + return 0; + +fail: + if (pf->context && pf->refcount == 0) { + /* note the refcount check: there could be an already-opened PF, so we shouldn't unload */ + unload_protected_file(pf); + } + if (fd >= 0) + ocall_close(fd); + return ret; +} + static int file_open(PAL_HANDLE* handle, const char* type, const char* uri, int access, int share, int create, int options) { if (strcmp(type, URI_TYPE_FILE)) return -PAL_ERROR_INVAL; - /* prepare the file handle */ - size_t len = strlen(uri) + 1; - PAL_HANDLE hdl = calloc(1, HANDLE_SIZE(file) + len); + int ret; + size_t uri_size = strlen(uri) + 1; + + /* allocate file handle (including enough space to hold its URI string) */ + PAL_HANDLE hdl = calloc(1, HANDLE_SIZE(file) + uri_size); if (!hdl) return -PAL_ERROR_NOMEM; SET_HANDLE_TYPE(hdl, file); HANDLE_HDR(hdl)->flags |= RFD(0) | WFD(0); - char* path = (void*)hdl + HANDLE_SIZE(file); - int ret; - if ((ret = get_norm_path(uri, path, &len)) < 0) { + + char* path = (char*)hdl + HANDLE_SIZE(file); + if ((ret = get_norm_path(uri, path, &uri_size)) < 0) { log_error("Could not normalize path (%s): %s\n", uri, pal_strerror(ret)); free(hdl); return ret; } - hdl->file.realpath = (PAL_STR)path; struct protected_file* pf = get_protected_file(path); - struct stat st; - /* whether to re-initialize the PF */ - bool pf_create = (create & PAL_CREATE_ALWAYS) || (create & PAL_CREATE_TRY); + if (pf) { + ret = pf_file_open(pf, path, hdl, access, share, create, options); + if (ret < 0) { + free(hdl); + return ret; + } - /* try to do the real open */ - int fd = ocall_open(uri, PAL_ACCESS_TO_LINUX_OPEN(access) | - PAL_CREATE_TO_LINUX_OPEN(create) | - PAL_OPTION_TO_LINUX_OPEN(options), + *handle = hdl; + return 0; + } + + /* not a protected file: must be trusted or allowed file */ + int fd = ocall_open(path, PAL_ACCESS_TO_LINUX_OPEN(access) | + PAL_CREATE_TO_LINUX_OPEN(create) | + PAL_OPTION_TO_LINUX_OPEN(options), share); if (fd < 0) { ret = unix_to_pal_error(fd); - goto out; + goto fail; } - hdl->file.fd = fd; - - /* check if the file is seekable and get real file size */ + struct stat st; ret = ocall_fstat(fd, &st); if (ret < 0) { log_error("file_open(%s): fstat failed: %d\n", path, ret); ret = unix_to_pal_error(ret); - goto out; + goto fail; } hdl->file.seekable = !S_ISFIFO(st.st_mode); + hdl->file.realpath = (PAL_STR)path; + hdl->file.fd = fd; - if (pf) { - pf_file_mode_t pf_mode = 0; - if ((access & PAL_ACCESS_RDWR) == PAL_ACCESS_RDWR) - pf_mode = PF_FILE_MODE_READ | PF_FILE_MODE_WRITE; - else if ((access & PAL_ACCESS_WRONLY) == PAL_ACCESS_WRONLY) - pf_mode = PF_FILE_MODE_WRITE; - else - pf_mode = PF_FILE_MODE_READ; - - /* disallow opening more than one writable handle to a PF */ - if (pf_mode & PF_FILE_MODE_WRITE) { - if (pf->writable_fd >= 0) { - log_error("file_open(%s): disallowing concurrent writable handle\n", path); - ret = -PAL_ERROR_DENIED; - goto out; - } - } - - ret = -PAL_ERROR_DENIED; - - /* the protected files should be regular files (seekable) */ - if (!hdl->file.seekable) { - log_error("file_open(%s): disallowing non-seekable file handle\n", path); - goto out; - } - - pf = load_protected_file(path, (int*)&hdl->file.fd, st.st_size, pf_mode, pf_create, pf); - if (pf) { - pf->refcount++; - if (pf_mode & PF_FILE_MODE_WRITE) { - pf->writable_fd = fd; - } - } else { - log_error("load_protected_file(%s, %d) failed\n", path, hdl->file.fd); - goto out; - } - } else { - sgx_chunk_hash_t* chunk_hashes; - uint64_t total; - void* umem; - ret = load_trusted_file(hdl, &chunk_hashes, &total, create, &umem); - if (ret < 0) { - log_error("Accessing file:%s is denied (%s). This file is not trusted or allowed." - " Trusted files should be regular files (seekable).\n", hdl->file.realpath, - pal_strerror(ret)); - goto out; - } - - if (chunk_hashes && total) { - assert(umem); - } - - hdl->file.chunk_hashes = (PAL_PTR)chunk_hashes; - hdl->file.total = total; - hdl->file.umem = umem; + sgx_chunk_hash_t* chunk_hashes; + uint64_t total; + void* umem; + ret = load_trusted_file(hdl, &chunk_hashes, &total, create, &umem); + if (ret < 0) { + log_error("Accessing file:%s is denied (%s). This file is not trusted or allowed." + " Trusted files should be regular files (seekable).\n", hdl->file.realpath, + pal_strerror(ret)); + goto fail; } - *handle = hdl; - ret = 0; + hdl->file.chunk_hashes = (PAL_PTR)chunk_hashes; + hdl->file.total = total; + hdl->file.umem = umem; -out: - if (ret != 0) { - if (pf && pf->context && pf->refcount == 0) - unload_protected_file(pf); + *handle = hdl; + return 0; - free(hdl); - if (fd >= 0) - ocall_close(fd); - } +fail: + free(hdl); + if (fd >= 0) + ocall_close(fd); return ret; } @@ -171,7 +228,6 @@ static int64_t pf_file_read(struct protected_file* pf, PAL_HANDLE handle, uint64 return bytes_read; } -/* 'read' operation for file streams. */ static int64_t file_read(PAL_HANDLE handle, uint64_t offset, uint64_t count, void* buffer) { struct protected_file* pf = find_protected_file_handle(handle); @@ -231,7 +287,6 @@ static int64_t pf_file_write(struct protected_file* pf, PAL_HANDLE handle, uint6 return count; } -/* 'write' operation for file streams. */ static int64_t file_write(PAL_HANDLE handle, uint64_t offset, uint64_t count, const void* buffer) { struct protected_file* pf = find_protected_file_handle(handle); @@ -259,53 +314,57 @@ static int64_t file_write(PAL_HANDLE handle, uint64_t offset, uint64_t count, co return -PAL_ERROR_DENIED; } -static int pf_file_close(struct protected_file* pf, PAL_HANDLE handle) { - int fd = handle->file.fd; +static int pf_file_close(struct protected_file* pf) { + int ret; if (pf->refcount == 0) { - log_error("pf_file_close(PF fd %d): refcount == 0\n", fd); + log_error("pf_file_close(PF fd %d): refcount == 0\n", pf->host_fd); return -PAL_ERROR_INVAL; } + /* NOTE: ideally, we want to narrow access mode for cases when a read-write PF handle is closed + * and only a read-only/write-only PF handles are left for this PF context; but this is a + * rare corner case */ + pf->refcount--; + if (pf->refcount > 0) + return 0; - if (pf->writable_fd == fd) - pf->writable_fd = -1; + /* this is last opened reference to protected file, clean up */ + ret = unload_protected_file(pf); + if (ret < 0) { + log_error("pf_file_close(PF fd %d): unload_protected_file failed\n", pf->host_fd); + return ret; + } - if (pf->refcount == 0) - return unload_protected_file(pf); + ret = ocall_close(pf->host_fd); + if (ret < 0) { + log_error("pf_file_close(PF fd %d): ocall_close failed\n", pf->host_fd); + return ret; + } + pf->host_fd = -1; return 0; } -/* 'close' operation for file streams. In this case, it will only - close the file without deleting it. */ static int file_close(PAL_HANDLE handle) { - int fd = handle->file.fd; - struct protected_file* pf = find_protected_file_handle(handle); + /* initial realpath is part of handle object and will be freed with it */ + if (handle->file.realpath && handle->file.realpath != (void*)handle + HANDLE_SIZE(file)) + free((void*)handle->file.realpath); - if (pf) { - int ret = pf_file_close(pf, handle); - if (ret < 0) - return ret; - } + struct protected_file* pf = find_protected_file_handle(handle); + if (pf) + return pf_file_close(pf); if (handle->file.chunk_hashes && handle->file.total) { /* case of trusted file: the whole file was mmapped in untrusted memory */ ocall_munmap_untrusted(handle->file.umem, handle->file.total); } - ocall_close(fd); - - /* initial realpath is part of handle object and will be freed with it */ - if (handle->file.realpath && handle->file.realpath != (void*)handle + HANDLE_SIZE(file)) - free((void*)handle->file.realpath); - + ocall_close(handle->file.fd); return 0; } -/* 'delete' operation for file streams. It will actually delete - the file if we can successfully close it. */ static int file_delete(PAL_HANDLE handle, int access) { if (access) return -PAL_ERROR_INVAL; @@ -403,7 +462,6 @@ static int pf_file_map(struct protected_file* pf, PAL_HANDLE handle, void** addr return ret; } -/* 'map' operation for file stream. */ static int file_map(PAL_HANDLE handle, void** addr, int prot, uint64_t offset, uint64_t size) { int ret; @@ -524,7 +582,6 @@ static int64_t pf_file_setlength(struct protected_file* pf, PAL_HANDLE handle, u return length; } -/* 'setlength' operation for file stream. */ static int64_t file_setlength(PAL_HANDLE handle, uint64_t length) { struct protected_file* pf = find_protected_file_handle(handle); if (pf) @@ -538,7 +595,6 @@ static int64_t file_setlength(PAL_HANDLE handle, uint64_t length) { return (int64_t)length; } -/* 'flush' operation for file stream. */ static int file_flush(PAL_HANDLE handle) { int fd = handle->file.fd; struct protected_file* pf = find_protected_file_handle(handle); @@ -574,7 +630,6 @@ static inline int file_stat_type(struct stat* stat) { return 0; } -/* copy attr content from POSIX stat struct to PAL_STREAM_ATTR */ static inline void file_attrcopy(PAL_STREAM_ATTR* attr, struct stat* stat) { attr->handle_type = file_stat_type(stat); attr->disconnected = PAL_FALSE; @@ -586,99 +641,84 @@ static inline void file_attrcopy(PAL_STREAM_ATTR* attr, struct stat* stat) { attr->pending_size = stat->st_size; } -static int pf_file_attrquery(struct protected_file* pf, int fd_from_attrquery, const char* path, - uint64_t real_size, PAL_STREAM_ATTR* attr) { - pf = load_protected_file(path, &fd_from_attrquery, real_size, PF_FILE_MODE_READ, - /*create=*/false, pf); - if (!pf) { - log_error("pf_file_attrquery: load_protected_file(%s, %d) failed\n", path, - fd_from_attrquery); - /* The call above will fail for PFs that were tampered with or have a wrong path. - * glibc kills the process if this fails during directory enumeration, but that - * should be fine given the scenario. - */ - return -PAL_ERROR_DENIED; +static int pf_file_attrquery(struct protected_file* pf, const char* path, PAL_STREAM_ATTR* attr) { + int ret; + + /* protected file may not be opened (and thus have no context from which to grab the actual data + * size), so temporarily open it */ + ret = pf_file_open(pf, path, /*handle=*/NULL, PAL_ACCESS_RDONLY, /*share=*/0, /*create=*/0, + /*options=*/0); + if (ret < 0) { + log_error("pf_file_attrquery(%s): pf_file_open failed\n", path); + return ret; } + assert(pf->context); + uint64_t size; pf_status_t pfs = pf_get_size(pf->context, &size); __UNUSED(pfs); assert(PF_SUCCESS(pfs)); attr->pending_size = size; - pf_handle_t pf_handle; - pfs = pf_get_handle(pf->context, &pf_handle); - assert(PF_SUCCESS(pfs)); - - if (fd_from_attrquery == *(int*)pf_handle) { /* this is a PF opened just for us, close it */ - pfs = pf_close(pf->context); - pf->context = NULL; - assert(PF_SUCCESS(pfs)); + ret = pf_file_close(pf); + if (ret < 0) { + log_error("pf_file_attrquery(%s): pf_file_close failed\n", path); + return ret; } return 0; } -/* 'attrquery' operation for file streams */ static int file_attrquery(const char* type, const char* uri, PAL_STREAM_ATTR* attr) { if (strcmp(type, URI_TYPE_FILE) && strcmp(type, URI_TYPE_DIR)) return -PAL_ERROR_INVAL; - /* open the file with O_NONBLOCK to avoid blocking the current thread if it is actually a FIFO - * pipe; O_NONBLOCK will be reset below if it is a regular file */ - int fd = ocall_open(uri, O_NONBLOCK, 0); - if (fd < 0) - return unix_to_pal_error(fd); - + int ret; + int fd = -1; char* path = NULL; - struct stat stat_buf; - int ret = ocall_fstat(fd, &stat_buf); + size_t len = URI_MAX; - /* if it failed, return the right error code */ + path = malloc(len); + ret = get_norm_path(uri, path, &len); if (ret < 0) { - ret = unix_to_pal_error(ret); + log_error("Could not normalize path (%s): %s\n", uri, pal_strerror(ret)); goto out; } - file_attrcopy(attr, &stat_buf); + /* open with O_NONBLOCK to avoid blocking the current thread if it is actually a FIFO pipe */ + fd = ocall_open(uri, O_NONBLOCK, 0); + if (fd < 0) { + ret = unix_to_pal_error(fd); + goto out; + } - size_t len = URI_MAX; - path = malloc(len); - ret = get_norm_path(uri, path, &len); + struct stat stat_buf; + ret = ocall_fstat(fd, &stat_buf); if (ret < 0) { - log_error("Could not normalize path (%s): %s\n", uri, pal_strerror(ret)); + ret = unix_to_pal_error(ret); goto out; } - /* For protected files return the data size, not real FS size */ + file_attrcopy(attr, &stat_buf); + + /* for protected files, replace attr's size (which is a real FS size) with PF data size */ struct protected_file* pf = get_protected_file(path); if (pf && attr->handle_type != pal_type_dir) { - /* protected files should be regular files */ - if (S_ISFIFO(stat_buf.st_mode)) { - ret = -PAL_ERROR_DENIED; - goto out; - } - - /* reset O_NONBLOCK because pf_file_attrquery() may issue reads which don't expect - * non-blocking mode */ - ret = ocall_fsetnonblock(fd, 0); - if (ret < 0) { - ret = unix_to_pal_error(ret); + ret = pf_file_attrquery(pf, path, attr); + if (ret < 0) goto out; - } - - ret = pf_file_attrquery(pf, fd, path, stat_buf.st_size, attr); - } else { - ret = 0; } + ret = 0; + out: + if (fd >= 0) + ocall_close(fd); free(path); - ocall_close(fd); return ret; } -/* 'attrquerybyhdl' operation for file streams */ static int file_attrquerybyhdl(PAL_HANDLE handle, PAL_STREAM_ATTR* attr) { int fd = handle->file.fd; struct stat stat_buf; diff --git a/Pal/src/host/Linux-SGX/enclave_pf.c b/Pal/src/host/Linux-SGX/enclave_pf.c index 9c81581527..f97bd5c720 100644 --- a/Pal/src/host/Linux-SGX/enclave_pf.c +++ b/Pal/src/host/Linux-SGX/enclave_pf.c @@ -376,7 +376,7 @@ static int register_protected_path(const char* path, struct protected_file** new memcpy(new->path, path, new->path_len + 1); new->refcount = 0; - new->writable_fd = -1; + new->host_fd = -1; bool is_dir; ret = is_directory(path, &is_dir); diff --git a/Pal/src/host/Linux-SGX/pal_linux.h b/Pal/src/host/Linux-SGX/pal_linux.h index 8ec8ad24e6..022372458c 100644 --- a/Pal/src/host/Linux-SGX/pal_linux.h +++ b/Pal/src/host/Linux-SGX/pal_linux.h @@ -177,8 +177,8 @@ struct protected_file { size_t path_len; char* path; pf_context_t* context; /* NULL until PF is opened */ - int64_t refcount; /* used for deciding when to call unload_protected_file() */ - int writable_fd; /* fd of underlying file for writable PF, -1 if no writable handles are open */ + int64_t refcount; /* used for deciding when to call unload_protected_file() */ + int host_fd; /* -1 until PF is opened */ }; /* Initialize the PF library, register PFs from the manifest */ diff --git a/Pal/src/host/Linux-SGX/protected-files/protected_files.c b/Pal/src/host/Linux-SGX/protected-files/protected_files.c index d9f3520277..8b42734640 100644 --- a/Pal/src/host/Linux-SGX/protected-files/protected_files.c +++ b/Pal/src/host/Linux-SGX/protected-files/protected_files.c @@ -1236,6 +1236,23 @@ pf_status_t pf_close(pf_context_t* pf) { return pf->last_error; } +pf_status_t pf_get_mode(pf_context_t* pf, pf_file_mode_t* mode) { + if (!g_initialized) + return PF_STATUS_UNINITIALIZED; + + *mode = pf->mode; + return PF_STATUS_SUCCESS; +} + +pf_status_t pf_set_mode(pf_context_t* pf, pf_file_mode_t mode) { + if (!g_initialized) + return PF_STATUS_UNINITIALIZED; + + assert((pf->mode & mode) == pf->mode); /* can only extend old mode, cannot narrow it */ + pf->mode = mode; + return PF_STATUS_SUCCESS; +} + pf_status_t pf_get_size(pf_context_t* pf, uint64_t* size) { if (!g_initialized) return PF_STATUS_UNINITIALIZED; diff --git a/Pal/src/host/Linux-SGX/protected-files/protected_files.h b/Pal/src/host/Linux-SGX/protected-files/protected_files.h index fba05e5beb..fef981c897 100644 --- a/Pal/src/host/Linux-SGX/protected-files/protected_files.h +++ b/Pal/src/host/Linux-SGX/protected-files/protected_files.h @@ -225,6 +225,24 @@ pf_status_t pf_read(pf_context_t* pf, uint64_t offset, size_t size, void* output */ pf_status_t pf_write(pf_context_t* pf, uint64_t offset, size_t size, const void* input); +/*! + * \brief Get access mode (read-only, write-only, read-write) of a PF + * + * \param [in] pf PF context + * \param [out] mode Access mode of \a pf + * \return PF status + */ +pf_status_t pf_get_mode(pf_context_t* pf, pf_file_mode_t* mode); + +/*! + * \brief Set access mode (read-only, write-only, read-write) of a PF + * + * \param [in] pf PF context + * \param [in] mode New access mode of \a pf + * \return PF status + */ +pf_status_t pf_set_mode(pf_context_t* pf, pf_file_mode_t mode); + /*! * \brief Get data size of a PF * From 65cb22a0b998b1bd04d7e637be9cb1aa06ebcf03 Mon Sep 17 00:00:00 2001 From: Dmitrii Kuvaiskii Date: Wed, 12 May 2021 07:38:44 -0700 Subject: [PATCH 2/3] fixup! [Pal/Linux-SGX] Fix refcounting on open/close of Protected Files Signed-off-by: Dmitrii Kuvaiskii --- LibOS/shim/test/regression/protected_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/LibOS/shim/test/regression/protected_file.c b/LibOS/shim/test/regression/protected_file.c index 57d11d1309..5132e77634 100644 --- a/LibOS/shim/test/regression/protected_file.c +++ b/LibOS/shim/test/regression/protected_file.c @@ -1,4 +1,4 @@ -/* This test opens the same file twice, reads from one FD, reads from another FD, and closes both +/* This test opens the same file twice, writes to one FD, reads from another FD, and closes both * FDs. This test exists mainly to test Protected Files Linux-SGX feature. */ #include From a961869963523e50d07d7ee44ec9777c5c0c5fae Mon Sep 17 00:00:00 2001 From: Dmitrii Kuvaiskii Date: Fri, 14 May 2021 01:26:04 -0700 Subject: [PATCH 3/3] fixup! [Pal/Linux-SGX] Fix refcounting on open/close of Protected Files Signed-off-by: Dmitrii Kuvaiskii --- LibOS/shim/test/regression/Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/LibOS/shim/test/regression/Makefile b/LibOS/shim/test/regression/Makefile index ce0132c7a9..6feeed8c9e 100644 --- a/LibOS/shim/test/regression/Makefile +++ b/LibOS/shim/test/regression/Makefile @@ -177,6 +177,7 @@ clean-tmp: *.sig \ *.tmp \ *.token \ + *.dat \ *~ \ .cache \ .pytest_cache \