-
Notifications
You must be signed in to change notification settings - Fork 352
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
provide settings configuration to set config_id/config_svn on ice lake platform enclaves #3799
Conversation
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.
Thanks for taking the old PR forwards! See comments for suggestions and concerns.
include/openenclave/host.h
Outdated
*/ | ||
typedef struct _oe_enclave_setting_config_data | ||
{ | ||
uint8_t enclave_config_id[64]; |
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.
According to the spec and #3054, the config id should be 32 bytes. Could you double check?
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.
uint8_t enclave_config_id[64]; | |
uint8_t config_id[32]; |
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.
Add @bodzhang
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.
Hi @bodzhang , Can you confirm if this is the right data structure to use for SECS data structure.
I am planning to replicate the below intels secs_t structure in OE codebase and in intel codebase config_id is allocated 64 bytes. https://github.com/intel/linux-sgx/blob/d3bd1571240bcdf85734c232a4f0c86828443ebb/common/inc/internal/arch.h#L59
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.
There seem to be an inconsistent in the Intel manual. According to the table 37-2 (secs) and 37-23 (report), the configid field should be 64 bytes. However, the text in 38.4.1.3 mentions that "enclave is created the platform can additionally provide 32-byte configuration identifier (CONFIGID).". I think it should be fine to keep 64 bytes for now.
@@ -292,7 +292,10 @@ typedef struct _sgx_secs | |||
uint8_t reserved3[96]; /* 160 */ | |||
uint16_t isvvprodid; /* 256 */ | |||
uint16_t isvsvn; /* 258 */ | |||
uint8_t reserved[3836]; /* 260 */ | |||
uint8_t reserved4[20]; /* 260 */ | |||
uint8_t config_id[64]; /* 280 */ |
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.
Please check if this is 32 or 64 bytes.
f4e30fe
to
620c5fe
Compare
620c5fe
to
4087b55
Compare
4087b55
to
04db2e1
Compare
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.
LGTM! Thanks for addressing all the feedbacks.
04db2e1
to
88fbafb
Compare
88fbafb
to
37d8932
Compare
bors r+ |
3799: provide settings configuration to set config_id/config_svn on ice lake platform enclaves r=mingweishih a=manojrupireddy This is in continuation to the work done by Alvin => #3735. Made changes to the above draft PR , based on the code review comments there. Signed-off-by: manoj rupireddy <marupire@microsoft.com> Co-authored-by: manoj rupireddy <marupire@microsoft.com>
Build failed: |
37d8932
to
039f5e6
Compare
bors r+ |
🔒 Permission denied Existing reviewers: click here to make manojrupireddy a reviewer |
bors r+ |
3799: provide settings configuration to set config_id/config_svn on ice lake platform enclaves r=mingweishih a=manojrupireddy This is in continuation to the work done by Alvin => #3735. Made changes to the above draft PR , based on the code review comments there. Signed-off-by: manoj rupireddy <marupire@microsoft.com> Co-authored-by: manoj rupireddy <marupire@microsoft.com>
Build failed: |
bors delegate+ |
✌️ manojrupireddy can now approve this pull request. To approve and merge a pull request, simply reply with |
…e platform enclaves Signed-off-by: manoj rupireddy <marupire@microsoft.com>
039f5e6
to
1dcccd7
Compare
bors r+ |
3799: provide settings configuration to set config_id/config_svn on ice lake platform enclaves r=manojrupireddy a=manojrupireddy This is in continuation to the work done by Alvin => #3735. Made changes to the above draft PR , based on the code review comments there. Signed-off-by: manoj rupireddy <marupire@microsoft.com> Co-authored-by: manoj rupireddy <marupire@microsoft.com>
Build failed: |
bors r+ |
3799: provide settings configuration to set config_id/config_svn on ice lake platform enclaves r=manojrupireddy a=manojrupireddy This is in continuation to the work done by Alvin => #3735. Made changes to the above draft PR , based on the code review comments there. Signed-off-by: manoj rupireddy <marupire@microsoft.com> 3826: replacing constant salt with per-file random salt r=mingweishih a=RRathna replacing constant salt, and generating it randomly per file to be encrypted. Fixes #3692 3831: Update the oeedger8r submodule r=mingweishih a=mingweishih This PR sync up the oeedger8r against the master branch, which introduces the following changes: - Enforce the safe multiplication (see the [PR](openenclave/oeedger8r-cpp#31)). Fix #2390 - Add checks against pointer array arguments (see the [PR](openenclave/oeedger8r-cpp#65)). Fix #3557 - Make the oeedger8r-generated variables conform OE guidelines (see the [PR](openenclave/oeedger8r-cpp#66)). Fix #3210 - Add new warning options for the non-serializable cases (see the [PR](openenclave/oeedger8r-cpp#67)). Fix #3513 Co-authored-by: manoj rupireddy <marupire@microsoft.com> Co-authored-by: rrathna <rathna1993@gmail.com> Co-authored-by: Ming-Wei Shih <mishih@microsoft.com>
This PR was included in a batch that was canceled, it will be automatically retried |
bors r- |
Canceled. |
bors r+ |
3799: provide settings configuration to set config_id/config_svn on ice lake platform enclaves r=mingweishih a=manojrupireddy This is in continuation to the work done by Alvin => #3735. Made changes to the above draft PR , based on the code review comments there. Signed-off-by: manoj rupireddy <marupire@microsoft.com> 3824: Avoid recompiling common files for each ssl test r=mingweishih a=anakrish This reduces the number of files being compiled and also allows cmake to parallelize builds better. Most dev machines should see some drop in build times. E.g: In my machine, tests/openssl build time drops from 192 secs to 90 secs. Signed-off-by: Anand Krishnamoorthi <anakrish@microsoft.com> 3826: replacing constant salt with per-file random salt r=mingweishih a=RRathna replacing constant salt, and generating it randomly per file to be encrypted. Fixes #3692 Co-authored-by: manoj rupireddy <marupire@microsoft.com> Co-authored-by: Anand Krishnamoorthi <anakrish@microsoft.com> Co-authored-by: rrathna <rathna1993@gmail.com>
Build failed (retrying...): |
Build succeeded: |
This is in continuation to the work done by Alvin => #3735.
Made changes to the above draft PR , based on the code review comments there.
Signed-off-by: manoj rupireddy marupire@microsoft.com