aws-nitro: Separate guest kernel module loading, updates#549
aws-nitro: Separate guest kernel module loading, updates#549tylerfanelli wants to merge 4 commits intocontainers:mainfrom
Conversation
Nitro enclaves do not allow user input, so it remains unclear if debugging the init binary would be possible. Strip debug symbols to shrink the binary size. Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>
Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>
Start the debug output proxy immediately after the enclave has booted (if debug mode is enabled). Change the debug enclave argument to instead represent enabling application output, as nothing has to be done by the enclave VM if debug output is enabled. In this case, output will be printed to the console, which libkrun will already be connected to and forwarding data to/from. Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>
The krun-nitro-eif-ctl tool allows users to configure the kernel modules loaded within their enclave. This is done through a directory `/root` in the initrd. Read this directory and load each module into the enclave kernel within the bootstrap process. Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>
tylerfanelli
requested review from
MatiasVara,
dorindabassey,
jakecorrenti,
mtjhrc and
slp
as code owners
|
/gemini |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request refactors the kernel module loading mechanism by introducing a new mod.c file and separating debug output handling from application output. The debug flag has been consistently renamed to app_output across the C and Rust codebases, improving clarity. The changes also include an optimization to strip symbols from the aws-nitro init binary and a new feature to stream debug output from the serial console. Overall, the changes enhance modularity, clarify intent, and improve the build process.
| continue; | ||
|
|
||
| // Copy the full path of the module file. | ||
| sprintf(path, "%s/%s", KRUN_LINUX_MODS_DIR_NAME, entry->d_name); |
There was a problem hiding this comment.
Using sprintf is generally unsafe as it does not perform bounds checking, which can lead to buffer overflows if entry->d_name is unexpectedly long. It's safer to use snprintf to prevent potential security vulnerabilities.
| sprintf(path, "%s/%s", KRUN_LINUX_MODS_DIR_NAME, entry->d_name); | |
| snprintf(path, sizeof(path), "%s/%s", KRUN_LINUX_MODS_DIR_NAME, entry->d_name); |
| * If not running in debug mode, initialize the application output proxy. In | ||
| * debug mode, the enclave uses the console (which is already connected) for | ||
| * output. |
There was a problem hiding this comment.
The comment here could be updated for better clarity. Since args->app_output now explicitly indicates whether application output is enabled (which implies non-debug mode), the phrase "If not running in debug mode" is slightly redundant. Consider rephrasing to directly state the purpose of app_output.
For example, "If application output is enabled, initialize the application output proxy. Otherwise, the enclave uses the console for output."
| * If not running in debug mode, initialize the application output proxy. In | |
| * debug mode, the enclave uses the console (which is already connected) for | |
| * output. | |
| * If application output is enabled, initialize the application output proxy. Otherwise, | |
| * the enclave uses the console (which is already connected) for output. | |
| */ | ||
| int mods_load(void) | ||
| { | ||
| char path[MOD_FILE_NAME_BUF_SIZE + strlen(KRUN_LINUX_MODS_DIR_NAME) + 1]; |
There was a problem hiding this comment.
Using strlen on a string literal like KRUN_LINUX_MODS_DIR_NAME at runtime is less efficient than using sizeof for compile-time calculation. While it works, sizeof is generally preferred for fixed-size string literals to avoid unnecessary runtime overhead.
| char path[MOD_FILE_NAME_BUF_SIZE + strlen(KRUN_LINUX_MODS_DIR_NAME) + 1]; | |
| char path[MOD_FILE_NAME_BUF_SIZE + sizeof(KRUN_LINUX_MODS_DIR_NAME) + 1]; |
| valid = 1; | ||
| valid &= !(strcmp(entry->d_name, ".") == 0); | ||
| valid &= !(strcmp(entry->d_name, "..") == 0); |
There was a problem hiding this comment.
The valid checks can be simplified for better readability. Instead of separate valid &= lines, you can combine them into a single conditional or use strcmp directly in the if statement.
| valid = 1; | |
| valid &= !(strcmp(entry->d_name, ".") == 0); | |
| valid &= !(strcmp(entry->d_name, "..") == 0); | |
| if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0) | |
| continue; |
2 participants
No description provided.