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

[svenkata9]

Description of the changes

With certain workloads involving Federated Learning, the frameworks create protected files and rename them as part of the flow. This does not work currently with protected files because every protected file maintains the path name in its metadata (stored when the file was first created) and which is checked during open to make sure the incoming path is the same as what is already stored in the metadata for this file.

This PR is to address this issue by handling rename for protected files. A normal rename call does not need the file to be open in RW mode, but since the metadata needs to be updated with the new path, we pass a special PAL_OPTION_RENAME flag to DkStreamOpen during rename and open the protected file in RW mode with required permissions, update the metadata with the new path and flush it. Then the flow goes through the one for normal file renaming. If the ocall to rename fails, we restore the metadata back to what it was earlier so the file remains usable still.

Signed-off-by: Sankaranarayanan Venkatasubramanian [email protected]

---

This change is 

[dimakuv]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @svenkata9)

---

LibOS/shim/src/fs/chroot/fs.c, line 204 at r1 (raw file):

/* Open a temporary PAL handle for a file (used by `rename`, `unlink` etc.) */
static int chroot_temp_open(struct shim_dentry* dent, mode_t type, PAL_HANDLE* out_palhdl, int pal_options) {

Please swap out_palhdl and pal_options places, like this:

static int chroot_temp_open(struct shim_dentry* dent, mode_t type, int pal_options, PAL_HANDLE* out_palhdl) {

This is because output arguments should be always the last ones in functions.

---

Pal/src/host/Linux-SGX/db_files.c, line 49 at r1 (raw file):

        return -PAL_ERROR_INVAL;

    /* normalize uri into normpath */

FYI: You can check the snippet below for the renames I suggest.

---

Pal/src/host/Linux-SGX/db_files.c, line 799 at r1 (raw file):

        return -PAL_ERROR_INVAL;

    char* tmp = strdup(uri);

This got very hard to understand. Let's rename everything to more meaningful names.

Please rename tmp to new_path (because that's what it is).

---

Pal/src/host/Linux-SGX/db_files.c, line 807 at r1 (raw file):

    /* 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. */

This comment is unnecessarily long. Simply:

/* TODO: Handle the case of renaming a file that has a file handle already open */

---

Pal/src/host/Linux-SGX/db_files.c, line 808 at r1 (raw file):

    /* 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. */
    struct protected_file* pf_new;

Please move this declaration to where you initialize this variable. Like this:

struct protected_file* pf_new = get_protected_file(new_path);

There is no sense in declaring this variable here, so far away from where it is actually used.

---

Pal/src/host/Linux-SGX/db_files.c, line 810 at r1 (raw file):

    struct protected_file* pf_new;
    if (pf) {
        size_t uri_size = strlen(uri) + 1;

Please rename uri_size to normpath_size

---

Pal/src/host/Linux-SGX/db_files.c, line 811 at r1 (raw file):

    if (pf) {
        size_t uri_size = strlen(uri) + 1;
        char* new_path = (char*)calloc(1, uri_size);

Please rename new_path to new_normpath

---

Pal/src/host/Linux-SGX/db_files.c, line 815 at r1 (raw file):

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

All these error handling cases are hard to read now... Please refactor them all to use goto, like in other code snippets.

You will have smth like this in the end:

        if (!new_path) {
            ret = -PAL_ERROR_NOMEM;
            goto out;
        }

...

    handle->file.realpath = new_path;
    ret = 0;
out:
    free(new_normpath);
    if (ret < 0)
        free(new_path);
    return ret;

---

Pal/src/host/Linux-SGX/db_files.c, line 827 at r1 (raw file):

        pf_new = get_protected_file(new_path);
        if (!pf_new) {
            log_warning("New path is disallowed for protected files (%s)", new_path);

This message doesn't make sense. I think what you're trying to say here is New path during rename is not a protected directory (%s)

---

Pal/src/host/Linux-SGX/db_files.c, line 848 at r1 (raw file):

    if (ret < 0) {
        free(tmp);
        /* restore the original file name in pf metadata */

You should only execute this code if pf != NULL, otherwise you will do this "rollback PF rename" even for a regular file.

---

Pal/src/host/Linux-SGX/db_files.c, line 856 at r1 (raw file):

    }

    /* TODO: Handle file_close for the source file during protected file rename works properly */

I can't parse this sentence.

---

Pal/src/host/Linux-SGX/db_files.c, line 859 at r1 (raw file):

    if (pf) {
        struct protected_file* tmp = pf;
        pf = pf_new;

What's the point of this operation?

---

Pal/src/host/Linux-SGX/db_files.c, line 860 at r1 (raw file):

        struct protected_file* tmp = pf;
        pf = pf_new;
        ret = pf_file_close(tmp, handle);

Why do you need to close the file?

[pwmarcz]

Reviewed 3 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @svenkata9)

---

Pal/src/host/Linux-SGX/db_files.c, line 827 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This message doesn't make sense. I think what you're trying to say here is New path during rename is not a protected directory (%s)

It's not necessarily a directory, though - if the target is foo/bar, the sgx.protected_files can indeed specify foo/, but it can also specify foo/bar exactly.

So maybe something like "New path during rename is not specified in 'sgx.protected_files' (%s)"?

[svenkata9]

Reviewable status: 4 of 6 files reviewed, 12 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @pwmarcz)

---

LibOS/shim/src/fs/chroot/fs.c, line 204 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please swap out_palhdl and pal_options places, like this:

static int chroot_temp_open(struct shim_dentry* dent, mode_t type, int pal_options, PAL_HANDLE* out_palhdl) {

This is because output arguments should be always the last ones in functions.

Done.

---

Pal/src/host/Linux-SGX/db_files.c, line 799 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This got very hard to understand. Let's rename everything to more meaningful names.

Please rename tmp to new_path (because that's what it is).

Done.

---

Pal/src/host/Linux-SGX/db_files.c, line 807 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This comment is unnecessarily long. Simply:

/* TODO: Handle the case of renaming a file that has a file handle already open */

Done.

---

Pal/src/host/Linux-SGX/db_files.c, line 808 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please move this declaration to where you initialize this variable. Like this:

struct protected_file* pf_new = get_protected_file(new_path);

There is no sense in declaring this variable here, so far away from where it is actually used.

This cannot be done because it needs to be outside the current scope for it to be used later.

---

Pal/src/host/Linux-SGX/db_files.c, line 810 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please rename uri_size to normpath_size

Done.

---

Pal/src/host/Linux-SGX/db_files.c, line 811 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please rename new_path to new_normpath

Done.

---

Pal/src/host/Linux-SGX/db_files.c, line 815 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

All these error handling cases are hard to read now... Please refactor them all to use goto, like in other code snippets.

You will have smth like this in the end:

        if (!new_path) {
            ret = -PAL_ERROR_NOMEM;
            goto out;
        }

...

    handle->file.realpath = new_path;
    ret = 0;
out:
    free(new_normpath);
    if (ret < 0)
        free(new_path);
    return ret;

There is a mixture of (inner/outer) scope here. I would let it the way it is now, and refactor later.

---

Pal/src/host/Linux-SGX/db_files.c, line 827 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

It's not necessarily a directory, though - if the target is foo/bar, the sgx.protected_files can indeed specify foo/, but it can also specify foo/bar exactly.

So maybe something like "New path during rename is not specified in 'sgx.protected_files' (%s)"?

Done.

---

Pal/src/host/Linux-SGX/db_files.c, line 848 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You should only execute this code if pf != NULL, otherwise you will do this "rollback PF rename" even for a regular file.

Done.

---

Pal/src/host/Linux-SGX/db_files.c, line 856 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I can't parse this sentence.

Like I said most of the issues that we currently have with protected files is because of the global g_protected_files path that requires unique key and fails when the same file name is provided for multiple simultaneous operations. In this rename case, we have already enumerated and if we close it immediately, some of the issues can be avoided (like the first two cases in rename_unlink will pass only when this is closed here). Like I said earlier, it is a workaround - a proper fix will require reworking g_protected_files to search for the handle (currently it happens based on merely the name).

---

Pal/src/host/Linux-SGX/db_files.c, line 859 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What's the point of this operation?

Explained above

---

Pal/src/host/Linux-SGX/db_files.c, line 860 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why do you need to close the file?

Explained above

[dimakuv]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @svenkata9)

---

Pal/src/host/Linux-SGX/db_files.c, line 808 at r1 (raw file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

This cannot be done because it needs to be outside the current scope for it to be used later.

You don't really use pf_new afterwards (after the line if (!pf_new) {). See my other comment.

---

Pal/src/host/Linux-SGX/db_files.c, line 815 at r1 (raw file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

There is a mixture of (inner/outer) scope here. I would let it the way it is now, and refactor later.

OK, personal preference. I'm fine with this way too.

---

Pal/src/host/Linux-SGX/db_files.c, line 856 at r1 (raw file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

Like I said most of the issues that we currently have with protected files is because of the global g_protected_files path that requires unique key and fails when the same file name is provided for multiple simultaneous operations. In this rename case, we have already enumerated and if we close it immediately, some of the issues can be avoided (like the first two cases in rename_unlink will pass only when this is closed here). Like I said earlier, it is a workaround - a proper fix will require reworking g_protected_files to search for the handle (currently it happens based on merely the name).

That I can understand, but the sentence itself is not proper English (or very confusing English).

Is Handle a verb here? works properly -- what should work properly? Or something already works properly? Please rephrase.

---

Pal/src/host/Linux-SGX/db_files.c, line 859 at r1 (raw file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

Explained above

So in this line you're assigning pf = pf_new. Why? You don't use pf later in the code, so this new value of pf is unused and thus meaningless. Why can't you replace this whole IF statement with a more simple one:

    if (pf) {
        ret = pf_file_close(pf, handle);
        if (ret < 0)
            log_warning("pf_file_close failed during rename");
    }

---

Pal/src/host/Linux-SGX/db_files.c, line 860 at r1 (raw file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

Explained above

I'm not sure I understand this, but Ok. We need to change the code of PFs anyway, because indeed there are multiple bugs when two handles for the same PF file are opened simulteneously.

[pwmarcz]

Reviewed 2 of 6 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @svenkata9)

---

LibOS/shim/src/fs/chroot/fs.c, line 204 at r2 (raw file):

/* Open a temporary PAL handle for a file (used by `rename`, `unlink` etc.) */
static int chroot_temp_open(struct shim_dentry* dent, mode_t type, int pal_options ,PAL_HANDLE* out_palhdl) {

There should be a space after the comma, and not before.

Also, please wrap this line to 100 characters (see coding style for details).

(I recommend configuring an editor to display a line at the 100 column mark, so that you can notice when you're going over it).

[svenkata9]

Reviewable status: 4 of 6 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @pwmarcz, and @svenkata9)

---

LibOS/shim/src/fs/chroot/fs.c, line 204 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

There should be a space after the comma, and not before.

Also, please wrap this line to 100 characters (see coding style for details).

(I recommend configuring an editor to display a line at the 100 column mark, so that you can notice when you're going over it).

Done.

---

Pal/src/host/Linux-SGX/db_files.c, line 808 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You don't really use pf_new afterwards (after the line if (!pf_new) {). See my other comment.

Done.

---

Pal/src/host/Linux-SGX/db_files.c, line 856 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

That I can understand, but the sentence itself is not proper English (or very confusing English).

Is Handle a verb here? works properly -- what should work properly? Or something already works properly? Please rephrase.

I removed the comment and kept the things simple

[svenkata9]

Reviewable status: 4 of 6 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @pwmarcz)

---

Pal/src/host/Linux-SGX/db_files.c, line 859 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

So in this line you're assigning pf = pf_new. Why? You don't use pf later in the code, so this new value of pf is unused and thus meaningless. Why can't you replace this whole IF statement with a more simple one:

    if (pf) {
        ret = pf_file_close(pf, handle);
        if (ret < 0)
            log_warning("pf_file_close failed during rename");
    }

Done.

[dimakuv]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz and @svenkata9)

---

Pal/src/host/Linux-SGX/db_files.c, line 802 at r3 (raw file):

    if (!new_path)
        return -PAL_ERROR_NOMEM;

One empty line can be removed. But not blocking, not too important.

[dimakuv]

Jenkins, test this please

[pwmarcz]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @svenkata9)

[mkow]

Reviewed 4 of 6 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @svenkata9)

a discussion (no related file):
Please add proper tests to this PR.

---

---

-- commits, line 2 at r3:
Fix renaming issue with protected files -> Add support for renaming protected files (it's not a bug, but a missing feature you're adding here)

---

Pal/include/pal/pal.h, line 300 at r3 (raw file):

    PAL_OPTION_EFD_SEMAPHORE = 2, /*!< specific to `eventfd` syscall */
    PAL_OPTION_NONBLOCK      = 4,
    PAL_OPTION_RENAME        = 8, /*!<specific to `rename` syscall */

missing space after <

---

Pal/src/host/Linux-SGX/db_files.c, line 128 at r3 (raw 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;

Why are you changing pal_share here? Isn't it ignored when we aren't creating a new file?

---

Pal/src/host/Linux-SGX/db_files.c, line 130 at r3 (raw file):

flags |= O_RDWR;

This isn't a bitmask, but an enum written on O_ACCMODE bits, so you can't OR its values.

---

Pal/src/host/Linux-SGX/db_files.c, line 827 at r3 (raw file):

            free(new_normpath);
            free(new_path);
            return -PAL_ERROR_DENIED;

Please create a single return point from this function and free all the buffers there.

---

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 378 at r3 (raw file):

    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);

What about g_protected_files? Isn't get_protected_file() returning wrong files from now on?

---

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 1336 at r3 (raw file):

        return PF_STATUS_UNINITIALIZED;

    if (!pf) {

Why would you check for this?

---

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 1341 at r3 (raw file):

    }

    if (!new_path)

ditto, NULL is an invalid argument to this function and no callsite should ever allow it

---

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 1344 at r3 (raw file):

        return PF_STATUS_INVALID_PATH;

    if (strlen(new_path) > PATH_MAX_SIZE - 1) {

Why here, not in ipf_rename_file()?

[svenkata9]

Reviewable status: 3 of 6 files reviewed, 10 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @pwmarcz)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

Please add proper tests to this PR.

rename_unlink tests should already cover this PR also, but since there are issues with some of the tests when it comes to Protected FS, we have disabled it rather than having a separate test (which I added a while ago). Please see earlier discussions.

---

---

-- commits, line 2 at r3:

Previously, mkow (Michał Kowalczyk) wrote…

Fix renaming issue with protected files -> Add support for renaming protected files (it's not a bug, but a missing feature you're adding here)

Actually it is a bug.

---

Pal/include/pal/pal.h, line 300 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

missing space after <

Done.

---

Pal/src/host/Linux-SGX/db_files.c, line 128 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why are you changing pal_share here? Isn't it ignored when we aren't creating a new file?

Without these share permissions, the write operation on this fail to succeed.

---

Pal/src/host/Linux-SGX/db_files.c, line 130 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

flags |= O_RDWR;

This isn't a bitmask, but an enum written on O_ACCMODE bits, so you can't OR its values.

Why is this then OR-ed in

gramine/Pal/src/host/Linux-SGX/db_files.c

Line 42 in 487969c

int flags = PAL_ACCESS_TO_LINUX_OPEN(pal_access) |

?

---

Pal/src/host/Linux-SGX/db_files.c, line 827 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Please create a single return point from this function and free all the buffers there.

We already discussed this in an earlier discussion. Please see that discussion. There is a mix of inner/outer scope here. I would leave it like that and revisit later.

---

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 378 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

What about g_protected_files? Isn't get_protected_file() returning wrong files from now on?

It would not. It would register the file as part of rename, but this pf will have the older context.

---

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 1336 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why would you check for this?

Done.

---

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 1341 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto, NULL is an invalid argument to this function and no callsite should ever allow it

Done.

---

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 1344 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why here, not in ipf_rename_file()?

Done.

[mkow]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @svenkata9)

a discussion (no related file):

rename_unlink tests should already cover this PR

It doesn't, currently it's disabled for PF, as you said. So, currently there's no test verifying this feature.

but since there are issues with some of the tests when it comes to Protected FS

AFAIU, Paweł disabled it just because his PR was based on master (not this PR), so the PF part couldn't work. And he expected you to enable this part of the test here.

Please see earlier discussions.

I can't find any discussion which concludes that it's fine to merge this without tests.

---

---

-- commits, line 2 at r3:

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

Actually it is a bug.

The only bug we had was that DkStreamChangeName() wasn't loudly failing when the app tried to rename a protected file. Before your PR renaming protected files was explicitly not supported and not considered in the security analysis.

---

Pal/src/host/Linux-SGX/db_files.c, line 815 at r1 (raw file):
+1 to the Dmitrii's request, it was pretty hard to analyze for me in the current form.

There is a mix of inner/outer scope here.

What do you mean? All the return paths look the same to me, so they can be just deduplicated.

I would leave it like that and revisit later.

This really shouldn't take more than 10 minutes and will make this PR easier to review, so I don't see a reason to postpone it to a separate PR.

---

Pal/src/host/Linux-SGX/db_files.c, line 128 at r3 (raw file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

Without these share permissions, the write operation on this fail to succeed.

How is this possible? The only path the pal_share value takes is to ocall_open()'s mode argument, then to ms->ms_mode, then to sgx_ocall_open() and then directly to the open syscall on the host. But open ignores the mode argument if we aren't creating a new file.
What am I missing?

---

Pal/src/host/Linux-SGX/db_files.c, line 130 at r3 (raw file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

Why is this then OR-ed in

gramine/Pal/src/host/Linux-SGX/db_files.c

Line 42 in 487969c

int flags = PAL_ACCESS_TO_LINUX_OPEN(pal_access) |

?

Because only PAL_ACCESS_TO_LINUX_OPEN() contains the access mode enum, so it's fine to OR them. Here you are ORing different access flags between each other, which doesn't make sense, because they are enum values (stored inside a larger bitmask), not bitflags.

---

Pal/src/host/Linux-SGX/db_files.c, line 827 at r3 (raw file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

We already discussed this in an earlier discussion. Please see that discussion. There is a mix of inner/outer scope here. I would leave it like that and revisit later.

Ok, let's move the discussion to that thread.

---

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 378 at r3 (raw file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

It would not. It would register the file as part of rename, but this pf will have the older context.

Sorry, but I don't understand your reply. You say "It would not", but at the same time you say "this pf will have the older context".
This seems like a bug in this PR.

---

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 1341 at r3 (raw file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

Done.

Not done, you just moved this check instead of removing it.

[svenkata9]

Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

rename_unlink tests should already cover this PR

It doesn't, currently it's disabled for PF, as you said. So, currently there's no test verifying this feature.

but since there are issues with some of the tests when it comes to Protected FS

AFAIU, Paweł disabled it just because his PR was based on master (not this PR), so the PF part couldn't work. And he expected you to enable this part of the test here.

Please see earlier discussions.

I can't find any discussion which concludes that it's fine to merge this without tests.

Sorry, I thought that is in reviewable, but it was a discussion that happed with Pawel on Slack. ("I think we can keep this whole test (rename_unlink) skipped for now, but add a TODO somewhere (SGX db_files.c) about what exactly doesn't work")

---

---

-- commits, line 2 at r3:

Previously, mkow (Michał Kowalczyk) wrote…

The only bug we had was that DkStreamChangeName() wasn't loudly failing when the app tried to rename a protected file. Before your PR renaming protected files was explicitly not supported and not considered in the security analysis.

Maybe the context has changed now and it is indeed a bug. Anyway, it is fixing the issue to make it work for a workload.

---

Pal/src/host/Linux-SGX/db_files.c, line 815 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

+1 to the Dmitrii's request, it was pretty hard to analyze for me in the current form.

There is a mix of inner/outer scope here.

What do you mean? All the return paths look the same to me, so they can be just deduplicated.

I would leave it like that and revisit later.

This really shouldn't take more than 10 minutes and will make this PR easier to review, so I don't see a reason to postpone it to a separate PR.

new_normpath is internal scope and new_path is external scope. We can take this up later.

---

Pal/src/host/Linux-SGX/db_files.c, line 128 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

How is this possible? The only path the pal_share value takes is to ocall_open()'s mode argument, then to ms->ms_mode, then to sgx_ocall_open() and then directly to the open syscall on the host. But open ignores the mode argument if we aren't creating a new file.
What am I missing?

I have removed it now

---

Pal/src/host/Linux-SGX/db_files.c, line 130 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Because only PAL_ACCESS_TO_LINUX_OPEN() contains the access mode enum, so it's fine to OR them. Here you are ORing different access flags between each other, which doesn't make sense, because they are enum values (stored inside a larger bitmask), not bitflags.

Done.

---

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 378 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Sorry, but I don't understand your reply. You say "It would not", but at the same time you say "this pf will have the older context".
This seems like a bug in this PR.

get_protected_file will not return wrong files. Because the get_protected_path for the new name will be registered. When I said this pf context, I was referring to the one in this code - so it is fine.

---

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 1341 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Not done, you just moved this check instead of removing it.

I don't see a problem in having this check. Anyway, you preferred to remove it, and I have done that.

[mkow]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @svenkata9)

a discussion (no related file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

Sorry, I thought that is in reviewable, but it was a discussion that happed with Pawel on Slack. ("I think we can keep this whole test (rename_unlink) skipped for now, but add a TODO somewhere (SGX db_files.c) about what exactly doesn't work")

So, your implementation doesn't pass this test? Could you fix it, so that it's actually tested? Right now we would have literally zero coverage for this feature in our tests.

---

---

-- commits, line 2 at r3:

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

Maybe the context has changed now and it is indeed a bug. Anyway, it is fixing the issue to make it work for a workload.

This way you could classify all our features as bugfixes, as they are fixing some "issues with workloads" ;) Marking this change as a bugfix is misleading for whoever will read the commit log in the future.

---

Pal/src/host/Linux-SGX/db_files.c, line 815 at r1 (raw file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

new_normpath is internal scope and new_path is external scope. We can take this up later.

Why is this a problem? You can just move new_normpath definition to the outer scope.

---

Pal/src/host/Linux-SGX/db_files.c, line 130 at r3 (raw file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

Done.

Not done, now you're erasing all the other flags.

---

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 378 at r3 (raw file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

get_protected_file will not return wrong files. Because the get_protected_path for the new name will be registered. When I said this pf context, I was referring to the one in this code - so it is fine.

Yes, both paths are registered, but if you aren't updating the entries then you'll get the protected_file struct for the old file, no?

---

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 1341 at r3 (raw file):
It's not a problem, but it makes little sense. You don't e.g. check if new_path == (void*)-1 or use something like PROBE_MEM_READ() on it to check if it actually points to some memory. It's the caller's responsibility to not pass garbage in the arguments and we can't start every function in our code with 10 checks for every possible caller's error.

Anyway, you preferred to remove it, and I have done that.

Yup, now it's removed, so I'm resolving.

[svenkata9]

Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

So, your implementation doesn't pass this test? Could you fix it, so that it's actually tested? Right now we would have literally zero coverage for this feature in our tests.

In fact, in the earlier PR, I did have a separate test added for this, but as per the review comments, I have removed since Pawel has added this rename_unlink tests. My implementation will pass two of the five tests that is part of rename_unlink, but since there are more issues, and as per the comments, I have let it remain in skipped state. We will revisit once this PR is merged and maybe selectively enable.

---

---

-- commits, line 2 at r3:

Previously, mkow (Michał Kowalczyk) wrote…

This way you could classify all our features as bugfixes, as they are fixing some "issues with workloads" ;) Marking this change as a bugfix is misleading for whoever will read the commit log in the future.

Why this discussion, and you alone disagree? Where was this called out as a feature. It came as a bug from the customers, and we are addressing it? Let it remain as a bug fix for now, or if you wish, please reword it during the merge.

---

Pal/src/host/Linux-SGX/db_files.c, line 815 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why is this a problem? You can just move new_normpath definition to the outer scope.

On one hand, we say move everything together, and on the other hand, the other part of the code still has such cases. see file_open or other part of the code. the new_normpath is very specific to pf and doesn't make sense defining it outside the scope for pf

---

Pal/src/host/Linux-SGX/db_files.c, line 130 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Not done, now you're erasing all the other flags.

This is a specific rename case which is custom. So, the flags are updated to RDWR - why not overwrite them? What else do you suggest? The file has already been checked for presence and is a pf.

---

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 378 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Yes, both paths are registered, but if you aren't updating the entries then you'll get the protected_file struct for the old file, no?

There is a bigger issue in g_protected_files maintenance and that is what we need to address. As mentioned in the earlier comments, we will take it up earlier post this rename case.

[dimakuv]

Reviewed all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow)

a discussion (no related file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

In fact, in the earlier PR, I did have a separate test added for this, but as per the review comments, I have removed since Pawel has added this rename_unlink tests. My implementation will pass two of the five tests that is part of rename_unlink, but since there are more issues, and as per the comments, I have let it remain in skipped state. We will revisit once this PR is merged and maybe selectively enable.

Quick content for Michal: we recently revealed a whole bunch of problems with Protected Files. This PR tries to fix one particular issue (missing renaming support for PFs) in one particular use case (from some customer).

In reality, a proper fix would be to re-write our PF implementation from scratch. So at this point, we have two possibilities:

1. Merge this PR more or less intact, because it covers that particular use case. And later have a comprehensive refactoring of the PF implementation.
2. Refactor PF implementation from the beginning, which will also incorporate modifications from this PR.

I understand that you want to do 2, but we currently don't have the bandwidth or anyone assigned to do this big refactoring. So we either do 1, or:
3. Keep everything as-is currently and not merge this PR.

In my opinion, we should do 1, because it at least fixes one bug for one customer and moves us closer to the "proper" renaming support.

---

---

-- commits, line 2 at r3:

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

Why this discussion, and you alone disagree? Where was this called out as a feature. It came as a bug from the customers, and we are addressing it? Let it remain as a bug fix for now, or if you wish, please reword it during the merge.

We will reword it during the merge. I also prefer Michal's text.

[mkow]

Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @svenkata9)

a discussion (no related file):

I did have a separate test added for this, but as per the review comments, I have removed since Pawel has added this rename_unlink tests.

Yes, I've seen it. I believe that Paweł's intention was that you can drop this test because you'll be able to enable his. We need to test this change somehow, if it's completely lacking coverage then tools like UBSan or ASan won't be able to check it.

We will revisit once this PR is merged and maybe selectively enable.

Why can't you selectively enable them now?

we recently revealed a whole bunch of problems with Protected Files

Yup, I'm aware of this and have seen these discussions.

In reality, a proper fix would be to re-write our PF implementation from scratch.

I don't agree with this, look e.g. at the problem with get_protected_file() - it looks like something which was introduced in this PR, that g_protected_files entries aren't swapped on rename and have now dangling links to other files (if I understand the change correctly). I know that we have refcounting problems in PF, but that's not a good justification to introduce even new kinds of bugs to it. And this one looks like something which was introduced in this PR and can be fixed without rewriting PF.

@dimakuv: Anyways, this discussion thread is not really about fixing bugs in this PR (for this see other discussions below), but about having literally 0 tests for it.

---

---

-- commits, line 2 at r3:

Why this discussion

From my review? I'm sorry, but I really don't understand your resistance here, it's just a trivial commit message change for uniformity with the rest of our development. What's the problem with this?

and you alone disagree?

That's quite normal in reviews, that different people notice different problems. The fact that only I pointed it out doesn't mean that everyone else who did review "votes" for the current version, they may have just missed it.

---

Pal/src/host/Linux-SGX/db_files.c, line 815 at r1 (raw file):
file_open is quite ugly and wasn't refactored in years, it's not a good example. We should write all new code better than this.

the new_normpath is very specific to pf and doesn't make sense defining it outside the scope for pf

Unfortunately we're using C and this is a very common thing you have to do (defining variables as "function-global"). We don't have RAII here, so if you want to have manageable cleanup you unfortunately need to move the variables to the outer scope, and that's what we do in all new code where you need some cleanup.

---

Pal/src/host/Linux-SGX/db_files.c, line 130 at r3 (raw file):
But it's the caller who passed PAL_OPTION_RENAME and also chose the flags. And now you're just silently erasing them.

What else do you suggest?

The same as I suggested from the beginning of this thread, that you just set this enum (which is encoded in a bitfield) to O_RDWR. There's no need to erase other values.

---

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 378 at r3 (raw file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

There is a bigger issue in g_protected_files maintenance and that is what we need to address. As mentioned in the earlier comments, we will take it up earlier post this rename case.

Wait, isn't this a problem that you introduced in this very PR, not a PF bug? You implemented swapping of files, but you don't swap their entries in g_protected_files, leaving dangling references. Previously there was no way to move protected files.

[dimakuv]

Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @svenkata9)

---

Pal/src/host/Linux-SGX/db_files.c, line 815 at r1 (raw file):

file_open is quite ugly and wasn't refactored in years, it's not a good example. We should write all new code better than this.

+1 to this. I started refactoring this madness, but completed only 50% of the task. The code we have for files/streams/pipes in Linux-SGX PAL is still a nightmare, and we need to continue making it better.

---

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 378 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Wait, isn't this a problem that you introduced in this very PR, not a PF bug? You implemented swapping of files, but you don't swap their entries in g_protected_files, leaving dangling references. Previously there was no way to move protected files.

Damn, I just realized that we have the same problem with g_trusted_file_list. Please note that this list stores both allowed and trusted files. And if trusted files cannot be renamed by their nature (they are immutable, and everything regarding them is immutable), allowed files can be renamed. And if a file was declared as sgx.allowed_files = "file:foo", then if this file is renamed and then accessed, it won't appear in the g_trusted_file_list, and Gramine will refuse to open it. (One can circumvent this with specifying an allowed dir, then all files renamed in the same dir will work fine. But this is just a workaround.)

Ok, this PR (alongside a couple other issues raised by Borys and Pawel) revealed that our current implementation of allowed/trusted/protected files is just broken in so many ways. We need proper refactoring of this whole subsystem, as a series of 5-10 PRs that fix this mess piece by piece.

Sorry, @svenkata9, but we'll need to start complete refactoring. Too many bugs uncovered. It's just simpler to burn the current code and rewrite it...

[mkow]

I'm closing this PR, as this solution to the problem won't be merged (because it doesn't really work and is too hacky). We're starting a big redesign of protected files, which should fix issues like this one at the core (see #371).