Add first version of encrypted mounts

[pwmarcz]

Description of the changes

This adds type = "encrypted" mounts according to the design in #371. They do not have all the features of protected files yet, but the change is already pretty big.

How to test this PR?

I will eventually convert all the tests that use protected files. For now, the send_handle test covers a simple scenario: opening, writing and then reading a file. (Also, it already correctly sends the handle to child process even if the file is deleted - something the old protected files could not do).

---

This change is 

[dimakuv]

Reviewed 11 of 11 files at r1, 10 of 10 files at r2, 1 of 1 files at r3, 3 of 3 files at r4, 11 of 11 files at r5, all commit messages.
Reviewable status: all files reviewed, 27 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @pwmarcz)

---

LibOS/shim/include/shim_fs_encrypted.h, line 8 at r5 (raw file):

/*
 * This module implements encrypted files. It is a wrapper around the platform-independent
 * `protected_files` module, and PAL handles.

I need some kind of a diagram that explains the relationships between:

• FS mounts (that each "encrypted" mount points to the encryption key in data)
• inodes (that each "encrypted" inode points to the shim_encrypted_file object in data)
• the relationship between shim_handle, shim_encrypted_file, pf_context, pal_handle

Currently, it is very hard for me to understand which objects are created together, which objects are destroyed together, which objects have which lifetimes, etc.

---

LibOS/shim/include/shim_fs_encrypted.h, line 106 at r5 (raw file):

/*
 * \brief Deallocate an encrypted file.

Does it basically free(enc)? It's a counterpart to encrypted_file_new_open() and encrypted_file_new_create(), right?

No need to change anything, just double-checking.

---

LibOS/shim/include/shim_fs_encrypted.h, line 115 at r5 (raw file):

 * \brief Increase the use count of an encrypted file.
 *
 * This increases `use_count`, and makes sure that the file is open.

...and makes sure that the file is open -- I don't get this part.

---

LibOS/shim/include/shim_fs_encrypted.h, line 122 at r5 (raw file):

 * \brief Decrease the use count of an encrypted file.
 *
 * This decreases `use_count`, and closes the file if it reaches 0.

Why can't encrypted_file_destroy() be merged into this function (when use_count == 0)?

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 32 at r5 (raw file):

        int ret = DkStreamRead(pal_handle, offset + buffer_offset, &count, buffer + buffer_offset,
                               /*source=*/NULL, /*size=*/0);
        if (ret == PAL_ERROR_INTERRUPTED)

Shouldn't this be -PAL_ERROR_INTERRUPTED (with minus)?

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 62 at r5 (raw file):

        int ret = DkStreamWrite(pal_handle, offset + buffer_offset, &count,
                                (void*)(buffer + buffer_offset), /*dest=*/NULL);
        if (ret == PAL_ERROR_INTERRUPTED)

Shouldn't this be -PAL_ERROR_INTERRUPTED (with minus)?

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 145 at r5 (raw file):

#endif

static int encrypted_file_open(struct shim_encrypted_file* enc, PAL_HANDLE pal_handle, bool create,

I desperately need a comment explaining the relationships between shim_encrypted_file, PAL_HANDLE, pf_context and shim_handle. Which of these are 1:1, which are shared among objects?

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 166 at r5 (raw file):

    if (ret < 0) {
        log_warning("%s: DkStreamAttributesQueryByHandle failed: %d", __func__, ret);
        DkObjectClose(pal_handle);

What is there was a pal_handle provided as a func argument? Then this function's failure has a side-effect of closing this file handle. Was that your intention?

I see that you don't close the PAL handle on the below error path, so looks like an inconsistency.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 178 at r5 (raw file):

        return -EACCES;
    }
    pf_status_t pfs = pf_open(pal_handle, path, size, PF_FILE_MODE_READ | PF_FILE_MODE_WRITE,

I understand that the underlying PAL handle should be opened for read + write. But why do you set the PF-specific mode for read + write? I think at this level, you can specify only the correct subset of modes.

Or is it because this pf context is shared among several shim handles/fds -- some of them may be opened for read, some for write?

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 218 at r5 (raw file):

                     &cb_random, cb_debug_ptr);

    /* Parse `fs.keys.*` */

keys -> insecure__keys

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 250 at r5 (raw file):

            goto out;
        }
        assert(key_str);

Please add an empty line after this assert.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 296 at r5 (raw file):

    struct shim_encrypted_files_key* key = get_or_create_key(name);

    /* TODO: load special keys (MRENCLAVE, MRSIGNER) here */

It shouldn't happen in this function (after all, this func may never be called if there are no fs.insecure__keys array in the manifest).

Instead, special keys should be loaded during init_encrypted_files().

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 321 at r5 (raw file):

        if (hi < 0 || lo < 0) {
            log_warning("%s: unexpected character encountered", __func__);
            free(key);

This is a completely unexpected side-effect. Please free the key in the callers (on this func's failure) instead of here.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 334 at r5 (raw file):

}

static int encrypted_file_new(const char* uri, struct shim_encrypted_files_key* key,

Can we rename it to encrypted_file_alloc()? I'm getting confused by these _new, _new_open, _new_create.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 395 at r5 (raw file):

void encrypted_file_destroy(struct shim_encrypted_file* enc) {
    assert(enc->use_count == 0);

Don't you want to assert also that enc->pf == NULL and enc->pal_handle == NULL?

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 407 at r5 (raw file):

    }
    assert(!enc->pf);
    int ret = encrypted_file_open(enc, /*pal_handle=*/NULL, /*create=*/false, /*share_flags=*/0);

Confusing... Why would anyone use such a flow (call encrypted_file_get() without an initial encrypted_file_new_open())?

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 419 at r5 (raw file):

    enc->use_count--;
    if (enc->use_count == 0) {
        encrypted_file_close(enc);

Why doesn't this call encrypted_file_destroy() immediately?

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 429 at r5 (raw file):

    if (PF_FAILURE(pfs)) {
        log_warning("%s: pf_flush failed: %s", __func__, pf_strerror(pfs));
        return -EACCES;

This is a bit stupid that we don't have a pf_failure_to_unix_errno() function. Not blocking, but we may want to get rid of this pf_status_t and just use normal UNIX error codes.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 436 at r5 (raw file):

int encrypted_file_read(struct shim_encrypted_file* enc, void* buf, size_t buf_size,
                        file_off_t offset, size_t* out_count) {
    if (offset < 0)

Don't you want to add assert(enc->pf);?

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 453 at r5 (raw file):

int encrypted_file_write(struct shim_encrypted_file* enc, const void* buf, size_t buf_size,
                         file_off_t offset, size_t* out_count) {
    if (offset < 0)

Don't you want to add assert(enc->pf);?

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 463 at r5 (raw file):

        return -EACCES;
    }
    /* We never write less than `buf_size */

Forgot the closing backtick

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 516 at r5 (raw file):

    /* HACK: Instead of `enc->key`, send the key name */
    DO_CP(str, enc->key->name, &new_enc->key);

Smart. I guess the cleaner approach would be to introduce a separate CP_FUNC and RS_FUNC for struct shim_encrypted_files_key, but you decided that it's too much code overhead?

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 516 at r5 (raw file):

    /* HACK: Instead of `enc->key`, send the key name */
    DO_CP(str, enc->key->name, &new_enc->key);

Actually, something doesn't add up here. This will work for the keys specified in the manifest (because these keys will be already created and set to correct values during init_encrypted_files()), but how can this work for dynamically-supplied keys? In that case, the parent will have some "ephemeral" entries in the g_keys list, but these entries will not be forwarded to the child. And during restore, the child will re-create the keys with name, but these keys won't have any values set.

I think you can't get away with the current hack. You actually need to copy the g_keys list to the child.

I even wrote the test for exactly this:

gramine/CI-Examples/ra-tls-secret-prov/src/secret_prov_pf_client.c

Line 59 in 6020888

/* execvp() is only for testing purposes: to validate that secret provisioning happens only once

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 539 at r5 (raw file):

    CP_REBASE(enc->uri);

    /* `enc->key` was set to the key name, retrieve the key */

See my other comment.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 548 at r5 (raw file):

    /* If the file was used, recreate `enc->pf` based on the PAL handle */
    assert(!enc->pf);
    if (enc->use_count > 0) {

How is it possible that the file is not used? Why isn't it destroyed immediately on use_count == 0?

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 550 at r5 (raw file):

    if (enc->use_count > 0) {
        assert(enc->pal_handle);
        ret = encrypted_file_open(enc, enc->pal_handle, /*create=*/false, /*share_flags=*/0);

This is the only caller who uses encrypted_file_open() with the PAL handle. Maybe duplicate that function into two independent ones? And call the encrypted_file_reopen() one here?

---

LibOS/shim/src/fs/chroot/encrypted.c, line 16 at r5 (raw file):

 * - rename
 * - mmap
 * - truncate (the protected_files module only supports extending the file)

But you have chroot_encrypted_truncate() already. Do you mean that you'll need to change the common/src/protected_files/ sources to support truncation, not only extending?

[pwmarcz]

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

---

LibOS/shim/include/shim_fs_encrypted.h, line 8 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I need some kind of a diagram that explains the relationships between:

• FS mounts (that each "encrypted" mount points to the encryption key in data)
• inodes (that each "encrypted" inode points to the shim_encrypted_file object in data)
• the relationship between shim_handle, shim_encrypted_file, pf_context, pal_handle

Currently, it is very hard for me to understand which objects are created together, which objects are destroyed together, which objects have which lifetimes, etc.

I am no good at diagrams, but I added a longer explanation (to the encrypted.c file which actually manages these objects). Does that help?

---

LibOS/shim/include/shim_fs_encrypted.h, line 106 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Does it basically free(enc)? It's a counterpart to encrypted_file_new_open() and encrypted_file_new_create(), right?

No need to change anything, just double-checking.

Yes.

---

LibOS/shim/include/shim_fs_encrypted.h, line 115 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

...and makes sure that the file is open -- I don't get this part.

Changed to "and opens the file if use_count was 0".

---

LibOS/shim/include/shim_fs_encrypted.h, line 122 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why can't encrypted_file_destroy() be merged into this function (when use_count == 0)?

Because the file can be open again later. I added "Note that the file can be open and closed multiple times before it's destroyed" above.

This is the reason why I'm calling the variable use_count, not ref_count: it is not tied to the object's lifetime. Maybe the operations also should be called something else than get/put? But I can't think of a better name.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 32 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Shouldn't this be -PAL_ERROR_INTERRUPTED (with minus)?

Right. Fixed.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 62 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Shouldn't this be -PAL_ERROR_INTERRUPTED (with minus)?

Fixed.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 145 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I desperately need a comment explaining the relationships between shim_encrypted_file, PAL_HANDLE, pf_context and shim_handle. Which of these are 1:1, which are shared among objects?

• A PAL_HANDLE and pf_context_t are owned by shim_encrypted_file, as long as the file is open.
• A shim_handle doesn't keep any data.

Let me know if the comments I just added clarify the situation.

(This module doesn't know anything about shim_handle or the rest of the filesystem, btw, I hope that's clear).

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 166 at r5 (raw file):

What is there was a pal_handle provided as a func argument?

This is for checkpointing. I added a comment.

Then this function's failure has a side-effect of closing this file handle. Was that your intention?

Yes, in this case, we fail to start the process anyway.

I see that you don't close the PAL handle on the below error path, so looks like an inconsistency.

Right. Fixed.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 178 at r5 (raw file):

Or is it because this pf context is shared among several shim handles/fds -- some of them may be opened for read, some for write?

Yes.

And renaming (which we do not support yet, but we're going to) should also work on read-only handles.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 218 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

keys -> insecure__keys

Done.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 250 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please add an empty line after this assert.

Done.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 296 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

It shouldn't happen in this function (after all, this func may never be called if there are no fs.insecure__keys array in the manifest).

Instead, special keys should be loaded during init_encrypted_files().

This function is also called during mount. I was planning to do it like this:

• the user specifies a key named _sgx_mrenclave
• the mount callback (chroot_encrypted_mount) will call get_encrypted_files_key("_sgx_mrenclave"),
• here, we notice the _ prefix, and call a PAL function like DkGetSpecialKey("_sgx_mrenclave").

In init_encrypted_files we don't know which keys to load: we could parse the mounts table again, have a hardcoded list of all special keys in LibOS, or ask PAL for a list of all supported special keys. Neither of these option sounds better to me.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 321 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This is a completely unexpected side-effect. Please free the key in the callers (on this func's failure) instead of here.

Yeah, it makes no sense, I think it's a leftover from an earlier version.

Still, we leave the key in a messy state here. I rewrote this function to use a local variable, and update the key on success.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 334 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Can we rename it to encrypted_file_alloc()? I'm getting confused by these _new, _new_open, _new_create.

OK. I renamed new, new_open, new_create to new, open, create, and renamed the internal open/close operations to internal_open, internal_close.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 395 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Don't you want to assert also that enc->pf == NULL and enc->pal_handle == NULL?

Good idea. Done.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 407 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Confusing... Why would anyone use such a flow (call encrypted_file_get() without an initial encrypted_file_new_open())?

They could close the file in the meantime (encrypted_file_new_open, encrypted_file_put, then encrypted_file_get). See also the discussion of get/put above.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 419 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why doesn't this call encrypted_file_destroy() immediately?

The user can call encrypted_file_get again. See also the discussion of get/put above.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 429 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This is a bit stupid that we don't have a pf_failure_to_unix_errno() function. Not blocking, but we may want to get rid of this pf_status_t and just use normal UNIX error codes.

Maybe... But the error conditions (see protected_files.h) are very much specific to this library.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 436 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Don't you want to add assert(enc->pf);?

Done.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 453 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Don't you want to add assert(enc->pf);?

Done.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 463 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Forgot the closing backtick

Fixed.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 516 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Actually, something doesn't add up here. This will work for the keys specified in the manifest (because these keys will be already created and set to correct values during init_encrypted_files()), but how can this work for dynamically-supplied keys? In that case, the parent will have some "ephemeral" entries in the g_keys list, but these entries will not be forwarded to the child. And during restore, the child will re-create the keys with name, but these keys won't have any values set.

I think you can't get away with the current hack. You actually need to copy the g_keys list to the child.

I even wrote the test for exactly this:

gramine/CI-Examples/ra-tls-secret-prov/src/secret_prov_pf_client.c

Line 59 in 6020888

/* execvp() is only for testing purposes: to validate that secret provisioning happens only once

You're right. Since we don't have /dev/attestation yet, I'll add this as a TODO.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 516 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Smart. I guess the cleaner approach would be to introduce a separate CP_FUNC and RS_FUNC for struct shim_encrypted_files_key, but you decided that it's too much code overhead?

As you say below, in this case it's not enough. I added a TODO.

But actually, for immutable data, I think this is a perfectly valid approach (transfer the name instead of the object). It's only our checkpointing system that makes it hacky (since the easiest way is to abuse a pointer field to store a pointer to something else...)

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 548 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

How is it possible that the file is not used? Why isn't it destroyed immediately on use_count == 0?

See the discussion of get/put above.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 550 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This is the only caller who uses encrypted_file_open() with the PAL handle. Maybe duplicate that function into two independent ones? And call the encrypted_file_reopen() one here?

I guess I could, but I just renamed that function to encrypted_file_internal_open. I do not want to create a monstrous name such as encrypted_file_internal_open_with_pal_handle, and I don't know how better to name these. I'd leave it like this (and allow it to be slightly messy because it's an "internal" function), unless you have a better suggestion for names.

"reopen" is not a good name, reopening a file is a valid scenario outside of checkpointing (since use_count can drop to 0 and then go back up).

---

LibOS/shim/src/fs/chroot/encrypted.c, line 16 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

But you have chroot_encrypted_truncate() already. Do you mean that you'll need to change the common/src/protected_files/ sources to support truncation, not only extending?

That's right. I edited this comment to make that more clear.

[dimakuv]

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners

---

LibOS/shim/include/shim_fs_encrypted.h, line 8 at r5 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

I am no good at diagrams, but I added a longer explanation (to the encrypted.c file which actually manages these objects). Does that help?

Yes, this helps. Thanks.

---

LibOS/shim/include/shim_fs_encrypted.h, line 122 at r5 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Because the file can be open again later. I added "Note that the file can be open and closed multiple times before it's destroyed" above.

This is the reason why I'm calling the variable use_count, not ref_count: it is not tied to the object's lifetime. Maybe the operations also should be called something else than get/put? But I can't think of a better name.

I understand now. The names are a bit misleading, but I also cannot come up with anything better.

---

LibOS/shim/include/shim_fs_encrypted.h, line 40 at r6 (raw file):

/*
 * Represents a specific encrypted file. The file is open as long as `use_count` is greater than 0.
 * Note that the file can be open and closed multiple times before it's destroyed.

Ok, I'm starting to understand the logic behind the separation into different files.

This file (shim_fs_encrypted.h) is an FS-subsystem-agnostic wrapper for common/protected-files:

• it adds an additional layer of shim_encrypted_files_key around pf_key, and
• it adds an additional layer of shim_encrypted_file around pf_context_t + PAL handle

The former struct allows to put all PF keys in a linked list (as well as tag them with names). The latter struct allows to keep the association <PF key - filename - PF context - PAL handle> in one logical object.

Or, mapping it to the current PAL implementation of PFs:

• the former struct corresponds to (and makes more generic) https://github.com/gramineproject/gramine/blob/master/Pal/src/host/Linux-SGX/protected-files/protected_files.h#L30-L33
• the latter struct corresponds to https://github.com/gramineproject/gramine/blob/master/Pal/src/host/Linux-SGX/enclave_pf.h#L54-L62

This separation is also why you don't mention inodes here -- you want this header file to be completely FS-agnostic.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 296 at r5 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

This function is also called during mount. I was planning to do it like this:

• the user specifies a key named _sgx_mrenclave
• the mount callback (chroot_encrypted_mount) will call get_encrypted_files_key("_sgx_mrenclave"),
• here, we notice the _ prefix, and call a PAL function like DkGetSpecialKey("_sgx_mrenclave").

In init_encrypted_files we don't know which keys to load: we could parse the mounts table again, have a hardcoded list of all special keys in LibOS, or ask PAL for a list of all supported special keys. Neither of these option sounds better to me.

Got it. Yes, the mount callback is a good idea, now I understand.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 429 at r5 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Maybe... But the error conditions (see protected_files.h) are very much specific to this library.

This is also true. Still, having a pf_failure_to_unix_errno() function instead of -EACCES will definitely be an improvement, even if we won't be able to mount all PF error conditions to UNIX codes.

---

LibOS/shim/src/fs/chroot/encrypted.c, line 17 at r6 (raw file):

 * - Inodes (`shim_inode`, for regular files) hold a `shim_encrypted_file` object. This object lives
 *   as long as the inode, but is kept *open* only as long as there are `shim_handle` objects
 *   corresponding to it. We use `encrypted_file_{get,put}` operations to maintain that invariant.

This paragraph is what I was missing. Thanks for the explanation!

[pwmarcz]

Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners

---

LibOS/shim/include/shim_fs_encrypted.h, line 40 at r6 (raw file):
Right. This module is an equivalent of enclave_pf.c, and it's not dependent on the rest of LibOS (well, except for manifest parsing).

the former struct corresponds to (and makes more generic) https://github.com/gramineproject/gramine/blob/master/Pal/src/host/Linux-SGX/protected-files/protected_files.h#L30-L33

Yes. These globals are actually also part of enclave_pf.c, and probably were declared in protected_files.h by mistake. We'll remove them soon anyway.

[mkow]

Reviewed 11 of 11 files at r1, 10 of 10 files at r2, 1 of 1 files at r3, 3 of 3 files at r4, 8 of 11 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

---

LibOS/shim/include/shim_fs.h, line 899 at r6 (raw file):

 * If `type` is KEEP_URI_PREFIX, we keep the URI prefix from mount URI.
 */
int chroot_dentry_uri(struct shim_dentry* dent, mode_t type, char** out_uri);

It was like this before, but this argument was very confusing to me, I couldn't easily tell if it's a standard POSIX enum/type or ours, and what values can be used there. Seems to be some weird mix of our KEEP_URI_PREFIX and standard S_IF* values, which sounds problematic, because we can't ensure the lack of collisions (i.e. that none of the S_IF* values is 0).

Code quote (i):

#define KEEP_URI_PREFIX 0

Code quote (ii):

mode_t type

Code quote (iii):

If `type` is KEEP_URI_PREFIX

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 218 at r5 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Done.

We also need to add this to the security warnings printed at the start.

[pwmarcz]

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

---

LibOS/shim/include/shim_fs.h, line 899 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

It was like this before, but this argument was very confusing to me, I couldn't easily tell if it's a standard POSIX enum/type or ours, and what values can be used there. Seems to be some weird mix of our KEEP_URI_PREFIX and standard S_IF* values, which sounds problematic, because we can't ensure the lack of collisions (i.e. that none of the S_IF* values is 0).

OK. This is actually needed in exactly one case, so I'll remove this KEEP_URI_PREFIX entirely.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 218 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

We also need to add this to the security warnings printed at the start.

It's probably a mistake that these warnings are implemented for Linux-SGX specifically... and actually we don't warn on sgx.insecure__protected_files_key at all. Anyway, added both.

[mkow]

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

---

LibOS/shim/include/shim_fs_encrypted.h, line 46 at r7 (raw file):

 */
struct shim_encrypted_file {
    unsigned int use_count;

Why not size_t?

Code quote:

unsigned int

---

LibOS/shim/include/shim_fs_encrypted.h, line 67 at r7 (raw file):

 * Sets `*out_key` to a key with a given name. Note that the key might not be set yet (see
 * `struct shim_encrypted_files_key`).
 */

Could you document the object ownership here? I guess it doesn't pass the ownership of **out_key to the caller?

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 218 at r5 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

It's probably a mistake that these warnings are implemented for Linux-SGX specifically... and actually we don't warn on sgx.insecure__protected_files_key at all. Anyway, added both.

Hard to say, some PALs technically may not be security-relevant (e.g. Linux PAL now, or some compat PALs used to run Linux ELFs outside of Linux). Also, on a different TEE the set of insecure configs may be different.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 107 at r7 (raw file):

static pf_status_t cb_aes_gcm_encrypt(const pf_key_t* key, const pf_iv_t* iv, const void* aad,
                                      size_t aad_size, const void* input, size_t input_size,
                                      void* output, pf_mac_t* mac) {

Please add out_ prefix to the out args.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 119 at r7 (raw file):

static pf_status_t cb_aes_gcm_decrypt(const pf_key_t* key, const pf_iv_t* iv, const void* aad,
                                      size_t aad_size, const void* input, size_t input_size,
                                      void* output, const pf_mac_t* mac) {

ditto

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 154 at r7 (raw file):

    assert(!enc->pf);

    const char* path = enc->uri + static_strlen(URI_PREFIX_FILE);

Could you assert here that enc->uri indeed starts with URI_PREFIX_FILE?

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 301 at r7 (raw file):

    lock(&g_keys_lock);

    struct shim_encrypted_files_key* key = get_or_create_key(name);

Shouldn't we return an error if key == NULL?

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 356 at r7 (raw file):

    enc->uri = strdup(uri);
    if (!uri) {

should be enc->uri

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 535 at r7 (raw file):

     *
     * TODO: Once we can change the keys through `/dev/attestation`, this will not be enough: we'll
     * need to copy the current key value. Convert this to `DEFINE_CP_FUNC(encrypted_file_key)` etc.

Hmm, so, this doesn't work if the PF key was set after remote attestation? This is the most standard use for PF, I think.
If so, I assume this fix is just left for the other part of the rewrite?

---

LibOS/shim/src/fs/chroot/encrypted.c, line 17 at r7 (raw file):

 * - Inodes (`shim_inode`, for regular files) hold a `shim_encrypted_file` object. This object lives
 *   as long as the inode, but is kept *open* only as long as there are `shim_handle` objects
 *   corresponding to it. We use `encrypted_file_{get,put}` operations to maintain that invariant.

Actually, why doesn't it have the same lifetime as its inode? Optimization to not keep the handle open unnecessarily?

[pwmarcz]

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

---

LibOS/shim/include/shim_fs_encrypted.h, line 46 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why not size_t?

Makes sense. Changed.

---

LibOS/shim/include/shim_fs_encrypted.h, line 67 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Could you document the object ownership here? I guess it doesn't pass the ownership of **out_key to the caller?

Yes. Done.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 107 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Please add out_ prefix to the out args.

These seem to be output and mac. Are you proposing to rename them to out_output and out_mac? I'm not sure if it will make things more readable. Maybe the fact that they're non-const pointers is enough?

(AFAIK, by convention we didn't label output buffers as out_, but mostly parameters that are double-star pointers).

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 301 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Shouldn't we return an error if key == NULL?

Right. Fixed.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 356 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

should be enc->uri

Right, thanks.

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 535 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Hmm, so, this doesn't work if the PF key was set after remote attestation? This is the most standard use for PF, I think.
If so, I assume this fix is just left for the other part of the rewrite?

That's right, this will be needed soon, but I didn't want to make this PR bigger.

---

LibOS/shim/src/fs/chroot/encrypted.c, line 17 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Actually, why doesn't it have the same lifetime as its inode? Optimization to not keep the handle open unnecessarily?

Yes. Right now, we do not delete dentries/inodes for existing files, so we would keep open handles for the whole lifetime of a process. And even a stat is enough to trigger opening such a file (since you need to decrypt the header to check real size).

[mkow]

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

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 107 at r7 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

These seem to be output and mac. Are you proposing to rename them to out_output and out_mac? I'm not sure if it will make things more readable. Maybe the fact that they're non-const pointers is enough?

(AFAIK, by convention we didn't label output buffers as out_, but mostly parameters that are double-star pointers).

Hmm, right, makes sense.

[pwmarcz]

Reviewable status: 29 of 30 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow)

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 154 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Could you assert here that enc->uri indeed starts with URI_PREFIX_FILE?

Done.

[mkow]

Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners

[dimakuv]

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

---

LibOS/shim/src/fs/shim_fs_encrypted.c, line 218 at r5 (raw file):

It's probably a mistake that these warnings are implemented for Linux-SGX specifically... and actually we don't warn on sgx.insecure__protected_files_key at all. Anyway, added both.

Oops, this was my mistake, I forgot to add that key to the warning list. Thanks for noticing this.

---

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

     * The only exception is when this is the root dentry of a "dev:" mount, i.e. a directly mounted
     * device. This is because PAL recognizes a special "dev:tty" device, which needs to be referred
     * to by this exact URI (and "file:tty" will not work).

Well, this probably can be (should be?) fixed at the PAL level, somewhere in db_devices.c. This whole logic is a bit weird, but at least it would be uniform for all PAL object types. Not blocking, just thinking out loud.

---

Pal/src/host/Linux-SGX/db_main.c, line 676 at r10 (raw file):

        goto out;
    if (protected_files_key_str) {
        free(protected_files_key_str);

We have a difference between how we treat log_level_str and protected_files_key_str strings (former is freed in out path, latter is freed immediately). Can we converge on one option? I would prefer the former.

---

Pal/src/host/Linux-SGX/db_main.c, line 739 at r10 (raw file):

    if (enable_sysfs_topo)
        log_always("  - fs.experimental__enable_sysfs_topology = true "
                   "(forwarding sysfs topology from untrusted host to the app)");

I would move this to be the last one. Then we'll have all options related to FS close to each other.

---

Pal/src/host/Linux-SGX/db_main.c, line 742 at r10 (raw file):

    if (protected_files_key)
        log_always("  - sgx.insecure__protected_files_key = \"...\" "

I think you need one more space at the end here, to align with all other strings.

---

Pal/src/host/Linux-SGX/db_main.c, line 746 at r10 (raw file):

    if (encrypted_files_keys)
        log_always("  - fs.insecure__keys.* = \"...\"              "

Why this syntax? Why not fs.insecure__keys = [ ... ]?

[pwmarcz]

Reviewable status: 29 of 30 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: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Well, this probably can be (should be?) fixed at the PAL level, somewhere in db_devices.c. This whole logic is a bit weird, but at least it would be uniform for all PAL object types. Not blocking, just thinking out loud.

Actually, I'm not sure why the "devices" subsystem even exists, since you can open devices using file:, and PAL will still report they are FILE_TYPE_DEV.

---

Pal/src/host/Linux-SGX/db_main.c, line 676 at r10 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

We have a difference between how we treat log_level_str and protected_files_key_str strings (former is freed in out path, latter is freed immediately). Can we converge on one option? I would prefer the former.

Makes sense. I moved the free to the out path.

---

Pal/src/host/Linux-SGX/db_main.c, line 739 at r10 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I would move this to be the last one. Then we'll have all options related to FS close to each other.

Done.

---

Pal/src/host/Linux-SGX/db_main.c, line 742 at r10 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I think you need one more space at the end here, to align with all other strings.

Right. Done.

---

Pal/src/host/Linux-SGX/db_main.c, line 746 at r10 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why this syntax? Why not fs.insecure__keys = [ ... ]?

The keys are a table, not an array. And while this syntax is not documented yet, I think it should be documented as fs.insecure__keys.[KEY] = "...", not as inline table: for a single key, inline table is unnecessary, and for multiple keys, it's annoying to use (due to lack of trailing comma). That's why I don't think { ... } is better here, either.

[dimakuv]

Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

---

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

Previously, pwmarcz (Paweł Marczewski) wrote…

Actually, I'm not sure why the "devices" subsystem even exists, since you can open devices using file:, and PAL will still report they are FILE_TYPE_DEV.

Yeah, this is yet another (rather small) thing that deserves some refactoring.

---

Pal/src/host/Linux-SGX/db_main.c, line 746 at r10 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

The keys are a table, not an array. And while this syntax is not documented yet, I think it should be documented as fs.insecure__keys.[KEY] = "...", not as inline table: for a single key, inline table is unnecessary, and for multiple keys, it's annoying to use (due to lack of trailing comma). That's why I don't think { ... } is better here, either.

Ok, makes sense.

[mkow]

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

---

Pal/src/host/Linux-SGX/db_main.c, line 628 at r11 (raw file):

    bool protected_files_key  = false;
    bool encrypted_files_keys = false;
    bool enable_sysfs_topo    = false;

Could you undo these change which move the sysfs related IFs around? I'm removing them in the sysfs rework and this will generate a lot of conflicts.

[pwmarcz]

Reviewable status: 29 of 30 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

---

Pal/src/host/Linux-SGX/db_main.c, line 628 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Could you undo these change which move the sysfs related IFs around? I'm removing them in the sysfs rework and this will generate a lot of conflicts.

OK. (CC @dimakuv)

[dimakuv]

Reviewed 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @mkow)

---

Pal/src/host/Linux-SGX/db_main.c, line 628 at r11 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

OK. (CC @dimakuv)

No problem for me. Approved.

[dimakuv]

Reviewed 9 of 10 files at r14, 2 of 3 files at r16, 12 of 12 files at r17, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mkow)

[mkow]

Reviewed 9 of 10 files at r14, 2 of 3 files at r16, 12 of 12 files at r17, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved