[LibOS] Move trusted and allowed files logic to LibOS

[dimakuv]

Description of the changes

With this PR, gramine-direct behaves similarly to gramine-sgx: it reads sgx.allowed_files and sgx.trusted_files arrays, as well as sgx.file_check_policy, and applies the corresponding file-access rules. Note that the names of the manifest options are kept the same for backward compatibility; a future PR will introduce new, more appropriate aliases.

Several tests that were previously SGX PAL-specific only are now enabled on gramine-direct as well. All LTP tests, even though they are run under gramine-direct only, check the allowed/trusted files logic now.

As the primary effect of this PR, the SGX PAL code is significantly simplified and is closer to the native Linux (direct) PAL code.

How to test this PR?

All current tests must pass. Should be enough of testing.

---

This change is 

[dimakuv]

Reviewable status: 0 of 23 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "WIP" found in commit messages' one-liners

a discussion (no related file):
One more TODO:

• Remove PAL_OPTION_PASSTHROUGH, it becomes redundant.

---

---

libos/src/libos_init.c line 401 at r1 (raw file):

    RUN_INIT(init_vma);
    RUN_INIT(init_r_debug);

This is an independent fix. Submitted as a separate PR: #1814

---

libos/src/bookkeep/libos_vma.c line 1498 at r1 (raw file):

}

static bool vma_filter_needs_prot_refresh(struct libos_vma* vma, void* arg) {

This code (among a lot of other code) is extracted into this PR: #1818

---

libos/src/fs/shm/fs.c line 168 at r1 (raw file):

}

static int shm_unlink(struct libos_dentry* dent) {

This is an unrelated change (well, slightly related because with this PR, shm filesystem definitely cannot reuse chroot_unlink() as the latter uses chroot_temp_open() which checks for trusted/allowed files).

Create a separate PR for this: #1815

---

libos/test/ltp/manifest.template line 20 at r1 (raw file):

  # many LTP multi-process tests rely on shared-memory IPC via `mmap(MAP_SHARED, </dev/shm fd>)`
  { type = "untrusted_shm", path = "/dev/shm", uri = "dev:/dev/shm" },

This particular change is extracted into PR:

---

libos/test/regression/test_libos.py line 872 at r1 (raw file):

        self.assertIn('TEST OK', stdout)

    @unittest.skip('sigaltstack isn\'t correctly implemented')

This re-enabling of the sigaltstack test was extracted into this PR: #1819

[dimakuv]

Reviewable status: 0 of 23 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "WIP" found in commit messages' one-liners

---

python/gramine-manifest line 33 at r1 (raw file):

    template = infile.read() if infile else string
    manifest = Manifest.from_template(template, define)
    manifest.expand_all_trusted_files(chroot=chroot)

Now that gramine-direct also works with Allowed and Trusted Files, we need to call this expansion function.

There is one side-effect: now both .manifest and .manifest.sgx files have exactly the same contents (previously they only differed in not-expanded vs expanded Trusted Files). This eats memory on the hard disk, so we could symlink .manifest.sgx -> .manifest. But on the other hand, users will most typically disregard .manifest file anyway and use only .manifest.sgx, so making the latter a symlink sounds wrong.

[dimakuv]

Reviewable status: 0 of 45 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "WIP" found in commit messages' one-liners

---

libos/test/regression/test_libos.py line 872 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This re-enabling of the sigaltstack test was extracted into this PR: #1819

Update: I removed this change from here, since something is broken on EDMM. Let's deal with that in the separate PR, here I'm reverting sigaltstack test to be skipped again.

[dimakuv]

Reviewable status: 0 of 45 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "WIP" found in commit messages' one-liners

---

pal/include/pal_internal.h line 296 at r2 (raw file):

int _PalDebugLog(const void* buf, size_t size);

int _PalValidateEntrypoint(const void* buf, size_t size);

This is extracted into separate PR: #1820

[dimakuv]

Reviewable status: 0 of 48 files reviewed, 4 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

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

One more TODO:

• Remove PAL_OPTION_PASSTHROUGH, it becomes redundant.

Done.

---

a discussion (no related file):
This PR is rebased (to get rid of the modifications covered in already-merged dependency PRs). Ready for review.

NOTE: Most of the code is copy-pasted from PAL into LibOS with no to minimal modifications. Unfortunately, I don't know how to highlight it in git diff or Reviewable. Please do manual diffing or just review from scratch.

---

---

pal/src/host/linux-sgx/pal_main.c line 411 at r4 (raw file):

extern size_t g_unused_tcs_pages_num;

static bool need_warn_about_allowed_files_usage(void) {

This part is ugly: we still want to warn users about potentially insecure manifest options in SGX PAL, but these manifest options are located in LibOS. So we have to do some small processing of LibOS-bound options here.

---

pal/src/host/linux-sgx/pal_main.c line 485 at r4 (raw file):

    ret = toml_string_in(g_pal_public_state.manifest_root, "sgx.file_check_policy",
                         &file_check_policy_str);

This part is ugly: we still want to warn users about potentially insecure manifest options in SGX PAL, but these manifest options are located in LibOS. So we have to do some small processing of LibOS-bound options here.

[dimakuv]

Reviewable status: 0 of 58 files reviewed, 4 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

---

CI-Examples/blender/Makefile line 44 at r5 (raw file):

	mkdir -p $@

blender.manifest: blender.manifest.template $(BLENDER_DIR)/blender | $(RUN_DIR)

A side effect I didn't realize before: the build instructions for applications must be modified such that all files must already exist for the .manifest generation.

The same must be applied in the Examples repo, so I'll create the PR there and block on this.

[dimakuv]

Reviewable status: 0 of 58 files reviewed, 4 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

---

CI-Examples/blender/Makefile line 44 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

A side effect I didn't realize before: the build instructions for applications must be modified such that all files must already exist for the .manifest generation.

The same must be applied in the Examples repo, so I'll create the PR there and block on this.

gramineproject/examples#105 -- done here. I'll keep this comment blocking.

[dimakuv]

Reviewable status: 0 of 58 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

---

libos/test/regression/fork_and_access_file.gdb line 17 at r5 (raw file):

  shell echo "WRITING NEW CONTENT IN FORK_AND_ACCESS_FILE_TESTFILE" > fork_and_access_file_testfile

  tbreak die_on_wrong_file_contents

This can be considered an independent change. This LibOS test previously relied on the Gramine's die_or_inf_loop function being called. However, this function is used only in SGX PAL (it makes sense to kill the SGX enclave immediately, but it seems to not make sense to kill the normal process in this way).

So I modified the LibOS test to not know/care about Gramine internals, and instead be self-contained.

This particular change can be split into a separate PR, if people want to.

[kailun-qin]

Reviewed 1 of 35 files at r4.
Reviewable status: 1 of 59 files reviewed, 6 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)

---

python/gramine-manifest line 33 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Now that gramine-direct also works with Allowed and Trusted Files, we need to call this expansion function.

There is one side-effect: now both .manifest and .manifest.sgx files have exactly the same contents (previously they only differed in not-expanded vs expanded Trusted Files). This eats memory on the hard disk, so we could symlink .manifest.sgx -> .manifest. But on the other hand, users will most typically disregard .manifest file anyway and use only .manifest.sgx, so making the latter a symlink sounds wrong.

It seems that we still allow gramine-sgx-sign to expand/hash files (if hashes not present), is this intended?

---

python/gramine-manifest line 35 at r6 (raw file):

@click.option('--chroot',
    type=click.Path(exists=True, dir_okay=True, file_okay=False),
    help='Measure a chroot directory, not the host filesystem')

What about https:/gramineproject/gramine/blob/master/Documentation/manpages/gramine-manifest.rst?

Code quote:

@click.option('--chroot',
    type=click.Path(exists=True, dir_okay=True, file_okay=False),
    help='Measure a chroot directory, not the host filesystem')

[dimakuv]

Reviewable status: 1 of 60 files reviewed, 6 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 @kailun-qin)

---

python/gramine-manifest line 33 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

It seems that we still allow gramine-sgx-sign to expand/hash files (if hashes not present), is this intended?

Yes, this is intended. I saw no particular reason to remove that functionality from gramine-sgx-sign. Is there any particular concern about this?

---

python/gramine-manifest line 35 at r6 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

What about https:/gramineproject/gramine/blob/master/Documentation/manpages/gramine-manifest.rst?

Done.

[dimakuv]

Reviewable status: 1 of 60 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin)

a discussion (no related file):
FYI: I squashed and rebased to the latest master, per request from @mkow and agreement from @kailun-qin

---

[mkow]

Reviewed 21 of 35 files at r4, 10 of 14 files at r5, 1 of 1 files at r7, 1 of 14 files at r8, all commit messages.
Reviewable status: 33 of 60 files reviewed, 10 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv and @kailun-qin)

---

CI-Examples/nginx/Makefile line 56 at r8 (raw file):

		$(INSTALL_DIR)/conf/nginx-gramine.conf \
		$(TEST_DATA) nginx_args \
		$(INSTALL_DIR)/conf/server.crt

Please align with spaces, not tabs (it's not a recipe).
Also, I think it would look better if aligned to :?

---

CI-Examples/ra-tls-secret-prov/Makefile line 80 at r8 (raw file):

#       should be relative to the manifest, not to current dir) - drop `cd` and `notdir`.
secret_prov_minimal/client.manifest: secret_prov_minimal/client.manifest.template \
	                                 secret_prov_minimal/client

this should be aligned with spaces

---

libos/test/ltp/ltp.cfg line 252 at r8 (raw file):

# very long test, does thousands of forks
[epoll01]
timeout = 600

That's suspicious, why is this needed?

---

libos/test/ltp/manifest.template line 43 at r8 (raw file):

  "file:install/testcases/bin/execlp01_child",  # for execlp01 test
  "file:install/testcases/bin/execv01_child",   # for execv01 test
  "file:install/testcases/bin/execvp01_child",  # for execvp01 test

Why are all these needed now, but weren't before?

---

python/gramine-manifest line 33 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Yes, this is intended. I saw no particular reason to remove that functionality from gramine-sgx-sign. Is there any particular concern about this?

So, maybe we could drop .manifest.sgx altogether? It would simplify life for the users.

[dimakuv]

Reviewable status: 33 of 60 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: Intel) (waiting on @dimakuv, @kailun-qin, and @mkow)

---

CI-Examples/blender/Makefile line 44 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

gramineproject/examples#105 -- done here. I'll keep this comment blocking.

That PR was merged, resolving.

---

CI-Examples/nginx/Makefile line 56 at r8 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Please align with spaces, not tabs (it's not a recipe).
Also, I think it would look better if aligned to :?

Are you sure we want to do this? That's literally how all Makefiles in CI-Examples are written... It will be strange if only this particular snippet would have the spaces, and everything else would have the tabs.

---

CI-Examples/ra-tls-secret-prov/Makefile line 80 at r8 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

this should be aligned with spaces

What do you mean? We always need the first tab in Makefiles, and then the rest can be either tabs or spaces. In this particular case, I use spaces.

---

libos/test/ltp/ltp.cfg line 252 at r8 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

That's suspicious, why is this needed?

Actually, good question. I assumed this is because now gramine-direct spends some time in reading lists of trusted/allowed files (recall that this epoll01 test does a bunch of forks, and each fork starts Gramine from scratch, including re-reading the list of trusted files). But I better profile and double-check this theory.

---

libos/test/ltp/manifest.template line 43 at r8 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why are all these needed now, but weren't before?

Because gramine-direct just allowed to open any files before. But now gramine-direct behaves as gramine-sgx: it disallows all files that are not in either allowed or trusted lists. So we need to be explicit now.

---

python/gramine-manifest line 33 at r1 (raw file):

So, maybe we could drop .manifest.sgx altogether?

What do you mean by drop? There are still many things that are completely irrelevant in .manifest, like sgx.enclave_size. Logically they should belong in .manifest.sgx, unless we agree on this (big) change of "all Gramine backends are served by a single file .manifest".

We can definitely do this, but it's out of scope of this PR I think. And it needs some discussions, especially with @woju.

But I think making .manifest.sgx a symlink to .manifest is an ok idea, and maybe can even be implemented in this PR.

[mwkmwkmwk]

Reviewed 1 of 14 files at r8.
Reviewable status: 59 of 60 files reviewed, 15 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, @kailun-qin, and @mkow)

[dimakuv]

Reviewable status: 58 of 60 files reviewed, 14 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 @kailun-qin, @mkow, and @mwkmwkmwk)

---

libos/src/fs/chroot/allowed.c line 159 at r9 (raw file):

Previously, mwkmwkmwk (Marcelina Kościelnicka) wrote…

that would work, yeah

Extracted here: #1983

I am reluctant to add this in this PR, as it will become harder to review (compare the changes).

---

libos/src/fs/chroot/fs.c line 51 at r10 (raw file):

Previously, mwkmwkmwk (Marcelina Kościelnicka) wrote…

redundant with assert below

Done. Looks like debugging leftover.

---

libos/src/fs/chroot/fs.c line 101 at r10 (raw file):

Previously, mwkmwkmwk (Marcelina Kościelnicka) wrote…

nit: could avoid get_allowed_file check for directories by rewriting to if (type == S_IFDIR || get_allowed_file(strip_prefix(uri))

Done

---

libos/src/fs/chroot/fs.c line 110 at r10 (raw file):

Previously, mwkmwkmwk (Marcelina Kościelnicka) wrote…

nit: can simplify to just &data->chunk_hashes and remove temp variable

I'm reluctant to do this, as a badly written load_trusted_file() could accidentally update the data object then and leave it in inconsistent state. I think it's a good practice to first put things in temporary variables, if there's a high chance that the function may return failure.

---

pal/src/host/linux-sgx/pal_main.c line 485 at r4 (raw file):

Previously, mwkmwkmwk (Marcelina Kościelnicka) wrote…

why not do it now? it has the potential to actually simplify the code with a bunch of negative diffstat

This is a large change that touches a lot of unrelated places. I still prefer to do it as a follow-up PR. I'm currently working on this (based on top of this PR). In the worst case, if all reviewers will agree, I can cherry-pick the commit from that new PR into this one. But to me, the PRs work on completely different things, and to have clear Git/GitHub history, I would prefer to split them.

[dimakuv]

Reviewable status: 58 of 60 files reviewed, 14 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 @kailun-qin, @mkow, and @mwkmwkmwk)

---

pal/src/host/linux-sgx/pal_main.c line 485 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This is a large change that touches a lot of unrelated places. I still prefer to do it as a follow-up PR. I'm currently working on this (based on top of this PR). In the worst case, if all reviewers will agree, I can cherry-pick the commit from that new PR into this one. But to me, the PRs work on completely different things, and to have clear Git/GitHub history, I would prefer to split them.

Done here: #1984

So turned out not to be that big, so theoretically I could stash the changes into this PR. Whatever @mwkmwkmwk @mkow @kailun-qin decide, I can oblige.

[mkow]

Reviewed 3 of 4 files at r9.
Reviewable status: 58 of 60 files reviewed, 11 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, @kailun-qin, and @mwkmwkmwk)

---

pal/src/host/linux-sgx/pal_main.c line 485 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done here: #1984

So turned out not to be that big, so theoretically I could stash the changes into this PR. Whatever @mwkmwkmwk @mkow @kailun-qin decide, I can oblige.

Hmm, would it be possible to merge #1984 to master first? I guess that's what mwk meant, that if we ship this fix then this PR will get simpler (but I'm not entirely sure if that was her suggestion).
Anyways, that would be preferred way from my side.

[mkow]

Reviewed all commit messages.
Reviewable status: 58 of 60 files reviewed, 11 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, @kailun-qin, and @mwkmwkmwk)

---

CI-Examples/nginx/Makefile line 56 at r8 (raw file):

That's literally how all Makefiles in CI-Examples are written

I didn't find any other place in our Makefiles where we wrap the dependencies list. Anyways, won't block on this.

---

CI-Examples/ra-tls-secret-prov/Makefile line 80 at r8 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What do you mean? We always need the first tab in Makefiles, and then the rest can be either tabs or spaces. In this particular case, I use spaces.

No, that's for recipes. This is a line wrap from the dependecies list, and in the way you did it here it will end up misaligned if someone has different tab size settings than you.

---

python/gramine-manifest line 33 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

But I think making .manifest.sgx a symlink to .manifest is an ok idea

That means these options end up in .manifest anyways? Am I missing something?

Yes, see here:

gramine/python/graminelibos/manifest.py

Line 300 in 4a73025

def __init__(self, manifest_str):

Basically, gramine-manifest assigns SGX-specific options too. And this tool is used to generate .manifest from .manifest.template.

That's just how we (well, Woju) implemented this. We have two tools currently: gramine-manifest and gramine-sgx-sign. The latter tool is SGX-specific but its name doesn't imply "also assign SGX-specific options". So I guess Woju decided to dump all options, SGX specific or not, into the gramine-manifest tool.

Then I don't see why we need manifest.sgx at all. We can just remove it and always use .manifest? (and as said above, that's a change for another PR if agreed)

[mkow]

Reviewed 2 of 23 files at r1, 2 of 23 files at r2, 1 of 14 files at r5, 8 of 14 files at r8, 1 of 4 files at r9, 1 of 1 files at r11.
Reviewable status: 59 of 60 files reviewed, 19 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, @kailun-qin, and @mwkmwkmwk)

---

libos/src/fs/chroot/allowed.c line 12 at r11 (raw file):

 *
 * Allowed files are useful for debugging, or when files are guaranteed to have no effect on
 * security of the execution (e.g. non-critical logs), or when the application itself protects these

Suggestion:

non-confidential

---

libos/src/fs/chroot/allowed.c line 33 at r11 (raw file):

DEFINE_LISTP(allowed_file);
static LISTP_TYPE(allowed_file) g_allowed_file_list = LISTP_INIT;

Suggestion:

g_allowed_files_list

---

libos/src/fs/chroot/allowed.c line 34 at r11 (raw file):

DEFINE_LISTP(allowed_file);
static LISTP_TYPE(allowed_file) g_allowed_file_list = LISTP_INIT;
static spinlock_t g_allowed_file_lock = INIT_SPINLOCK_UNLOCKED;

Suggestion:

g_allowed_files_lock

---

libos/src/fs/chroot/allowed.c line 36 at r11 (raw file):

static spinlock_t g_allowed_file_lock = INIT_SPINLOCK_UNLOCKED;

static bool path_is_equal_or_subpath(const struct allowed_file* af, const char* path,

This name is confusing, which of the arguments will be treated as the subpath?

Code quote:

path_is_equal_or_subpath

---

libos/src/fs/chroot/allowed.c line 36 at r11 (raw file):

static spinlock_t g_allowed_file_lock = INIT_SPINLOCK_UNLOCKED;

static bool path_is_equal_or_subpath(const struct allowed_file* af, const char* path,

Does this assume that the paths are already normalized? If so, please add a comment.

---

libos/src/fs/chroot/file_check_policy.c line 9 at r11 (raw file):

#include "toml_utils.h"

int g_file_check_policy = FILE_CHECK_POLICY_STRICT;

Please name the enum and use it via enum name instead of int.

---

libos/src/fs/chroot/fs.c line 45 at r11 (raw file):

struct chroot_inode_data {
    bool is_trusted;
    bool is_allowed;

Why two bools? One is always the negation of the other, right? If you want to be explicit with the names, then I'd use an enum instead.

// Update: I see, it's used with "allow everything" via the manifest switch. Could you add a comment here explaining that? Or maybe change this into a tri-state enum? (disallowed-allowed-trusted)

---

libos/src/fs/chroot/fs.c line 302 at r11 (raw file):

    mode_t perm = pal_attr.share_flags;
    size_t file_size = (type == S_IFREG ? pal_attr.pending_size : 0);

Suggestion:

size_t file_size = type == S_IFREG ? pal_attr.pending_size : 0;

---

libos/src/fs/chroot/fs.c line 538 at r11 (raw file):

    if (is_trusted_from_inode_data(hdl->inode)) {
        struct chroot_inode_data* data = hdl->inode->data;
        ret = copy_and_verify_trusted_file(hdl->pal_handle, offset, actual_count, buf,

It's the same, but IMO this makes more sense. Or maybe make it an in-out argument, like in PalStreamRead()? copy_and_verify_trusted_file() calculates this value on its own anyways.

Suggestion:

count

---

pal/src/host/linux-sgx/pal_streams.c line 338 at r8 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

To @mkow: this is the generic code that sets the handle->file.fd (which is aliased by a generic field handle->generic.fd).

Huh, this "aliasing" in struct PAL_HANDLE is weird. One union member doesn't start with PAL_IDX fd;?

Ok, I see there's a magic pair of flags in the handle header which decide whether this field is present... This is a really terrible design, will probably lead to a bug someday...

[dimakuv]

Reviewable status: 59 of 60 files reviewed, 18 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 @kailun-qin, @mkow, and @mwkmwkmwk)

---

pal/src/host/linux-sgx/pal_main.c line 485 at r4 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Hmm, would it be possible to merge #1984 to master first? I guess that's what mwk meant, that if we ship this fix then this PR will get simpler (but I'm not entirely sure if that was her suggestion).
Anyways, that would be preferred way from my side.

Done, I re-created #1984 on top of current master: #1986

I decided to create a separate PR so that we can decide which one to use better. I guess we should favor #1986.

---

pal/src/host/linux-sgx/pal_streams.c line 338 at r8 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Huh, this "aliasing" in struct PAL_HANDLE is weird. One union member doesn't start with PAL_IDX fd;?

Ok, I see there's a magic pair of flags in the handle header which decide whether this field is present... This is a really terrible design, will probably lead to a bug someday...

Yes, the design is pretty bad. Do you want me to create an issue on this? And what would be the preferred fix -- just remove generic.fd and always use specific <file|process|etc>.id (this will result in slightly bloated code because of duplication)?

---

python/gramine-manifest line 33 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Then I don't see why we need manifest.sgx at all. We can just remove it and always use .manifest? (and as said above, that's a change for another PR if agreed)

Well, yes, we won't need this file (other than for backwards-compatibility reasons). Do you want me to create a GitHub issue?

[dimakuv]

Reviewable status: 53 of 60 files reviewed, 18 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 @kailun-qin, @mkow, and @mwkmwkmwk)

---

CI-Examples/nginx/Makefile line 56 at r8 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

That's literally how all Makefiles in CI-Examples are written

I didn't find any other place in our Makefiles where we wrap the dependencies list. Anyways, won't block on this.

Done. Ok, my bad, I didn't realize that you're talking about wrapping the list of dependencies. Sorry.

---

CI-Examples/ra-tls-secret-prov/Makefile line 80 at r8 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

No, that's for recipes. This is a line wrap from the dependecies list, and in the way you did it here it will end up misaligned if someone has different tab size settings than you.

Done.

---

libos/src/fs/chroot/allowed.c line 12 at r11 (raw file):

 *
 * Allowed files are useful for debugging, or when files are guaranteed to have no effect on
 * security of the execution (e.g. non-critical logs), or when the application itself protects these

Done.

---

libos/src/fs/chroot/allowed.c line 33 at r11 (raw file):

DEFINE_LISTP(allowed_file);
static LISTP_TYPE(allowed_file) g_allowed_file_list = LISTP_INIT;

Done.

---

libos/src/fs/chroot/allowed.c line 34 at r11 (raw file):

DEFINE_LISTP(allowed_file);
static LISTP_TYPE(allowed_file) g_allowed_file_list = LISTP_INIT;
static spinlock_t g_allowed_file_lock = INIT_SPINLOCK_UNLOCKED;

Done.

---

libos/src/fs/chroot/allowed.c line 36 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This name is confusing, which of the arguments will be treated as the subpath?

Done.

---

libos/src/fs/chroot/allowed.c line 36 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Does this assume that the paths are already normalized? If so, please add a comment.

Done.

---

libos/src/fs/chroot/file_check_policy.c line 9 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Please name the enum and use it via enum name instead of int.

Done.

---

libos/src/fs/chroot/fs.c line 45 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why two bools? One is always the negation of the other, right? If you want to be explicit with the names, then I'd use an enum instead.

// Update: I see, it's used with "allow everything" via the manifest switch. Could you add a comment here explaining that? Or maybe change this into a tri-state enum? (disallowed-allowed-trusted)

Done.

---

libos/src/fs/chroot/fs.c line 302 at r11 (raw file):

    mode_t perm = pal_attr.share_flags;
    size_t file_size = (type == S_IFREG ? pal_attr.pending_size : 0);

Done.

---

libos/src/fs/chroot/fs.c line 538 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

It's the same, but IMO this makes more sense. Or maybe make it an in-out argument, like in PalStreamRead()? copy_and_verify_trusted_file() calculates this value on its own anyways.

Done.

[mkow]

Reviewed 5 of 6 files at r12, all commit messages.
Reviewable status: 58 of 60 files reviewed, 10 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, @kailun-qin, and @mwkmwkmwk)

---

libos/src/fs/chroot/allowed.c line 159 at r9 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Extracted here: #1983

I am reluctant to add this in this PR, as it will become harder to review (compare the changes).

I'm fine with doing that optimization separately.

---

libos/src/fs/chroot/fs.c line 64 at r12 (raw file):

/* this data is set up only once (at inode creation or restore), so doesn't require locking */
struct chroot_inode_data {
    enum file_protection_kind file_kind;

Suggestion:

prot_kind;

---

libos/src/fs/chroot/fs.c line 97 at r12 (raw file):

    data->file_kind = FILE_PROTECTION_KIND_NONE;
    if (get_allowed_file(strip_prefix(uri)))
        data->file_kind = FILE_PROTECTION_KIND_ALLOWED;

Suggestion:

    data->file_kind = get_allowed_file(strip_prefix(uri))
                      ? FILE_PROTECTION_KIND_ALLOWED
                      : FILE_PROTECTION_KIND_NONE;

---

pal/src/host/linux-sgx/pal_streams.c line 338 at r8 (raw file):

Do you want me to create an issue on this?

Sounds good.

Regarding how to fix it - I'm not sure, I'd need to think about that more, so let's just create an issue and work on it later.

---

python/gramine-manifest line 33 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Well, yes, we won't need this file (other than for backwards-compatibility reasons). Do you want me to create a GitHub issue?

Yes, please.

[dimakuv]

Reviewable status: 58 of 60 files reviewed, 10 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 @kailun-qin, @mkow, @mwkmwkmwk, and @woju)

---

libos/src/fs/chroot/fs.c line 64 at r12 (raw file):

/* this data is set up only once (at inode creation or restore), so doesn't require locking */
struct chroot_inode_data {
    enum file_protection_kind file_kind;

Done.

---

libos/src/fs/chroot/fs.c line 97 at r12 (raw file):

    data->file_kind = FILE_PROTECTION_KIND_NONE;
    if (get_allowed_file(strip_prefix(uri)))
        data->file_kind = FILE_PROTECTION_KIND_ALLOWED;

Done.

---

pal/src/host/linux-sgx/pal_streams.c line 338 at r8 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Do you want me to create an issue on this?

Sounds good.

Regarding how to fix it - I'm not sure, I'd need to think about that more, so let's just create an issue and work on it later.

Done: #1989

---

python/gramine-manifest line 33 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Yes, please.

Done, #1990

[dimakuv]

Reviewable status: 48 of 60 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin, @mkow, @mwkmwkmwk, and @woju)

---

pal/src/host/linux-sgx/pal_main.c line 411 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This part is ugly: we still want to warn users about potentially insecure manifest options in SGX PAL, but these manifest options are located in LibOS. So we have to do some small processing of LibOS-bound options here.

Done, this was removed (because of #1986)

---

pal/src/host/linux-sgx/pal_main.c line 485 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done, I re-created #1984 on top of current master: #1986

I decided to create a separate PR so that we can decide which one to use better. I guess we should favor #1986.

Done, rebased on top of #1986

[mkow]

Reviewed 1 of 2 files at r10, 1 of 6 files at r12, 1 of 1 files at r13, 9 of 9 files at r14, all commit messages.
Reviewable status: all files reviewed, 15 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, @kailun-qin, and @mwkmwkmwk)

---

libos/src/fs/chroot/trusted.c line 18 at r14 (raw file):

 * each chunk (of size TRUSTED_CHUNK_SIZE) in the file. The per-chunk hashes are used for partial
 * verification in future reads, to avoid re-verifying the whole file again or the need of caching
 * file contents.

that's an important detail, caching parts is totally fine (and AFAIR we actually considered doing so)

Suggestion:

the whole file contents.

---

libos/src/fs/chroot/trusted.c line 44 at r14 (raw file):

};

/* initialized once at startup, so doesn't require locking */

Suggestion:

/* initialized once at startup and read-only afterwards, so doesn't require locking */

---

libos/src/fs/chroot/trusted.c line 48 at r14 (raw file):

static LISTP_TYPE(trusted_file) g_trusted_file_list = LISTP_INIT;

static int read_trusted_file(PAL_HANDLE handle, void* buffer, uint64_t offset, size_t size) {

Can't we use read_exact() instead? (but you'd need to add offset arg to it)
Also, read_trusted_file is misleading, it's not reading a trusted file with some checks, it's just a generic file read without any verification.

---

libos/src/fs/chroot/trusted.c line 86 at r14 (raw file):

    struct trusted_file* tmp;
    LISTP_FOR_EACH_ENTRY(tmp, &g_trusted_file_list, list) {
        if ((tmp->path_len == norm_path_size - 1)

Suggestion:

tmp->path_len == norm_path_size - 1

---

libos/src/fs/chroot/trusted.c line 97 at r14 (raw file):

size_t get_chunk_hashes_size(size_t file_size) {
    return (sizeof(struct trusted_chunk_hash) * UDIV_ROUND_UP(file_size, TRUSTED_CHUNK_SIZE));

Suggestion:

return sizeof(struct trusted_chunk_hash) * UDIV_ROUND_UP(file_size, TRUSTED_CHUNK_SIZE);

---

libos/src/fs/chroot/trusted.c line 115 at r14 (raw file):

    memcpy(uri, URI_PREFIX_FILE, URI_PREFIX_FILE_LEN);
    memcpy(uri + URI_PREFIX_FILE_LEN, tf->path, tf->path_len);
    uri[URI_PREFIX_FILE_LEN + tf->path_len] = '\0';

You could use alloc_concat() instead.

---

libos/src/fs/chroot/trusted.c line 212 at r14 (raw file):

}

int copy_and_verify_trusted_file(PAL_HANDLE handle, uint64_t offset, size_t count, uint8_t* buf,

Suggestion:

read_and_verify_trusted_file

---

libos/src/fs/chroot/trusted.c line 221 at r14 (raw file):

    uint64_t end = MIN(offset + count, file_size);
    uint64_t aligned_offset = ALIGN_DOWN(offset, TRUSTED_CHUNK_SIZE);
    uint64_t aligned_end    = ALIGN_UP(end, TRUSTED_CHUNK_SIZE);

Why do we need that? You use it in only one place where end can be used instead.

---

libos/src/fs/chroot/trusted.c line 256 at r14 (raw file):

            buf_pos += chunk_size;
        } else {
            /* if current chunk-to-copy only partially overlaps with the requested region-to-copy,

Suggestion:

chunk-to-verify

---

libos/src/fs/chroot/trusted.c line 352 at r14 (raw file):

    ret = toml_rtos(toml_trusted_sha256_raw, &toml_trusted_sha256_str);
    if (ret < 0 || !toml_trusted_sha256_str) {

Why is this if different than the above one? (when parsing toml_trusted_uri_str)

---

libos/src/fs/chroot/trusted.c line 359 at r14 (raw file):

    if (!strstartswith(toml_trusted_uri_str, URI_PREFIX_FILE)) {
        log_error("Invalid URI [%s]: Trusted files must start with 'file:'", toml_trusted_uri_str);

Suggestion:

"Invalid URI [%s]: Trusted files must start with '" URI_PREFIX_FILE "'"

[dimakuv]

Reviewable status: 57 of 60 files reviewed, 15 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 @kailun-qin, @mkow, and @mwkmwkmwk)

---

libos/src/fs/chroot/trusted.c line 18 at r14 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

that's an important detail, caching parts is totally fine (and AFAIR we actually considered doing so)

Done.

---

libos/src/fs/chroot/trusted.c line 44 at r14 (raw file):

};

/* initialized once at startup, so doesn't require locking */

Done.

---

libos/src/fs/chroot/trusted.c line 48 at r14 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Can't we use read_exact() instead? (but you'd need to add offset arg to it)
Also, read_trusted_file is misleading, it's not reading a trusted file with some checks, it's just a generic file read without any verification.

Done, partially.

I cannot use read_exact() because that function is used for pipes/unix sockets, and explicitly has offset = 0 argument always. The PAL pipes/sockets verify that PalStreamRead() must have offset == 0, so I would need a rather complex logic like this:

read_exact(..., bool is_file, uint64_t offset) {
    while (stuff to read) {
        uint64_t curr_offset = is_file ? read + offset : 0; /* pipes require offset 0 */
        PalStreamRead(..., curr_offset);
    }
}

This just didn't feel right, so I kept the function here as a local one, and renamed it.

---

libos/src/fs/chroot/trusted.c line 86 at r14 (raw file):

    struct trusted_file* tmp;
    LISTP_FOR_EACH_ENTRY(tmp, &g_trusted_file_list, list) {
        if ((tmp->path_len == norm_path_size - 1)

Done.

---

libos/src/fs/chroot/trusted.c line 97 at r14 (raw file):

size_t get_chunk_hashes_size(size_t file_size) {
    return (sizeof(struct trusted_chunk_hash) * UDIV_ROUND_UP(file_size, TRUSTED_CHUNK_SIZE));

Done.

---

libos/src/fs/chroot/trusted.c line 115 at r14 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

You could use alloc_concat() instead.

Done.

---

libos/src/fs/chroot/trusted.c line 212 at r14 (raw file):

}

int copy_and_verify_trusted_file(PAL_HANDLE handle, uint64_t offset, size_t count, uint8_t* buf,

Done.

---

libos/src/fs/chroot/trusted.c line 221 at r14 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why do we need that? You use it in only one place where end can be used instead.

Done.

---

libos/src/fs/chroot/trusted.c line 256 at r14 (raw file):

            buf_pos += chunk_size;
        } else {
            /* if current chunk-to-copy only partially overlaps with the requested region-to-copy,

Done.

---

libos/src/fs/chroot/trusted.c line 352 at r14 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why is this if different than the above one? (when parsing toml_trusted_uri_str)

Done.

---

libos/src/fs/chroot/trusted.c line 359 at r14 (raw file):

    if (!strstartswith(toml_trusted_uri_str, URI_PREFIX_FILE)) {
        log_error("Invalid URI [%s]: Trusted files must start with 'file:'", toml_trusted_uri_str);

Done.

[mkow]

Reviewed 3 of 3 files at r15, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mwkmwkmwk)

---

libos/src/fs/chroot/trusted.c line 48 at r14 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done, partially.

I cannot use read_exact() because that function is used for pipes/unix sockets, and explicitly has offset = 0 argument always. The PAL pipes/sockets verify that PalStreamRead() must have offset == 0, so I would need a rather complex logic like this:

read_exact(..., bool is_file, uint64_t offset) {
    while (stuff to read) {
        uint64_t curr_offset = is_file ? read + offset : 0; /* pipes require offset 0 */
        PalStreamRead(..., curr_offset);
    }
}

This just didn't feel right, so I kept the function here as a local one, and renamed it.

Ah, ok then.

[kailun-qin]

Reviewed 1 of 4 files at r9, 4 of 6 files at r12, 9 of 9 files at r14, 3 of 3 files at r15, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @mwkmwkmwk)

[dimakuv]

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

---

libos/src/fs/chroot/allowed.c line 159 at r9 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I'm fine with doing that optimization separately.

@mwkmwkmwk You're still blocking on this discussion.

[mkow]

Reviewed 5 of 5 files at r16, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mwkmwkmwk)

[kailun-qin]

Reviewed 4 of 5 files at r16, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[kailun-qin]

Reviewed 1 of 4 files at r9, 1 of 5 files at r16.
Reviewable status: complete! all files reviewed, all discussions resolved