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

Updated README \w Frontal Attack publication links #37

Merged
merged 1 commit into from
Aug 13, 2021

Conversation

dn0sar
Copy link
Contributor

@dn0sar dn0sar commented Aug 13, 2021

No description provided.

@jovanbulck jovanbulck merged commit e4cc283 into jovanbulck:master Aug 13, 2021
@jovanbulck
Copy link
Owner

jovanbulck commented Aug 13, 2021

Hi Ivan,

Thanks for the PR, looks good! And many congrats for having Frontal accepted at USENIX!! :-) 🎉

Very nice work, I enjoyed reading the paper! Also kudos for open sourcing the code and passing artifact evaluation 👍

I plan to do some much-needed refactoring for SGX-Step some time in the future, and then maybe it makes sense to also upstream some of the improvements from your repo? TBD

@dn0sar dn0sar deleted the readme_pull_request branch August 14, 2021 07:56
@dn0sar
Copy link
Contributor Author

dn0sar commented Aug 14, 2021

Hi Jo,

Thanks for accepting the PR so quickly :)

TBH the most important change we did is not needed anymore as I saw that you implemented it already in the meantime. Particularly, I saw that now the irq_handler can be installed so it does not cause a #GPF anymore if its fired while the kernel is executing.
We went for a kernel patch for that, but the way you solved the problem is much cleaner and easier to use IMO.

Other things that might be integrated:

  1. We moved the setting of the APIC timer to the aep_trampoline after the aep_cb_func (see here) and slightly changed the way instructions are serialized before ERESUME. We found that this helped in reducing the noise from the experiments, and it made the timing measurement's standard deviation smaller.

  2. Another thing that we found helped a lot in terms of measurement noise was to linearize the aep_cb_func as much as possible (see comment here). Perhaps it might make sense to add some comments there in your repo as well.

  3. Finally, one thing I regretted not implementing is an automatic TIMER_INTERVAL finder. This would have helped a lot during the artifact evaluation. Now that the #GPF does not happen anymore, it should not be difficult to implement this in a stable way, and I'd be happy to contribute to this if you want.

In any case, let me know if you want to integrate any of these and what the best way for that would be. I suppose just opening pull requests from my side? But let me know if any of these requires a bit more coordination between us (feel free to shoot me an email in that case).

Btw, many thanks for open sourcing and maintaining sgx-step. It has been a great resource for us!

Ps. You might also want to check a summary of some of the other things we tried to stabilize sgx-step here but I think I listed already the most important stuff above :)

@dn0sar
Copy link
Contributor Author

dn0sar commented Aug 14, 2021

PPs. Another thing I remembered that could be ported upstream from our changes is support for recording the performance counters while single-stepping.

@jovanbulck
Copy link
Owner

Hi Ivan,

Sounds great, thanks for summarizing the changes, that really helps! Some thoughts below:

TBH the most important change we did is not needed anymore as I saw that you implemented it already in the meantime. Particularly, I saw that now the irq_handler can be installed so it does not cause a #GPF anymore if its fired while the kernel is executing.
We went for a kernel patch for that, but the way you solved the problem is much cleaner and easier to use IMO.

Yes, this was a very annoying issue, that's fortunately fixed now. I see a kernel patch may have helped, nice hack! Ideally I want SGX-Step to work drop-in on stock Linux kernels, so the trick and current solution to avoid this altogether turned out to be simply executing the irq_handler with CPL 0.

We moved the setting of the APIC timer to the aep_trampoline after the aep_cb_func (see here) and slightly changed the way instructions are serialized before ERESUME. We found that this helped in reducing the noise from the experiments, and it made the timing measurement's standard deviation smaller.

This makes a lot of sense! I agree this approach is cleaner, and good to hear it reduced the noise/std dev. Let's merge this.

Another thing that we found helped a lot in terms of measurement noise was to linearize the aep_cb_func as much as possible (see comment here). Perhaps it might make sense to add some comments there in your repo as well.

Yes, also makes sense. I agree this is best documented in a comment and/or guideline in the README. Let's merge this.

Finally, one thing I regretted not implementing is an automatic TIMER_INTERVAL finder. This would have helped a lot during the artifact evaluation. Now that the #GPF does not happen anymore, it should not be difficult to implement this in a stable way, and I'd be happy to contribute to this if you want.

Yes, also agreed this would be a very useful addition that seems feasible, especially now that single-stepping is more stable. This has been on my list of todos for a very long time already (issue #4 ), but never made time to code something up. I guess the easiest is to do this in some kind of shell script and/or a custom main.c runner that uses a sort of informed binary search to find the optimal TIMER_INTERVAL for the platform. If you'd want to take a shot at this now or later, I'd always be very happy to merge! Even if it's not entirely crash-free, such a helper script would still be of great use I agree.

PPs. Another thing I remembered that could be ported upstream from our changes is support for recording the performance counters while single-stepping.

Nice, that sounds extremely useful. I'd be happy to merge this upstream as well!

In any case, let me know if you want to integrate any of these and what the best way for that would be. I suppose just opening pull requests from my side? But let me know if any of these requires a bit more coordination between us (feel free to shoot me an email in that case).

I think all of your above changes make sense (i.e., apart from the kernel patch, which, as you said, is fortunately not needed anymore in the current upstream) and I'd be happy to merge them upstream. The easiest is indeed to open PRs for each separate issue and then I'll review them and we can discuss possible changes in the PR via GitHub. If needed, we can always open another communication channel with more bandwidth, but where possible I suggest to use the GitHub interface to discuss code.

Re timing, I have several ongoing things atm and a holiday, so I'll probably only have time to review any PRs from end of September onward. So take your time when it's most comfortable for you. Feel free to open PRs whenever you'd like (before end of September or later), and I'll simply leave them open until I have time to look at them, which could take some time but I'll certainly do!

Btw, many thanks for open sourcing and maintaining sgx-step. It has been a great resource for us!

Happy to hear!! :-)

@dn0sar
Copy link
Contributor Author

dn0sar commented Aug 23, 2021

Sounds good!

I'm currently also on vacation and given other deadlines I think realistically I can push some stuff upstream only at the beginning of October.

In any case, I hope you are enjoying your holidays :)

Cheers,
Ivan

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