Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

[Pal/Linux-SGX] Fix renaming issue with protected files #2681

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion LibOS/shim/src/fs/chroot/fs.c
Original file line number Diff line number Diff line change
Expand Up @@ -969,7 +969,7 @@ static int chroot_rename(struct shim_dentry* old, struct shim_dentry* new) {
}

PAL_HANDLE pal_hdl = NULL;
ret = DkStreamOpen(qstrgetstr(&old_data->host_uri), 0, 0, 0, 0, &pal_hdl);
ret = DkStreamOpen(qstrgetstr(&old_data->host_uri), 0, 0, 0, PAL_OPTION_RENAME, &pal_hdl);
if (ret < 0) {
return pal_to_unix_errno(ret);
}
Expand Down
5 changes: 4 additions & 1 deletion LibOS/shim/test/regression/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
/multi_pthread
/multi_pthread_exitless
/openmp
/pf_rename
/pipe
/pipe_nonblocking
/pipe_ocloexec
Expand Down Expand Up @@ -107,7 +108,9 @@
/sysfs_common
/tcp_ipv6_v6only
/tcp_msg_peek
/tmp
/udp
/unix
/vfork_and_exec

/pftmp
/tmp
1 change: 1 addition & 0 deletions LibOS/shim/test/regression/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ c_executables = \
mprotect_prot_growsdown \
multi_pthread \
openmp \
pf_rename \
pipe \
pipe_nonblocking \
pipe_ocloexec \
Expand Down
5 changes: 5 additions & 0 deletions LibOS/shim/test/regression/manifest.template
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,8 @@ sgx.trusted_files = [
"file:{{ entrypoint }}",
"file:exec_victim",
]

sgx.protected_files_key = "ffeeddccbbaa99887766554433221100"
sgx.protected_files = [
"file:pftmp/"
]
77 changes: 77 additions & 0 deletions LibOS/shim/test/regression/pf_rename.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/* Protected file renaming. */

#include <assert.h>
#include <err.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>
#include <unistd.h>

#define PF_FOO_PATH "pftmp/foo.txt"
#define PF_BAR_PATH "pftmp/bar.txt"
#define BUF_SIZE 15

int main(void) {
char buf[BUF_SIZE];
char input_text[] = "Hello, world!";
int fd = open(PF_FOO_PATH, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
if (fd < 0)
err(1, "File creation failed");

size_t size = strlen(input_text) + 1;
char* str = input_text;
while (size > 0) {
ssize_t n = write(fd, str, size);
if (n == -1) {
close(fd);
err(1, "Writing to file failed");
}
assert(n <= size);
size -= n;
str += n;
}

int ret = close(fd);
if (ret < 0)
err(1, "Cannot close file");

ret = rename(PF_FOO_PATH, PF_BAR_PATH);
if (ret < 0) {
err(1, "Rename failed");
}

/* Open the renamed file */
fd = open(PF_BAR_PATH, O_RDONLY);
if (fd < 0)
err(1, "Cannot open renamed file");

size_t pos = 0;
do {
ssize_t n = read(fd, &buf[pos], BUF_SIZE - pos);
if (n == -1)
err(1, "Reading from renamed file failed");
if (n == 0) {
if (size > 0) {
warnx("Read less bytes than expected");
return -1;
}
break;
}
pos += n;
} while (pos < BUF_SIZE);

ret = close(fd);
if (ret < 0)
err(1, "Cannot close file");

buf[sizeof(buf) - 1] = '\0';

/* Check if the renamed file's contents are same as original text */
if (strncmp(input_text, buf, sizeof(input_text)))
err(1, "Renamed file content mismatching");

printf("TEST OK\n");
return 0;
}
7 changes: 7 additions & 0 deletions LibOS/shim/test/regression/test_libos.py
Original file line number Diff line number Diff line change
Expand Up @@ -830,6 +830,13 @@ def test_040_sysfs(self):
self.assertIn(f'{node}/hugepages/hugepages-2048kB/nr_hugepages: file', lines)
self.assertIn(f'{node}/hugepages/hugepages-1048576kB/nr_hugepages: file', lines)

def test_080_pf_rename(self):
for path in ['pftmp/foo.txt', 'pftmp/bar.txt']:
if os.path.exists(path):
os.remove(path)
os.makedirs('pftmp/', exist_ok=True)
stdout, _ = self.run_binary(['pf_rename'])
self.assertIn('TEST OK', stdout)

class TC_50_GDB(RegressionTestCase):
def setUp(self):
Expand Down
2 changes: 1 addition & 1 deletion Pal/include/host/Linux-common/pal_flags_conv.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ static inline int PAL_CREATE_TO_LINUX_OPEN(int create) {
}

static inline int PAL_OPTION_TO_LINUX_OPEN(int options) {
assert(WITHIN_MASK(options, PAL_OPTION_CLOEXEC | PAL_OPTION_NONBLOCK));
assert(WITHIN_MASK(options, PAL_OPTION_CLOEXEC | PAL_OPTION_NONBLOCK | PAL_OPTION_RENAME));
return (options & PAL_OPTION_CLOEXEC ? O_CLOEXEC : 0) |
(options & PAL_OPTION_NONBLOCK ? O_NONBLOCK : 0);
}
Expand Down
3 changes: 2 additions & 1 deletion Pal/include/pal/pal.h
Original file line number Diff line number Diff line change
Expand Up @@ -297,8 +297,9 @@ enum PAL_OPTION {
PAL_OPTION_CLOEXEC = 1,
PAL_OPTION_EFD_SEMAPHORE = 2, /*!< specific to `eventfd` syscall */
PAL_OPTION_NONBLOCK = 4,
PAL_OPTION_RENAME = 8, /*!<specific to `rename` syscall */

PAL_OPTION_MASK = 7,
PAL_OPTION_MASK = 15,
};

#define WITHIN_MASK(val, mask) (((val) | (mask)) == (mask))
Expand Down
53 changes: 53 additions & 0 deletions Pal/src/host/Linux-SGX/db_files.c
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,14 @@ static int file_open(PAL_HANDLE* handle, const char* type, const char* uri, int
pf_mode = PF_FILE_MODE_READ | PF_FILE_MODE_WRITE;
}

/* The file is being opened for renaming. We will need to update the metadata in the file,
* so open with RDWR mode with necessary share permissions. */
if (pal_options & PAL_OPTION_RENAME) {
pal_share = PAL_SHARE_OWNER_R | PAL_SHARE_OWNER_W;
pf_mode = PF_FILE_MODE_READ | PF_FILE_MODE_WRITE;
flags |= O_RDWR;
}

if ((pf_mode & PF_FILE_MODE_WRITE) && pf->writable_fd >= 0) {
log_warning("file_open(%s): disallowing concurrent writable handle",
hdl->file.realpath);
Expand Down Expand Up @@ -792,9 +800,54 @@ static int file_rename(PAL_HANDLE handle, const char* type, const char* uri) {
if (!tmp)
return -PAL_ERROR_NOMEM;


struct protected_file* pf = find_protected_file_handle(handle);

/* TODO: Handle the case of renaming a file that has a file handle already open, so that the
* file operations work on both the handles properly. */
if (pf) {
size_t uri_size = strlen(uri) + 1;
char* new_path = (char*)calloc(1, uri_size);

if (!new_path) {
free(tmp);
return -PAL_ERROR_NOMEM;
}

if (get_norm_path(uri, new_path, &uri_size) < 0) {
log_warning("Could not normalize path (%s)", uri);
free(tmp);
free(new_path);
return -PAL_ERROR_DENIED;
}

if (!get_protected_file(new_path)) {
log_warning("New path is disallowed for protected files (%s)", new_path);
free(tmp);
free(new_path);
return -PAL_ERROR_DENIED;
}

/* update the metadata of the protected file */
pf_status_t pf_ret = pf_rename(pf->context, new_path);

free(new_path);

if (PF_FAILURE(pf_ret)) {
log_warning("pf_rename failed: %s", pf_strerror(pf_ret));
free(tmp);
return -PAL_ERROR_DENIED;
}
}

int ret = ocall_rename(handle->file.realpath, uri);
if (ret < 0) {
free(tmp);
/* restore the original file name in pf metadata */
pf_status_t pf_ret = pf_rename(pf->context, handle->file.realpath);
if (PF_FAILURE(pf_ret)) {
log_warning("Rename failed: %s, the file might be unusable", pf_strerror(pf_ret));
}
return unix_to_pal_error(ret);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need free(tmp); after renaming?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't get your comment exactly. But, tmp needs to be freed up anyway before we return from this function. And, this rename is restoring the metadata back to what it was before. And, tmp is no more of use when we enter this if (ret < 0) condition.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@svenkata9 I mean heap variable tmp is not freed when renaming returns >= 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@duanbing It is assigned in this line, so cannot be freed.

handle->file.realpath = tmp;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thanks.


Expand Down
34 changes: 34 additions & 0 deletions Pal/src/host/Linux-SGX/protected-files/protected_files.c
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,15 @@ static bool ipf_init_new_file(pf_context_t* pf, const char* path) {
return true;
}

static bool ipf_rename_file(pf_context_t* pf, const char* new_path) {
memset(&pf->encrypted_part_plain.path, 0, sizeof(pf->encrypted_part_plain.path));
memcpy(pf->encrypted_part_plain.path, new_path, strlen(new_path) + 1);

pf->need_writing = true;

return true;
}

static bool ipf_close(pf_context_t* pf) {
void* data;
bool retval = true;
Expand Down Expand Up @@ -1320,6 +1329,31 @@ pf_status_t pf_flush(pf_context_t* pf) {
return PF_STATUS_SUCCESS;
}

pf_status_t pf_rename(pf_context_t* pf, const char* new_path) {
if (!g_initialized)
return PF_STATUS_UNINITIALIZED;

if (!pf) {
DEBUG_PF("pf_rename(PF): PF not initialized\n");
return PF_STATUS_UNKNOWN_ERROR;
}

if (!new_path)
return PF_STATUS_INVALID_PATH;

if (strlen(new_path) > PATH_MAX_SIZE - 1) {
return PF_STATUS_PATH_TOO_LONG;
}

if (!ipf_rename_file(pf, new_path))
return pf->last_error;

if (!ipf_internal_flush(pf))
return pf->last_error;

return PF_STATUS_SUCCESS;
}

pf_status_t pf_get_handle(pf_context_t* pf, pf_handle_t* handle) {
if (!g_initialized)
return PF_STATUS_UNINITIALIZED;
Expand Down
12 changes: 12 additions & 0 deletions Pal/src/host/Linux-SGX/protected-files/protected_files.h
Original file line number Diff line number Diff line change
Expand Up @@ -275,4 +275,16 @@ pf_status_t pf_get_handle(pf_context_t* pf, pf_handle_t* handle);
*/
pf_status_t pf_flush(pf_context_t* pf);

/*!
* \brief Update the path in the metadata during a rename
*
* \param [in] pf PF context
* \param [in] new_path Renamed path
* \return PF status
* \details For protected files, the file name including the path is stored in the encrypted
* metadata which is verified against the actual path during open. So, during a rename
* we need to update the metadata with the new path.
*/
pf_status_t pf_rename(pf_context_t* pf, const char* new_path);

#endif /* PROTECTED_FILES_H_ */