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

kernel/cpu: add safety comments #584

Merged
merged 3 commits into from
Mar 6, 2025
Merged

Conversation

p4zuu
Copy link
Collaborator

@p4zuu p4zuu commented Dec 20, 2024

Safety comments for:

  • stac and clac instructions
  • some TSS-related pieces of GDT code
  • as described in kernel/cpu: add safety comments #553 write_msr() should be unsafe since it can break memory safety. This part is still missing two comments here and here in the GHCB MSR protocol where I have doubt if the MSR request is subject to breaking memory safety or not, specifically SNP_REG_GHCB_GPA_REQ and SNP_STATE_CHANGE_REQ GHCB requests. My guess is a garbage request parameter can break the platform, but does it still count as memory safety?

@msft-jlange
Copy link
Collaborator

I have doubt if the MSR request is subject to breaking memory safety or not, specifically SNP_REG_GHCB_GPA_REQ and SNP_STATE_CHANGE_REQ GHCB requests. My guess is a garbage request parameter can break the platform, but does it still count as memory safety?

Both of these GHCB operations can cause physical memory to be remapped (one may cause the GHCB data to appear in a different place, and one may cause memory to be remapped with a different encryption attribute). Because both of these operations can cause mapped memory to change contents, they are technically unsafe.

@msft-jlange msft-jlange added the in-review PR is under active review and not yet approved label Dec 20, 2024
@p4zuu
Copy link
Collaborator Author

p4zuu commented Feb 1, 2025

Both of these GHCB operations can cause physical memory to be remapped (one may cause the GHCB data to appear in a different place, and one may cause memory to be remapped with a different encryption attribute). Because both of these operations can cause mapped memory to change contents, they are technically unsafe.

Thanks for your input on this Jon, and sorry for the massively delayed update. I entered a busy relocation mode and will start working on this again in a couple of weeks hopefully.

stac and clac instructions don't break memory safety.

Signed-off-by: Thomas Leroy <[email protected]>
@p4zuu p4zuu force-pushed the cpu_unsafe branch 2 times, most recently from fae85db to fdcc08d Compare February 28, 2025 23:32
@p4zuu
Copy link
Collaborator Author

p4zuu commented Mar 1, 2025

Thanks for the initial review @msft-jlange. I addressed the comments and this is open again for review

p4zuu added 2 commits March 1, 2025 10:27
Writing to some MSRs can break memory safety. Therefore, write_msr()
should be unsafe.

Signed-off-by: Thomas Leroy <[email protected]>
Missing safety comments were missing in GDT code. Adding a couple of
checks to validate the safety requirements.
Also, to ensure we call `ltr` instruction with a tss that has a valid
lifetime, we can set `load_tss()` to take a static tss reference.

Signed-off-by: Thomas Leroy <[email protected]>
Copy link
Collaborator

@msft-jlange msft-jlange left a comment

Choose a reason for hiding this comment

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

Thanks for returning to this PR! These changes look good.

@msft-jlange msft-jlange added ready-to-merge PR is ready for merging into main branch and removed in-review PR is under active review and not yet approved labels Mar 3, 2025
@joergroedel joergroedel merged commit fe4b8d9 into coconut-svsm:main Mar 6, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge PR is ready for merging into main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants