[PAL] Validate entrypoint ELF file separately #1820
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
-- commits line 16 at r1:
Please note that this assumption is already enforced by our PAL code:
Lines 261 to 269 in e3d3aa3
There was a problem hiding this comment.
Reviewable status: 0 of 6 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) (waiting on @dimakuv)
pal/src/host/linux-sgx/pal_misc.c line 22 at r1 (raw file):
#include "pal_linux_error.h" #include "seqlock.h" #include "sgx_arch.h"
TODO: Get rid of sgx_arch.h and sgx_file_hash_t
pal/src/host/linux-sgx/pal_misc.c line 834 at r1 (raw file):
} if (!entrypoint_sha256_str || strlen(entrypoint_sha256_str) != sizeof(sgx_file_hash_t) * 2) {
TODO: The first clause was already checked above, remove
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, all discussions resolved, 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
pal/src/host/linux-sgx/pal_misc.c line 22 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
TODO: Get rid of
sgx_arch.handsgx_file_hash_t
Done
pal/src/host/linux-sgx/pal_misc.c line 834 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
TODO: The first clause was already checked above, remove
Done
There was a problem hiding this comment.
Reviewed 5 of 6 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all 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)
-- commits line 2 at r2:
that's not optional, which "allow" implies
Suggestion:
[PAL] Validate entrypoint ELF file separately
pal/src/pal_rtld.c line 659 at r2 (raw file):
ret = _PalStreamAttributesQueryByHandle(handle, &attr); if (ret < 0) { log_error("Getting size of ELF file failed");
Suggestion:
log_error("Getting size of loader entrypoint binary failed");pal/src/pal_rtld.c line 665 at r2 (raw file):
buf = malloc(attr.pending_size); if (!buf) { log_error("Allocating buffer to hold ELF file of size %lu failed", attr.pending_size);
ditto - missing context which ELF file
actually, same for a few other places below
pal/src/pal_rtld.c line 672 at r2 (raw file):
size_t remaining = attr.pending_size; while (remaining > 0) { int64_t read = _PalStreamRead(handle, buf_offset, remaining, buf + buf_offset);
Everything here is wrong ;) ret vs read
pal/src/host/linux-sgx/pal_misc.c line 832 at r2 (raw file):
log_error("Cannot find 'loader.entrypoint_sha256' in manifest"); ret = -PAL_ERROR_INVAL; goto out;
Suggestion:
return -PAL_ERROR_INVAL;python/graminelibos/manifest.py line 304 at r2 (raw file):
if not 'entrypoint_sha256' in loader: entrypoint_tf = TrustedFile.from_realpath(uri2path(loader['entrypoint'])).ensure_hash() loader.setdefault('entrypoint_sha256', entrypoint_tf.sha256);
Suggestion:
loader['entrypoint_sha256'] = entrypoint_tf.sha256
There was a problem hiding this comment.
Reviewable status: all files reviewed, 7 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)
pal/src/host/linux/pal_misc.c line 84 at r2 (raw file):
int _PalValidateEntrypoint(const void* buf, size_t size) { __UNUSED(buf);
it isn't obvious that this should be a noop — perhaps it could be a good idea to also validate entrypoint_sha256 here if present, so that we can test the hash logic more easily, but without raising an error if the option isn't present?
There was a problem hiding this comment.
Reviewable status: 3 of 6 files reviewed, 7 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, @mkow, and @mwkmwkmwk)
Previously, mkow (Michał Kowalczyk) wrote…
that's not optional, which "allow" implies
Will do during final rebase
pal/src/pal_rtld.c line 659 at r2 (raw file):
ret = _PalStreamAttributesQueryByHandle(handle, &attr); if (ret < 0) { log_error("Getting size of ELF file failed");
Done.
pal/src/pal_rtld.c line 665 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
ditto - missing context which ELF file
actually, same for a few other places below
Done.
pal/src/pal_rtld.c line 672 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Everything here is wrong ;)
retvsread
Done.
pal/src/host/linux/pal_misc.c line 84 at r2 (raw file):
Previously, mwkmwkmwk (Marcelina Kościelnicka) wrote…
it isn't obvious that this should be a noop — perhaps it could be a good idea to also validate
entrypoint_sha256here if present, so that we can test the hash logic more easily, but without raising an error if the option isn't present?
I tried it but realized we can't do it -- the Linux PAL does not use the mbedTLS library, thus I don't have SHA256 implementation to use...
I'm currently keeping as-is. Adding mbedTLS dep for Linux PAL seems like an unrelated task.
pal/src/host/linux-sgx/pal_misc.c line 832 at r2 (raw file):
log_error("Cannot find 'loader.entrypoint_sha256' in manifest"); ret = -PAL_ERROR_INVAL; goto out;
Done.
python/graminelibos/manifest.py line 304 at r2 (raw file):
if not 'entrypoint_sha256' in loader: entrypoint_tf = TrustedFile.from_realpath(uri2path(loader['entrypoint'])).ensure_hash() loader.setdefault('entrypoint_sha256', entrypoint_tf.sha256);
Done.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mwkmwkmwk)
There was a problem hiding this comment.
Reviewed 2 of 6 files at r1, 1 of 3 files at r3, all commit messages.
Reviewable status: all 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
pal/src/pal_rtld.c line 570 at r3 (raw file):
continue; ret = _PalVirtualMemoryAlloc((void*)c->map_end, c->alloc_end - c->map_end, c->prot);
this is no longer necessary
pal/src/host/linux/pal_misc.c line 84 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I tried it but realized we can't do it -- the Linux PAL does not use the mbedTLS library, thus I don't have SHA256 implementation to use...
I'm currently keeping as-is. Adding mbedTLS dep for Linux PAL seems like an unrelated task.
ah, oops.
let's leave it like that, then
There was a problem hiding this comment.
Reviewable status: all 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @mwkmwkmwk)
pal/src/pal_rtld.c line 570 at r3 (raw file):
Previously, mwkmwkmwk (Marcelina Kościelnicka) wrote…
this is no longer necessary
Why are you saying this? That's for the BSS part.
Above we mmapped only [c->start, c->map_end) (for data and code). Now we also mmap [c->map_end, c->alloc_end) which is used for BSS.
We could refactor this code to only have one _PalVirtualMemoryAlloc(), but then we'll lose this clarify of data+code vs BSS. Additionally, we'll need to play with page protections more...
There was a problem hiding this comment.
Reviewable status: all 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
pal/src/pal_rtld.c line 570 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why are you saying this? That's for the BSS part.
Above we mmapped only
[c->start, c->map_end)(for data and code). Now we also mmap[c->map_end, c->alloc_end)which is used for BSS.We could refactor this code to only have one
_PalVirtualMemoryAlloc(), but then we'll lose this clarify of data+code vs BSS. Additionally, we'll need to play with page protections more...
yes, that's what I mean — we can have just one allocation above, by mapping up to alloc_end in one go, and relying on the 0-initialization. this should simplify the code a bit.
I don't understand the bit about page protections — the whole resulting area should have uniform protection, no?
There was a problem hiding this comment.
Reviewable status: 5 of 6 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow and @mwkmwkmwk)
pal/src/pal_rtld.c line 570 at r3 (raw file):
Previously, mwkmwkmwk (Marcelina Kościelnicka) wrote…
yes, that's what I mean — we can have just one allocation above, by mapping up to
alloc_endin one go, and relying on the 0-initialization. this should simplify the code a bit.I don't understand the bit about page protections — the whole resulting area should have uniform protection, no?
Done.
You're right. The code got simpler. Plus I also removed a code snippet (see below) that redundantly zeroed out the memory; we don't need this explicit zero-out since we rely on the implicit zero-initialization in PalVirtualMemoryAlloc().
pal/src/pal_rtld.c line 550 at r4 (raw file):
assert(IS_ALLOC_ALIGNED_PTR(map_addr)); assert(IS_ALLOC_ALIGNED(map_size));
FYI: These two asserts are copied from #1818, they were requested by @mkow . See https://reviewable.io/reviews/gramineproject/gramine/1818#-NzzllZo0bvL5jvIhJix
There was a problem hiding this comment.
Reviewed 1 of 6 files at r1.
Reviewable status: 5 of 6 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
pal/src/pal_rtld.c line 570 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done.
You're right. The code got simpler. Plus I also removed a code snippet (see below) that redundantly zeroed out the memory; we don't need this explicit zero-out since we rely on the implicit zero-initialization in
PalVirtualMemoryAlloc().
nice! there's two more simplifications that can be done now:
map_endis no longer needed and can be removed- it would be easier to just store
p_fileszinstead ofdata_end(cpy_sizeis effectively recomputedp_filesz)
There was a problem hiding this comment.
Reviewable status: 5 of 6 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow and @mwkmwkmwk)
pal/src/pal_rtld.c line 570 at r3 (raw file):
Previously, mwkmwkmwk (Marcelina Kościelnicka) wrote…
nice! there's two more simplifications that can be done now:
map_endis no longer needed and can be removed- it would be easier to just store
p_fileszinstead ofdata_end(cpy_sizeis effectively recomputedp_filesz)
Done.
There was a problem hiding this comment.
Reviewed 3 of 6 files at r1, all commit messages.
Reviewable status: 5 of 6 files reviewed, 3 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, @mkow, and @mwkmwkmwk)
a discussion (no related file):
How about using loader.entrypoint.uri and loader.entrypoint.sha256?
There was a problem hiding this comment.
Reviewable status: 5 of 6 files reviewed, 3 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 @llly, @mkow, and @mwkmwkmwk)
a discussion (no related file):
Previously, llly (Li Xun) wrote…
How about using
loader.entrypoint.uriandloader.entrypoint.sha256?
I'm a bit reluctant to deprecate loader.entrypoint in favor of loader.entrypoint.uri. (See https://gramine.readthedocs.io/en/stable/manifest-syntax.html#loader-entrypoint)
This is indeed a cleaner approach. If other reviewers agree, I can do it.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 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 @llly)
pal/src/pal_rtld.c line 88 at r6 (raw file):
elf_addr_t alloc_end; /* Segment's file-image size, i.e. the exact size of the segment in ELF file */
this is not quite true given the + (p_vaddr - align_down(p_vaddr)) part
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 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 @llly)
a discussion (no related file):
How about using
loader.entrypoint.uriandloader.entrypoint.sha256?
this sounds good
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 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 @llly)
a discussion (no related file):
Previously, mwkmwkmwk (Marcelina Kościelnicka) wrote…
How about using
loader.entrypoint.uriandloader.entrypoint.sha256?this sounds good
SGTM
dimakuv
force-pushed
the
dimakuv/pal-validate-entrypoint-separately
branch
from
cf41fcc
to
97e831c
Compare
There was a problem hiding this comment.
Reviewable status: 3 of 58 files reviewed, 3 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 @llly, @mkow, and @mwkmwkmwk)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
SGTM
Done. Had to rebase to latest master, since this PR conflicted with #1716
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Will do during final rebase
Done.
pal/src/pal_rtld.c line 88 at r6 (raw file):
Previously, mwkmwkmwk (Marcelina Kościelnicka) wrote…
this is not quite true given the
+ (p_vaddr - align_down(p_vaddr))part
Done, reformulated. I don't know how to say it better without going into those page-alignment explanations. There is a trick for the OS kernel to effectively share pages belonging to a file from which the segments are loaded, and I don't want to describe this. See:
mkow
changed the title
There was a problem hiding this comment.
Reviewed 55 of 55 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 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, @llly, and @mwkmwkmwk)
pal/src/pal_main.c line 571 at r7 (raw file):
if (ret < 0) { /* didn't find `loader.entrypoint.uri`, try deprecated `loader.entrypoint`; * TODO: remove this in Gramine v1.9 */
missing deprecation warning?
There was a problem hiding this comment.
Reviewed 55 of 55 files at r7, all commit messages.
Reviewable status: all 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: Intel) (waiting on @dimakuv and @mwkmwkmwk)
There was a problem hiding this comment.
Reviewable status: 57 of 58 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @llly, @mkow, and @mwkmwkmwk)
pal/src/pal_main.c line 571 at r7 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
missing deprecation warning?
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @mwkmwkmwk)
There was a problem hiding this comment.
Reviewed 53 of 55 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
python/graminelibos/manifest.py line 342 at r8 (raw file):
# `loader.entrypoint.sha256`; replace with the default LibOS loader = manifest.setdefault('loader', {}) loader_entrypoint = loader.setdefault('entrypoint', {})
shouldn't we have legacy syntax handling here?
There was a problem hiding this comment.
Reviewable status: 55 of 59 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @llly, @mkow, and @mwkmwkmwk)
python/graminelibos/manifest.py line 342 at r8 (raw file):
Previously, mwkmwkmwk (Marcelina Kościelnicka) wrote…
shouldn't we have legacy syntax handling here?
Done.
Good catch. If I have an old syntax in the manifest template, I get an error:
~/gramineproject/gramine/CI-Examples/helloworld$ git diff --unified=0
diff --git a/CI-Examples/helloworld/helloworld.manifest.template b/CI-Examples/helloworld/helloworld.manifest.template
@@ -5,0 +6 @@
+loader.entrypoint = "blabla"
~/gramineproject/gramine/CI-Examples/helloworld$ make
gramine-manifest \
-Dlog_level=error \
helloworld.manifest.template helloworld.manifest
Traceback (most recent call last):
...
File "/home/dimakuv/gramineproject/gramine/built-debug/lib/python3.8/site-packages/graminelibos/manifest.py", line 344, in _
_init__
loader_entrypoint['uri'] = f'file:{_env.globals["gramine"]["libos"]}'
TypeError: 'str' object does not support item assignmentFixed now.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r9, 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
python/graminelibos/manifest_check.py line 62 at r9 (raw file):
Required('loader'): { # TODO: validator for sha256 Required('entrypoint'): {'uri': _uri, 'sha256': str},
should the 'uri' also be Required?
There was a problem hiding this comment.
Reviewable status: 58 of 59 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @mwkmwkmwk)
python/graminelibos/manifest_check.py line 62 at r9 (raw file):
Previously, mwkmwkmwk (Marcelina Kościelnicka) wrote…
should the
'uri'also beRequired?
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, 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
There was a problem hiding this comment.
Reviewed 3 of 4 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
There was a problem hiding this comment.
Reviewed 3 of 6 files at r1, 52 of 55 files at r7, 3 of 4 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
Previously, the entrypoint ELF file (`loader.entrypoint` in manifest) contents were validated during file read and file mmap, just like with all other files. In particular, Linux-SGX PAL expects the entrypoint to be marked as `sgx.trusted_files` and validates its SHA256 hash during file read/mmap. However, future commits will move handling of `sgx.trusted_files` to the LibOS layer. Thus, file read/mmap at the PAL layer will lose validation guarantees. This commit introduces a separate `loader.entrypoint.sha256` manifest option and a corresponding logic to validate the (only required) entrypoint file. A new assumption is made now, that the only file to be loaded by PAL (and not by LibOS) is this entrypoint file; this assumption can be made because the entrypoint has no dependencies. As a side effect, `loader.entrypoint` (which was previously a URI of the entrypoint file) is renamed to `loader.entrypoint.uri`. Additionally, all Gramine tests either remove `loader.entrypoint` (and corresponding `sgx.trusted_files`) or rename it to explicit `loader.entrypoint.uri`. Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
dimakuv
force-pushed
the
dimakuv/pal-validate-entrypoint-separately
branch
from
1b5396c
to
72668bb
Compare
There was a problem hiding this comment.
Reviewed 3 of 3 files at r11, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Reviewed 3 of 3 files at r11, all commit messages.
Reviewable status:
Description of the changes
Previously, the entrypoint ELF file (
loader.entrypointin manifest) contents were validated during file read and file mmap, just like with all other files. In particular, Linux-SGX PAL expects the entrypoint to be marked assgx.trusted_filesand validates its SHA256 hash during file read/mmap.However, future commits will move handling of
sgx.trusted_filesto the LibOS layer. Thus, file read/mmap at the PAL layer will lose validation guarantees. This commit introduces a separateloader.entrypoint_sha256manifest option and a corresponding logic to validate the (only required) entrypoint file. A new assumption is made now, that the only file to be loaded by PAL (and not by LibOS) is this entrypoint file; this assumption can be made because the entrypoint has no dependencies.Extracted from #1812.
Related to #1716 (ideally both should be merged one after another).
How to test this PR?
CI is enough.
This change is