-
Notifications
You must be signed in to change notification settings - Fork 0
Allow for tcb migration of sealed files #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: njeans <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request implements automatic TCB (Trusted Computing Base) migration for sealed/encrypted files when the CPU SVN (Security Version Number) changes after microcode updates. The feature adds new PAL APIs to access CPU SVN values and retrieve sealing keys for specific SVN versions, enabling seamless migration of encrypted files between different platform security states.
Key changes:
- Adds three new PAL APIs:
PalGetCPUSVN,PalSetCPUSVN, andPalGetSpecialKeyForSVNto support SVN-based key derivation - Implements automatic file migration that detects CPU SVN changes and re-encrypts files with the new sealing key
- Introduces
allow_tcb_migrationmanifest parameter to enable this feature per mount point
Reviewed changes
Copilot reviewed 28 out of 31 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| pal/include/pal/pal.h | Adds function declarations for the three new PAL APIs with documentation |
| pal/include/pal_internal.h | Adds internal function declarations for PAL implementations |
| pal/src/pal_symbols | Exports the three new PAL API symbols |
| pal/src/pal_misc.c | Implements PAL wrapper functions that delegate to host-specific implementations |
| pal/src/host/skeleton/pal_misc.c | Provides stub implementations returning PAL_ERROR_NOTIMPLEMENTED |
| pal/src/host/linux/pal_misc.c | Provides stub implementations for non-SGX Linux |
| pal/src/host/linux-sgx/pal_misc.c | Implements SGX-specific CPU SVN get/set operations and SVN-based key retrieval |
| pal/src/host/linux-sgx/pal_linux.h | Declares SGX helper function for seal key derivation with custom SVN |
| pal/src/host/linux-sgx/enclave_framework.c | Implements seal key derivation using custom CPU SVN via EGETKEY |
| libos/include/libos_fs.h | Adds allow_tcb_migration field to mount parameters struct |
| libos/include/libos_fs_encrypted.h | Adds TCB migration constants, types, and function declarations |
| libos/src/fs/libos_fs.c | Parses allow_tcb_migration parameter from manifest for root and non-root mounts |
| libos/src/fs/chroot/encrypted.c | Triggers TCB migration check during encrypted filesystem mount |
| libos/src/fs/libos_fs_encrypted.c | Implements core migration logic including file copying, directory traversal, and TCB info file management |
| libos/src/fs/dev/attestation.c | Adds /dev/attestation/cpu_svn pseudo file and /dev/attestation/keys/svn/_sgx_mrenclave/<cpu_svn> interface |
| libos/test/regression/sealed_file_tcb_migration.c | Adds comprehensive test that simulates CPU SVN changes and verifies migration |
| libos/test/regression/keys.c | Tests the new CPU SVN-based key retrieval interface |
| libos/test/regression/manifest.template | Configures test mount points with TCB migration enabled |
| libos/test/regression/meson.build | Builds the new TCB migration test binary |
| libos/test/regression/tests.toml | Registers the test in test suite |
| libos/test/regression/tests_musl.toml | Registers the test in musl test suite |
| libos/test/regression/test_libos.py | Implements Python test harness that runs migration test across two executions |
| libos/test/regression/.gitignore | Adds ignore patterns for TCB test directories |
| libos/test/regression/tmp_tcb/* | Adds .gitkeep files for test directory structure |
| python/graminelibos/manifest_check.py | Adds schema validation for allow_tcb_migration mount parameter |
| Documentation/pal/host-abi.rst | Documents the three new PAL APIs in host ABI reference |
| Documentation/manifest-syntax.rst | Documents the allow_tcb_migration mount parameter with security warnings |
| Documentation/devel/features.md | Lists the new CPU SVN pseudo file interfaces |
| Documentation/devel/encfiles.rst | Explains TCB migration feature, its use case, and security implications |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!buf) | ||
| return -ENOMEM; |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Memory leak: if malloc on line 406 fails, the function returns without freeing the previously allocated key structure and its key->name field. The error path cleanup should be reached via goto out instead of an early return.
| if (!buf) | |
| return -ENOMEM; | |
| if (!buf) { | |
| ret = -ENOMEM; | |
| goto out; | |
| } |
| log_error("Failed to generate sealing key using SGX EGETKEY"); | ||
| return PAL_ERROR_DENIED; | ||
| } | ||
| return 0; |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace at the end of the line. This should be removed to conform to coding standards.
| return 0; | |
| return 0; |
| assert(dest->pf); | ||
| assert(src->pf); | ||
| int ret; | ||
| char * buf = NULL; |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after asterisk in pointer declaration. The style should be char* buf = NULL; instead of char * buf = NULL; to be consistent with the codebase style.
| char * buf = NULL; | |
| char* buf = NULL; |
| } | ||
| if (memcmp(¤t_cpu_svn, &saved_cpu_svn, sizeof(saved_cpu_svn)) != 0) { | ||
| log_warning("CPU SVN has changed - doing TCB migration for %s", uri); | ||
| ret = do_migrate(uri, &saved_cpu_svn, key_name); |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After successful migration, the gramine.tcb_info file needs to be updated with the new CPU SVN. Currently, after do_migrate succeeds, the file still contains the old CPU SVN. The new CPU SVN should be written to the file after successful migration.
| ret = do_migrate(uri, &saved_cpu_svn, key_name); | |
| ret = do_migrate(uri, &saved_cpu_svn, key_name); | |
| if (ret < 0) { | |
| log_warning("TCB migration failed for %s", uri); | |
| goto out; | |
| } | |
| /* Migration succeeded: update tcb_info file with the new CPU SVN */ | |
| PalObjectDestroy(tcb_info_file_pal_handle); | |
| tcb_info_file_pal_handle = NULL; | |
| char* tcb_info_uri_update = NULL; | |
| size_t uri_len_update = strlen(uri); | |
| if (uri[uri_len_update - 1] == '/') { | |
| tcb_info_uri_update = alloc_concat(uri, -1, TCB_INFO_FILE_NAME, -1); | |
| } else { | |
| tcb_info_uri_update = | |
| alloc_concat3(uri, -1, "/\0", -1, TCB_INFO_FILE_NAME, -1); | |
| } | |
| if (!tcb_info_uri_update) { | |
| log_warning("failed to allocate URI for tcb_info update"); | |
| ret = -ENOMEM; | |
| goto out; | |
| } | |
| log_debug("Updating TCB info file URI: %s", tcb_info_uri_update); | |
| ret = PalStreamOpen(tcb_info_uri_update, PAL_ACCESS_RDWR, TCB_INFO_PERM_RW, | |
| PAL_CREATE_ALWAYS, /*options=*/0, &tcb_info_file_pal_handle); | |
| free(tcb_info_uri_update); | |
| if (ret < 0) { | |
| log_warning("tcb_info PalStreamOpen (update) failed: %s", pal_strerror(ret)); | |
| ret = pal_to_unix_errno(ret); | |
| goto out; | |
| } | |
| ret = write_exact(tcb_info_file_pal_handle, current_cpu_svn, CPU_SVN_SIZE); | |
| if (ret < 0) { | |
| log_warning("writing updated CPU SVN to tcb_info file failed"); | |
| goto out; | |
| } |
|
|
||
| The ``allow_tcb_migration`` mount parameter (default: ``false``) determines whether | ||
| the TCB migration feature is enabled for the mount. This feature allows sealed files to | ||
| be migrated to latest CPU SVN version after applying microcode updates. This feature |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting issue: There are two spaces between sentences instead of one. Should be "microcode updates. This feature" with only one space after the period.
| be migrated to latest CPU SVN version after applying microcode updates. This feature | |
| be migrated to latest CPU SVN version after applying microcode updates. This feature |
| return false; | ||
| } | ||
| cpu_svn_t cpu_svn; | ||
|
|
||
| if (!hex2bytes((char*)dent->name, strlen(dent->name), &cpu_svn, sizeof(cpu_svn_t))) { | ||
| log_warning("key_name_exists_svn: invalid key name format"); | ||
| return false; |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function returns false (boolean) but the return type is int. The function should return an appropriate integer error code such as -EINVAL instead of false.
| return false; | |
| } | |
| cpu_svn_t cpu_svn; | |
| if (!hex2bytes((char*)dent->name, strlen(dent->name), &cpu_svn, sizeof(cpu_svn_t))) { | |
| log_warning("key_name_exists_svn: invalid key name format"); | |
| return false; | |
| return -EINVAL; | |
| } | |
| cpu_svn_t cpu_svn; | |
| if (!hex2bytes((char*)dent->name, strlen(dent->name), &cpu_svn, sizeof(cpu_svn_t))) { | |
| log_warning("key_name_exists_svn: invalid key name format"); | |
| return -EINVAL; |
|
|
||
| int ret; | ||
|
|
||
| char * key_name = dent->parent->name; |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after asterisk in pointer declaration. Should be char* key_name = dent->parent->name; instead of char * key_name = dent->parent->name; to be consistent with the codebase style.
| char * key_name = dent->parent->name; | |
| char* key_name = dent->parent->name; |
| key = calloc(1, sizeof(*key)); | ||
| if (!key) { | ||
| log_error("Cannot allocate memory for key"); | ||
| ret = -ENOMEM; | ||
| goto out; | ||
| } |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Memory allocation redundancy: The key is allocated on line 391, but then the pointer is overwritten by create_encrypted_files_key_for_svn on line 397. This first allocation is unnecessary and causes a memory leak. Remove the initial calloc since create_encrypted_files_key_for_svn creates and returns a new key.
| key = calloc(1, sizeof(*key)); | |
| if (!key) { | |
| log_error("Cannot allocate memory for key"); | |
| ret = -ENOMEM; | |
| goto out; | |
| } |
| be migrated to latest CPU SVN version after applying microcode updates. This feature | ||
| is only valid for ``key_name`` of ``"_sgx_mrenclave"``. Enabling this feature can | ||
| negatively impact security, as it allow the enclave to unseal files that were created | ||
| with an old potentially vulnerable CPU SVN version. It is the responsibility of the |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting issue: There are trailing spaces at the end of the line. These should be removed to conform to documentation standards.
| with an old potentially vulnerable CPU SVN version. It is the responsibility of the | |
| with an old potentially vulnerable CPU SVN version. It is the responsibility of the |
|
|
||
| .. doxygenfunction:: PalGetCPUSVN | ||
| :project: pal | ||
|
|
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting issue: There are trailing spaces at the end of the line. These should be removed to conform to documentation standards.
| .. doxygenfunction:: PalGetCPUSVN | |
| :project: pal | |
| .. doxygenfunction:: PalGetCPUSVN | |
| :project: pal |
amaury1093
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, but I do not have enough knowledge about the codebase.
Description of the changes
gramine.tcb_infofile. On startup if the current CPU SVN is different the files mounted in the directory are re-encrypted with the sealing key of the current CPU SVN. Did not include MRSigner based keys from the issue discussion it seems like migration can be done by signing a new enclave with the MRSigner key./dev/attestation/cpu_svnfile and get the cooresponding key with/dev/attestation/keys/svn/_sgx_mrenclave/<cpu_svn>/dev/attestation/cpu_svnfile. On the first run the enclave sets an older cpu svn and seals some files. Thegramine.tcb_infofile it set to the old svn. On the second run the files should have been automatically migrated. The enclave checks if the file contents match.How to test this PR?
This change is