cpu/vc: improve instruction decoder + handle more `IN` / `OUT` instructions + handle `RDMSR` / `WRMSR` + add SVSM discovery support #309

00xc · 2024-03-26T19:27:50Z

Simplify the instruction decoder and its API.
Decode remaining IN / OUT instruction cases and enable related #VC tests.
Handle SVM_EXIT_MSR in the #VC handler and enable related #VC tests.
Support SVSM discovery via MSR for post-boot services.
Support SVSM discovery via CPUID for post-boot services.

This works towards a more complete implementation of #14

00xc · 2024-03-26T19:38:55Z

@AdamCDunlap have you tested test_sev_snp_enablement_msr() in qemu? It fails for me.

00xc · 2024-03-26T20:36:50Z

@roy-hopkins added your review to verify that the gdb stub still works since there are some changes around X86_TRAP_DB.

AdamCDunlap · 2024-03-26T22:09:07Z

@AdamCDunlap have you tested test_sev_snp_enablement_msr() in qemu? It fails for me.

I have not, sorry. I would expect that one to work, though. I'm on vacation this week but I could check things out when I get back on Monday.

p4zuu · 2024-03-27T08:28:47Z

According to the SVSM specs, the #VC handler is also supposed to set the eax[28] bit when handling a CPUID(EAX=8000_001F). I think this is not implemented neither. A quick change of the cpuid #VC in-svsm test fails for me.
While you are at it, could you please adapt the CPUID handler and the related test to check the eax[28] bit of a CPUID(EAX=8000_001F) result? :)

00xc · 2024-03-27T14:06:35Z

According to the SVSM specs, the #VC handler is also supposed to set the eax[28] bit when handling a CPUID(EAX=8000_001F). I think this is not implemented neither. A quick change of the cpuid #VC in-svsm test fails for me. While you are at it, could you please adapt the CPUID handler and the related test to check the eax[28] bit of a CPUID(EAX=8000_001F) result? :)

Done. I simply adjusted the CPUID page on initialization instead of handling the specific case in the #VC handler.

00xc · 2024-04-15T06:32:53Z

@roy-hopkins could you take a brief look at the gdbstub-related changes?
@p4zuu could you approve the review if everything looks good?
@AdamCDunlap any chance you had time to take a look at the test I mentioned above?

roy-hopkins

Changes look good to me.
Tested the GDB stub from using this branch and it all still functions correctly.

p4zuu

Except the small comments I already left, this looks good to me

kernel/src/cpu/insn.rs

kernel/src/cpu/vc.rs

Replace C-style comments with Rust-like comments using two slashes. Signed-off-by: Carlos López <carlos.lopez@suse.com>

Return an enum when decoding an instruction, cleaning up the API. The new DecodedInsn describes each supported instruction and its operands. This also allows the #VC handler to verify that the decoded instruction matches the reported exit code. Signed-off-by: Carlos López <carlos.lopez@suse.com>

The IN/OUT instructions can specify an I/O port via register or via immediate. At the moment we are only decoding the first, so support the second to avoid issues in the future. While we are at it, enable related in-SVSM tests. Signed-off-by: Carlos López <carlos.lopez@suse.com>

Now that we've simplified the instruction decoder, remove uneeded fields and types. Signed-off-by: Carlos López <carlos.lopez@suse.com>

Add a new case to the #VC handler for RDMSR / WRMSR instructions. While we are at it, enable the relevant tests that we had disabled due to lack of support. The new handler simply copies relevant registers to the GHCB and issues a VMGExit. Signed-off-by: Carlos López <carlos.lopez@suse.com>

The SVSM spec states that post-boot elements may require discovering the presence of the SVSM, and can do so by reading a reserved MSR. Since we have implemented MSR decoding in the #VC handler, catch this specific case and return the expected value (the phsyical address of the SVSM Calling Area) to the guest. Signed-off-by: Carlos López <carlos.lopez@suse.com>

The SVSM spec states that post-boot elements may require discovering the presence of the SVSM, and can do so by reading bit 28 in EAX for CPUID(EAX=0x8000001F). To do so, set this bit when initializing the CPUID page for the SVSM. We do not need to do this for stage2 as post-boot elements will only come only come online after the main SVSM kernel has started, at which time the stage2 CPUID will no longer be in use. Signed-off-by: Carlos López <carlos.lopez@suse.com>

00xc · 2024-04-15T08:37:27Z

Addressed Thomas' comments and rebased on main.

AdamCDunlap · 2024-04-17T16:27:32Z

I checked out test_sev_snp_enablement_msr and it looks like it was written incorrectly. Here's a diff that fixes it and this passes for me:

diff --git a/kernel/src/cpu/vc.rs b/kernel/src/cpu/vc.rs
index e265f6f..0d8f3e2 100644
--- a/kernel/src/cpu/vc.rs
+++ b/kernel/src/cpu/vc.rs
@@ -508,13 +508,12 @@ mod tests {
     }

     #[test]
-    // #[cfg_attr(not(test_in_svsm), ignore = "Can only be run inside guest")]
-    #[ignore = "Currently unhandled by #VC handler"]
+    #[cfg_attr(not(test_in_svsm), ignore = "Can only be run inside guest")]
     fn test_sev_snp_enablement_msr() {
-        const MSR_SEV_STATUS: u32 = 0b10;
-        const MSR_SEV_STATUS_SEV_SNP_ENABLED: u64 = 0b100;
+        const MSR_SEV_STATUS: u32 = 0xc0010131;
+        const MSR_SEV_STATUS_SEV_SNP_ENABLED: u64 = 0b10;

-        let sev_status = verify_ghcb_gets_altered(|| read_msr(MSR_SEV_STATUS));
+        let sev_status = read_msr(MSR_SEV_STATUS);
         assert_ne!(sev_status & MSR_SEV_STATUS_SEV_SNP_ENABLED, 0);
     }

00xc · 2024-04-17T16:53:17Z

I checked out test_sev_snp_enablement_msr and it looks like it was written incorrectly.

Ah yes, I did notice the MSR_SEV_STATUS value being wrong. However, I'm not sure why the GHCB is not being modified. In fact, the rdmsr is not even generating a #VC exception as far as I can tell, which it should. Am I missing something?

AdamCDunlap · 2024-04-17T17:22:47Z

However, I'm not sure why the GHCB is not being modified.

I spent a while trying to debug this but I didn't get too far. I spent a while trying to figure out why adding print statements made the test pass before realizing that the print statements generate #VCs themselves which do modify the ghcb (I was searching for UB due to the unsafe). Our internal version of this test does not check that the ghcb was modified, either, which is why I was OK leaving it out.

I wasn't completely sure that the ghcb used in verify_ghcb_gets_altered is the same as the ghcb used in the vc handler. I was trying to print and compare their addresses and I thought they were different, but I got sidetracked and thought it wasn't too important.

In fact, the rdmsr is not even generating a #VC exception as far as I can tell, which it should

I added print statements in and it looks like the correct #VC was generated.

AdamCDunlap · 2024-04-17T18:00:23Z

In fact, the rdmsr is not even generating a #VC exception as far as I can tell, which it should

I added print statements in and it looks like the correct #VC was generated.

Actually I took a closer look and you're right, the #VC isn't generated for readmsr. I'm not sure why that is. Maybe only some msrs are trapped by #VC? Would need to take a closer look at the spec.

00xc · 2024-04-17T18:17:03Z

In fact, the rdmsr is not even generating a #VC exception as far as I can tell, which it should

I added print statements in and it looks like the correct #VC was generated.

Actually I took a closer look and you're right, the #VC isn't generated for readmsr. I'm not sure why that is. Maybe only some msrs are trapped by #VC? Would need to take a closer look at the spec.

I took a brief look and could not find anything, other than a slight hint in this sentence (AMD APM Rev. 3.40, section 15.34.10):

The SEV_STATUS MSR is read-only and accesses to the SEV_STATUS MSR cannot be intercepted by the hypervisor.

So perhaps, and this is a wild guess, reading this MSR never causes a #VC since there is no point? After all the whole idea of #VC is to set up the necessary state to enable some hypervisor involvement. Maybe @tlendacky can provide a better explanation.

The test had various mistakes, so fix them and enable the test: 1. The MSR number was wrong 2. The expected set bits were wrong 3. The RDMSR does not trigger a GHCB modification Suggested-by: Adam Dunlap <acdunlap@google.com> Signed-off-by: Carlos López <carlos.lopez@suse.com>

00xc · 2024-04-17T18:19:07Z

I checked out test_sev_snp_enablement_msr and it looks like it was written incorrectly. Here's a diff that fixes it and this passes for me:

diff --git a/kernel/src/cpu/vc.rs b/kernel/src/cpu/vc.rs
index e265f6f..0d8f3e2 100644
--- a/kernel/src/cpu/vc.rs
+++ b/kernel/src/cpu/vc.rs
@@ -508,13 +508,12 @@ mod tests {
     }

     #[test]
-    // #[cfg_attr(not(test_in_svsm), ignore = "Can only be run inside guest")]
-    #[ignore = "Currently unhandled by #VC handler"]
+    #[cfg_attr(not(test_in_svsm), ignore = "Can only be run inside guest")]
     fn test_sev_snp_enablement_msr() {
-        const MSR_SEV_STATUS: u32 = 0b10;
-        const MSR_SEV_STATUS_SEV_SNP_ENABLED: u64 = 0b100;
+        const MSR_SEV_STATUS: u32 = 0xc0010131;
+        const MSR_SEV_STATUS_SEV_SNP_ENABLED: u64 = 0b10;

-        let sev_status = verify_ghcb_gets_altered(|| read_msr(MSR_SEV_STATUS));
+        let sev_status = read_msr(MSR_SEV_STATUS);
         assert_ne!(sev_status & MSR_SEV_STATUS_SEV_SNP_ENABLED, 0);
     }

In the meantime I pushed this change. I added you in the Suggested-by tag of the commit.

00xc · 2024-04-17T19:30:09Z

Forgot to mention this before - the changes in this PR should prevent the WeSee attack (via "cpu/insn: improve instruction decoding"):
https://ahoi-attacks.github.io/wesee/

tlendacky · 2024-04-17T21:11:01Z

So perhaps, and this is a wild guess, reading this MSR never causes a #VC since there is no point? After all the whole idea of #VC is to set up the necessary state to enable some hypervisor involvement. Maybe @tlendacky can provide a better explanation.

The SEV_STATUS MSR is a non-interceptable MSR. Since it is non-interceptable it does not generate a #VC, it just executes.

00xc requested a review from p4zuu March 26, 2024 19:27

00xc force-pushed the cpu/vc-msr branch from 3560f51 to 1790e04 Compare March 26, 2024 20:27

00xc changed the title ~~cpu/vc: improve instruction decoder and handle RDMSR / WRMSR instructions~~ cpu/vc: improve instruction decoder + handle more IN / OUT instructions + handle RDMSR / WRMSR Mar 26, 2024

00xc requested a review from roy-hopkins March 26, 2024 20:35

00xc added the in-review PR is under active review and not yet approved label Mar 27, 2024

00xc force-pushed the cpu/vc-msr branch 2 times, most recently from 40291df to b692610 Compare March 27, 2024 14:03

00xc changed the title ~~cpu/vc: improve instruction decoder + handle more IN / OUT instructions + handle RDMSR / WRMSR~~ cpu/vc: improve instruction decoder + handle more IN / OUT instructions + handle RDMSR / WRMSR + add SVSM discovery support Mar 27, 2024

roy-hopkins approved these changes Apr 15, 2024

View reviewed changes

p4zuu approved these changes Apr 15, 2024

View reviewed changes

kernel/src/cpu/insn.rs Outdated Show resolved Hide resolved

kernel/src/cpu/vc.rs Outdated Show resolved Hide resolved

kernel/src/cpu/vc.rs Outdated Show resolved Hide resolved

00xc added 7 commits April 15, 2024 10:18

cpu/vc: switch to Rust-style comments

541ebce

Replace C-style comments with Rust-like comments using two slashes. Signed-off-by: Carlos López <carlos.lopez@suse.com>

cpu/insn: remove InsnBuffer and unneeded Instruction fields

8a51f7d

Now that we've simplified the instruction decoder, remove uneeded fields and types. Signed-off-by: Carlos López <carlos.lopez@suse.com>

00xc force-pushed the cpu/vc-msr branch from b692610 to fb4832b Compare April 15, 2024 08:35

00xc force-pushed the cpu/vc-msr branch from 5d1a6ec to 351735b Compare April 17, 2024 18:17

00xc added the ready-to-merge PR is ready for merging into main branch label Apr 18, 2024

joergroedel merged commit 0c9a9c0 into coconut-svsm:main Apr 22, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpu/vc: improve instruction decoder + handle more `IN` / `OUT` instructions + handle `RDMSR` / `WRMSR` + add SVSM discovery support #309

cpu/vc: improve instruction decoder + handle more `IN` / `OUT` instructions + handle `RDMSR` / `WRMSR` + add SVSM discovery support #309

00xc commented Mar 26, 2024 •

edited

00xc commented Mar 26, 2024

00xc commented Mar 26, 2024

AdamCDunlap commented Mar 26, 2024

p4zuu commented Mar 27, 2024

00xc commented Mar 27, 2024

00xc commented Apr 15, 2024

roy-hopkins left a comment

p4zuu left a comment

00xc commented Apr 15, 2024

AdamCDunlap commented Apr 17, 2024

00xc commented Apr 17, 2024

AdamCDunlap commented Apr 17, 2024

AdamCDunlap commented Apr 17, 2024

00xc commented Apr 17, 2024

00xc commented Apr 17, 2024

00xc commented Apr 17, 2024

tlendacky commented Apr 17, 2024

cpu/vc: improve instruction decoder + handle more IN / OUT instructions + handle RDMSR / WRMSR + add SVSM discovery support #309

cpu/vc: improve instruction decoder + handle more IN / OUT instructions + handle RDMSR / WRMSR + add SVSM discovery support #309

Conversation

00xc commented Mar 26, 2024 • edited

00xc commented Mar 26, 2024

00xc commented Mar 26, 2024

AdamCDunlap commented Mar 26, 2024

p4zuu commented Mar 27, 2024

00xc commented Mar 27, 2024

00xc commented Apr 15, 2024

roy-hopkins left a comment

Choose a reason for hiding this comment

p4zuu left a comment

Choose a reason for hiding this comment

00xc commented Apr 15, 2024

AdamCDunlap commented Apr 17, 2024

00xc commented Apr 17, 2024

AdamCDunlap commented Apr 17, 2024

AdamCDunlap commented Apr 17, 2024

00xc commented Apr 17, 2024

00xc commented Apr 17, 2024

00xc commented Apr 17, 2024

tlendacky commented Apr 17, 2024

cpu/vc: improve instruction decoder + handle more `IN` / `OUT` instructions + handle `RDMSR` / `WRMSR` + add SVSM discovery support #309

cpu/vc: improve instruction decoder + handle more `IN` / `OUT` instructions + handle `RDMSR` / `WRMSR` + add SVSM discovery support #309

00xc commented Mar 26, 2024 •

edited