soundness improvements around hypervisor-shared memory #451

Freax13 · 2024-08-30T07:17:16Z

This PR improves the soundness of code around hypervisor-shared memory.

The first patch, 174274d, is blocked on google/zerocopy#1601. Let me know if you want me to drop that patch if we don't want to wait on a new zerocopy release. I used the following patch to override zerocopy for testing:

diff --git a/Cargo.toml b/Cargo.toml
index b7bdb46..d87444d 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -49,6 +49,9 @@ zerocopy = { version = "0.7.32", features = ["alloc", "derive"] }
 # other repos
 packit = { git = "https://github.com/coconut-svsm/packit", version = "0.1.1" }
 
+[patch.crates-io]
+zerocopy = { git = "https://github.com/Freax13/zerocopy.git", rev = "68e1cc8" }
+
 [workspace.lints.rust]
 future_incompatible = { level = "deny", priority = 127 }
 nonstandard_style = { level = "deny", priority = 126 }

joergroedel · 2024-09-11T15:19:14Z

This PR improves the soundness of code around hypervisor-shared memory.

The first patch, 174274d, is blocked on google/zerocopy#1601. Let me know if you want me to drop that patch if we don't want to wait on a new zerocopy release.

Please move that patch to a separate draft-PR, which you can then "undraft" once all blockers are solved.

In general I like these changes, especially the SharedBox implementation. That will simplify a lot of things.

Once updated this needs testing by @msft-jlange and possibly also a review by @cclaudio .

Freax13 · 2024-09-12T09:22:56Z

Please move that patch to a separate draft-PR, which you can then "undraft" once all blockers are solved.

Done.

This makes it possible to implement get_aad_slice without any unsafe code. Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>

Given that the hypervisor has write access to that memory, we need to treat the memory as interiorly mutable. Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>

Freax13 · 2024-09-18T09:04:18Z

Just rebased onto main. I resolved the TODOs by switching to the functions in crate::cpu::mem.

msft-jlange · 2024-09-19T18:23:20Z

kernel/src/stage2.rs

-    this_cpu()
-        .shutdown()
-        .expect("Failed to shut down percpu data (including GHCB)");
+unsafe fn shutdown_percpu() {


Why is this function unsafe? At a minimum, there should be a Safety comment here. But there is also nothing about the function declaration that suggests that it can only be called from unsafe code. My understanding of the convention we have been using is that a function should only be declared unsafe if it is not possible to call it from safe code due to its parameters or return value (this was the position advocated by @00xc as I always understood it).

Why is this function unsafe? At a minimum, there should be a Safety comment here.

I added a safety section.

But there is also nothing about the function declaration that suggests that it can only be called from unsafe code. My understanding of the convention we have been using is that a function should only be declared unsafe if it is not possible to call it from safe code due to its parameters or return value (this was the position advocated by @00xc as I always understood it).

No, that's not how unsafe is used in Rust: A function must be marked as unsafe if (and usually only if) safe code using the function can cause undefined behavior (see the Rustonomicon). Whether or not a function must be marked as unsafe has very little to do with its signature and much more to do with what it actually does (you could make any unsafe function with any signature safe by replacing it's body with loop {}). In this case, the caller must ensure that the PerCpu instance isn't used after it's been dropped because doing so is UB. This isn't something that the compiler can enforce, so we have to mark the function as unsafe and shift the responsibility of upholding this to the user. This isn't a convention that's specified by us in this project, those are the rules for writing unsafe code in Rust.

I agree with your perspective on the use of unsafe. However, I once submitted a PR to this project that declared a function unsafe for similar reasons, and received feedback from @00xc that the function should not be declared this way, for the reasons I included in my earlier comment. I'm happy with your approach now that you've added a safety comment, and will leave it to @00xc to comment on whether this use of unsafe is appropriate.

msft-jlange · 2024-09-19T18:24:45Z

kernel/src/cpu/mem.rs

+/// This function has all the safety requirements of `core::ptr::read` except
+/// that data races are explicitly permitted.
+#[inline(always)]
+pub unsafe fn is_clear(src: usize, size: usize) -> bool {


This appears to be used only by test code. Should it therefore be within the test module so it is not accidentally referenced by non-test code?

This appears to be used only by test code.

This function is also referenced by SharedBox::is_clear which itself is used by send_extended_guest_request.

If this function is required for production code, then why is it written in assembly? In the interest of minimizing the use of unsafe code, it seems like we don't want to write anything in assembly unless it has to be done that way, and at a glance it appears that this function could be implemented in Rust instead of asm. If you have a strong justification for the use of assembly, then can you include that justification in the safety comments?

If performance is your primary concern, then there are many ways this loop could be improved (switching to SCASQ at a minimum, and better yet would be to use XMM). But if performance is not a primary concern, then avoiding assembly altogether would be best.

msft-jlange · 2024-09-19T18:26:16Z

kernel/src/mm/page_visibility.rs

+/// # Safety
+///
+/// Converting the memory at `vaddr` must be safe within Rust's memory model.
+/// Notably any objects at `vaddr` must tolerate unsychronized writes of any


Typo: unsychronized => unsynchronized.

Fixed, thanks!

msft-jlange · 2024-09-19T18:29:42Z

kernel/src/sev/ghcb.rs

+            .expect("Could not re-encrypt page");
+
+        // Unregister GHCB PA
+        register_ghcb_gpa_msr(PhysAddr::null()).expect("Could not unregister GHCB");


This is not guaranteed to succeed, because the VMM is not required to accept NULL as a valid GHCB page.

That's true, but this PR didn't introduce this code, it merely moved it around. We were already panicking before this PR. We should fix this in a later PR.

That is a reasonable position.

msft-jlange · 2024-09-19T18:32:05Z

kernel/src/sev/ghcb.rs

@@ -160,7 +162,28 @@ impl GhcbPage {

 impl Drop for GhcbPage {
    fn drop(&mut self) {


Is there any scenario for which dropping a GHCB is not associated with a fatal termination of the SVSM? The process of restoring a GHCB page to the private state is fragile, and I believe it would be best to avoid any attempt to do so unless we were aware of a valid use case for this. If we cannot come up with one, then it would be simplest for GhcbPage::drop() simply to panic, since we shouldn't get here anyway.

Is there any scenario for which dropping a GHCB is not associated with a fatal termination of the SVSM? The process of restoring a GHCB page to the private state is fragile, and I believe it would be best to avoid any attempt to do so unless we were aware of a valid use case for this. If we cannot come up with one, then it would be simplest for GhcbPage::drop() simply to panic, since we shouldn't get here anyway.

Yes, calling the shutdown_percpu function will cause the GhcbPage to be dropped. We do this to release the GHCB used by stage2 shortly before entering the svsm kernel. That said, this code wasn't introduced by this PR, this PR just moved that code around.

If we didn't need this, I'd would totally be on your side, we shouldn't write fragile code when we shouldn't need it in practice.

I'll take another look at this flow after your PR merges to see if we can be more robust here, but I accept your point that your PR isn't really changing the logic that exists today.

msft-jlange · 2024-09-19T18:34:03Z

kernel/src/sev/ghcb.rs

    pub fn clear(&self) {
        // Clear valid bitmap
-        self.valid_bitmap.set([0, 0]);
+        self.valid_bitmap[0].store(0, Ordering::SeqCst);


The GHCB page is not manipulated across processors, so there is no reason for expensive memory barriers when modifying its contents. The only race conditions we might expect are between the SVSM environment and the host running on the same processor; this possibility for interruption means that atomic operations are necessary, but Ordering::Relaxed will be sufficient for all such operations. That is true everywhere throughout this file.

Sure, sounds reasonable.

msft-jlange · 2024-09-19T18:36:08Z

kernel/src/mm/page_visibility.rs

@@ -78,3 +91,139 @@ pub fn make_page_private(vaddr: VirtAddr) -> Result<(), SvsmError> {

    Ok(())
 }
+
+/// SharedBox is a safe wrapper around memory pages shared with the host.
+pub struct SharedBox<T> {


This structure appears to have nothing to do with GHCB - the GhcbPage structure doesn't even make use of it - and thus it should be in its own source file and not stuck in the GHCB sources. In addition, the notion of SharedBox will be used on other confidential platforms (like TDX) and definitely should not be in the SEV sources directory.

I'm a little confused, SharedBox isn't declared in a file that has anything to do with the GHCB or SEV specific sources. It's declared in the same file as two other functions that a related to page visibility and that file is already platform independent.

Sorry, somehow I observed this code to be in kernel/src/sev/ghcb.rs. The current location is great.

msft-jlange · 2024-09-19T18:42:47Z

kernel/src/sev/hv_doorbell.rs

-    }
+/// Allocates a new HV doorbell page and registers it on the hypervisor
+/// using the given GHCB.
+pub fn allocate_hv_doorbell_page(ghcb: &GHCB) -> Result<&'static HVDoorbell, SvsmError> {


Why does this function not use SharedBox? It seems that SharedBox is designed to do exactly the sort of assignment and visibility management expected here, and unlike the GHCB, there is no chicken-and-egg problem because SharedBox only requires the existence of a GHCB, not of a doorbell page.

Good idea! I changed the function to use SharedBox.

Cell doesn't allow concurrent accesses. This is a problem because we share the memory with the host and the host could write to the memory while we're reading it. Use atomic accesses instead. Atomic accesses can tolerate concurrent writes. Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>

This method makes it possible to check if memory is clear without having to worry about data races. Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>

SharedBox is a safe wrapper around memory pages shared with the host. Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>

HVDoorbellPage was only used in one place and leak was immediately called on it. Given that we don't ever need to free up a doorbell page let's just implement this in a single function returning a static reference. Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>

This is better for a couple of reasons: 1. drop_in_place destroys the object rather than mutating it to release resources. The downside with simply mutating but not destroying is that the object still has to be in a valid state and this limits the shutdown code (for example it can't release the memory associated with a PageBox) 2. After the object has been dropped, it can't be accessed anymore. This means that the shutdown code doesn't have to worry about later accesses like the previous code had to. 3. All resources are freed, not just the GHCB. This also fixes a soundness issue where if the shutdown were to be called twice on the same GHCB that would result in a double-pvalidate bug. Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>

This impl is unused. It is also unsound because we can never have unique ownership over the GHCB as long as it is shared with the host. Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>

Now that the shutdown code is only called from the Drop impl we might as well move it in there. This also makes it impossible to call shutdown more than once (or to call shutdown and the Drop the GhcbPage). Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>

Freax13 force-pushed the more-zerocopy branch 3 times, most recently from bfe8671 to a084a9a Compare August 30, 2024 07:24

Freax13 mentioned this pull request Sep 12, 2024

greq: retire new_box_zeroed in favor of FromZeroes derive #454

Draft

Freax13 force-pushed the more-zerocopy branch from a084a9a to 333d1db Compare September 12, 2024 09:22

joergroedel requested review from msft-jlange and cclaudio September 16, 2024 12:23

joergroedel added the wait-for-review PR needs for approval by reviewers label Sep 16, 2024

Freax13 added 2 commits September 18, 2024 06:43

greq: derive AsBytes for SnpGuestRequestMsgHdr

baa6142

This makes it possible to implement get_aad_slice without any unsafe code. Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>

hv_doorbell: wrap reserved_63_4 in UnsafeCell

08bd0e5

Given that the hypervisor has write access to that memory, we need to treat the memory as interiorly mutable. Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>

Freax13 force-pushed the more-zerocopy branch from 333d1db to 7172851 Compare September 18, 2024 09:03

msft-jlange reviewed Sep 19, 2024

View reviewed changes

Freax13 force-pushed the more-zerocopy branch from 7172851 to 9320a20 Compare September 20, 2024 06:44

Freax13 force-pushed the more-zerocopy branch from 9320a20 to 00e638e Compare September 20, 2024 07:20

Freax13 added 5 commits September 20, 2024 07:45

mem: add is_clear

0603cf7

This method makes it possible to check if memory is clear without having to worry about data races. Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>

mm: implement SharedBox

4d10b5e

SharedBox is a safe wrapper around memory pages shared with the host. Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>

ghcb: remove DerefMut impl

0fe7e31

This impl is unused. It is also unsound because we can never have unique ownership over the GHCB as long as it is shared with the host. Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>

Freax13 force-pushed the more-zerocopy branch from 00e638e to a5acf4a Compare September 20, 2024 08:30

Freax13 requested a review from msft-jlange September 20, 2024 08:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

soundness improvements around hypervisor-shared memory #451

soundness improvements around hypervisor-shared memory #451

Freax13 commented Aug 30, 2024

joergroedel commented Sep 11, 2024

Freax13 commented Sep 12, 2024

Freax13 commented Sep 18, 2024

msft-jlange Sep 19, 2024

Freax13 Sep 20, 2024

msft-jlange Sep 20, 2024

msft-jlange Sep 19, 2024

Freax13 Sep 20, 2024

msft-jlange Sep 20, 2024

msft-jlange Sep 19, 2024

Freax13 Sep 20, 2024

msft-jlange Sep 19, 2024

Freax13 Sep 20, 2024

msft-jlange Sep 20, 2024

msft-jlange Sep 19, 2024

Freax13 Sep 20, 2024

msft-jlange Sep 20, 2024

msft-jlange Sep 19, 2024 •

edited

Loading

Freax13 Sep 20, 2024

msft-jlange Sep 19, 2024

Freax13 Sep 20, 2024

msft-jlange Sep 20, 2024

msft-jlange Sep 19, 2024

Freax13 Sep 20, 2024

		@@ -160,7 +162,28 @@ impl GhcbPage {

		impl Drop for GhcbPage {
		fn drop(&mut self) {

soundness improvements around hypervisor-shared memory #451

Are you sure you want to change the base?

soundness improvements around hypervisor-shared memory #451

Conversation

Freax13 commented Aug 30, 2024

joergroedel commented Sep 11, 2024

Freax13 commented Sep 12, 2024

Freax13 commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-jlange Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-jlange Sep 19, 2024 •

edited

Loading