Skip to content
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

Dcap 04 #1629

Merged
merged 77 commits into from
Apr 11, 2024
Merged

Dcap 04 #1629

merged 77 commits into from
Apr 11, 2024

Conversation

valdok
Copy link
Contributor

@valdok valdok commented Mar 7, 2024

No description provided.

Copy link
Member

@Cashmaney Cashmaney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, it's quite a bit of code but I think I reviewed most of it. I focused mainly on the stuff inside the enclave since that should be most of the critical path. Either way, I think these comments are a good place to start.

For the most part it looks good - I think especially for someone that hasn't had much experience with Rust it's in a pretty good state.

Some general stuff:

I suggest splitting all the dcap and EPID stuff into different files - it's not entirely clear where one ends and where another begins, and so it's hard to see whether all the code paths that currently rely on epid have been ported to dcap.

We could also use a bit more comments on what exactly is going on here, as all the validation stuff isn't trivial.

Try to use more informative naming where possible (size_dcap_q or size_dcap_s are not great)

If you're using unsafe anywhere you should comment on why you're doing that, and why it's okay - if you're not specifically doing FFI a lot of the times you might find that you're doing something unnecessary

@valdok valdok dismissed Cashmaney’s stale review April 11, 2024 13:26

fixed (mostly)

@valdok valdok marked this pull request as ready for review April 11, 2024 13:27
@valdok valdok merged commit 819f054 into master Apr 11, 2024
12 of 13 checks passed
@valdok valdok deleted the dcap-04 branch April 11, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants