[PATCH v8 08/32] OvmfPkg/ResetVector: use SEV-SNP-validated CPUID values


Brijesh Singh
 

From: Michael Roth <michael.roth@amd.com>

CPUID instructions are issued during early boot to do things like probe
for SEV support. Currently these are handled by a minimal #VC handler
that uses the MSR-based GHCB protocol to fetch the CPUID values from
the hypervisor. When SEV-SNP is enabled, use the firmware-validated
CPUID values from the CPUID page instead [1].

[1]: SEV SNP Firmware ABI Specification, Rev. 0.8, 8.13.2.6

Cc: Michael Roth <michael.roth@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
Signed-off-by: Michael Roth <michael.roth@amd.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/ResetVector/Ia32/AmdSev.asm | 80 +++++++++++++++++++++++++++--
1 file changed, 75 insertions(+), 5 deletions(-)

diff --git a/OvmfPkg/ResetVector/Ia32/AmdSev.asm b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
index 48d9178168b0..1f827da3b929 100644
--- a/OvmfPkg/ResetVector/Ia32/AmdSev.asm
+++ b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
@@ -34,6 +34,18 @@ BITS 32
%define GHCB_CPUID_REGISTER_SHIFT 30
%define CPUID_INSN_LEN 2

+; #VC handler offsets/sizes for accessing SNP CPUID page
+;
+%define SNP_CPUID_ENTRY_SZ 48
+%define SNP_CPUID_COUNT 0
+%define SNP_CPUID_ENTRY 16
+%define SNP_CPUID_ENTRY_EAX_IN 0
+%define SNP_CPUID_ENTRY_ECX_IN 4
+%define SNP_CPUID_ENTRY_EAX 24
+%define SNP_CPUID_ENTRY_EBX 28
+%define SNP_CPUID_ENTRY_ECX 32
+%define SNP_CPUID_ENTRY_EDX 36
+

%define SEV_GHCB_MSR 0xc0010130
%define SEV_STATUS_MSR 0xc0010131
@@ -335,11 +347,61 @@ SevEsIdtNotCpuid:
TerminateVmgExit TERM_VC_NOT_CPUID
iret

- ;
- ; Total stack usage for the #VC handler is 44 bytes:
- ; - 12 bytes for the exception IRET (after popping error code)
- ; - 32 bytes for the local variables.
- ;
+; Use the SNP CPUID page to handle the cpuid lookup
+;
+; Modified: EAX, EBX, ECX, EDX
+;
+; Relies on the stack setup/usage in #VC handler:
+;
+; On entry,
+; [esp + VC_CPUID_FUNCTION] contains EAX input to cpuid instruction
+;
+; On return, stores corresponding results of CPUID lookup in:
+; [esp + VC_CPUID_RESULT_EAX]
+; [esp + VC_CPUID_RESULT_EBX]
+; [esp + VC_CPUID_RESULT_ECX]
+; [esp + VC_CPUID_RESULT_EDX]
+;
+SnpCpuidLookup:
+ mov eax, [esp + VC_CPUID_FUNCTION]
+ mov ebx, [CPUID_BASE + SNP_CPUID_COUNT]
+ mov ecx, CPUID_BASE + SNP_CPUID_ENTRY
+ ; Zero these out now so we can simply return if lookup fails
+ mov dword[esp + VC_CPUID_RESULT_EAX], 0
+ mov dword[esp + VC_CPUID_RESULT_EBX], 0
+ mov dword[esp + VC_CPUID_RESULT_ECX], 0
+ mov dword[esp + VC_CPUID_RESULT_EDX], 0
+
+SnpCpuidCheckEntry:
+ cmp ebx, 0
+ je VmmDoneSnpCpuid
+ cmp dword[ecx + SNP_CPUID_ENTRY_EAX_IN], eax
+ jne SnpCpuidCheckEntryNext
+ ; As with SEV-ES handler we assume requested CPUID sub-leaf/index is 0
+ cmp dword[ecx + SNP_CPUID_ENTRY_ECX_IN], 0
+ je SnpCpuidEntryFound
+
+SnpCpuidCheckEntryNext:
+ dec ebx
+ add ecx, SNP_CPUID_ENTRY_SZ
+ jmp SnpCpuidCheckEntry
+
+SnpCpuidEntryFound:
+ mov eax, [ecx + SNP_CPUID_ENTRY_EAX]
+ mov [esp + VC_CPUID_RESULT_EAX], eax
+ mov eax, [ecx + SNP_CPUID_ENTRY_EBX]
+ mov [esp + VC_CPUID_RESULT_EBX], eax
+ mov eax, [ecx + SNP_CPUID_ENTRY_EDX]
+ mov [esp + VC_CPUID_RESULT_ECX], eax
+ mov eax, [ecx + SNP_CPUID_ENTRY_ECX]
+ mov [esp + VC_CPUID_RESULT_EDX], eax
+ jmp VmmDoneSnpCpuid
+
+;
+; Total stack usage for the #VC handler is 44 bytes:
+; - 12 bytes for the exception IRET (after popping error code)
+; - 32 bytes for the local variables.
+;
SevEsIdtVmmComm:
;
; If we're here, then we are an SEV-ES guest and this
@@ -367,6 +429,13 @@ SevEsIdtVmmComm:
; Save the CPUID function being requested
mov [esp + VC_CPUID_FUNCTION], eax

+ ; If SEV-SNP is enabled, use the CPUID page to handle the CPUID
+ ; instruction.
+ mov ecx, SEV_STATUS_MSR
+ rdmsr
+ bt eax, 2
+ jc SnpCpuidLookup
+
; The GHCB CPUID protocol uses the following mapping to request
; a specific register:
; 0 => EAX, 1 => EBX, 2 => ECX, 3 => EDX
@@ -424,6 +493,7 @@ VmmDone:
mov ecx, SEV_GHCB_MSR
wrmsr

+VmmDoneSnpCpuid:
mov eax, [esp + VC_CPUID_RESULT_EAX]
mov ebx, [esp + VC_CPUID_RESULT_EBX]
mov ecx, [esp + VC_CPUID_RESULT_ECX]
--
2.25.1


Gerd Hoffmann
 

Hi,

+ ; If SEV-SNP is enabled, use the CPUID page to handle the CPUID
+ ; instruction.
+ mov ecx, SEV_STATUS_MSR
+ rdmsr
+ bt eax, 2
+ jc SnpCpuidLookup
Maybe check SNP_CPUID_COUNT instead, so the cpuid page can also be used
without SEV-SNP ?

take care,
Gerd


Michael Roth <michael.roth@...>
 

On Wed, Sep 22, 2021 at 09:55:58AM +0200, Gerd Hoffmann wrote:
Hi,

+ ; If SEV-SNP is enabled, use the CPUID page to handle the CPUID
+ ; instruction.
+ mov ecx, SEV_STATUS_MSR
+ rdmsr
+ bt eax, 2
+ jc SnpCpuidLookup
Maybe check SNP_CPUID_COUNT instead, so the cpuid page can also be used
without SEV-SNP ?
One issue with that is that the contents of the CPUID page are not part
of guest measurement that will be checked later during attestation (only
the metadata such as page type/location is recorded in the measurement).

So if someone on the host slipped in, say, a malicious QEMU, and modified
it to zero out the CPUID page prior to launching the guest, it would end up
being accepted by firmware as legitimate CPUID table encoding 0 entries. So
implementing the check based on SNP_CPUID_COUNT would make it easy to bypass
the CPUID page in such a scenario, and even worse, they'd be able to get
all the way past attestation, since the CPUID metadata is the same, it's
only the contents that have changed.

Since the CPUID page is required by SNP, the approach taken here is to
always utilize it when SNP is enabled. In that case, if someone were to
maliciously zero out the CPUID page, it would still get used by the
guest, rather than bypassed, in which case the guest would never make it
to attestation since bits that get checked for early like
SEV/SEV-ES/SEV-SNP flags in 0x8000001F would all be 0.

That said, for the !SNP case, additional handling *could* be added to make
use of the CPUID page, but in that case it wouldn't be validated by firmware,
so isn't much better security-wise than asking KVM. It might be possible to
bake the CPUID page into the measurement to ensure integrity, but that
requires accounting for the CPUID page along with all the other elements of
the initial payload (like OVMF), and unlike with OVMF, the CPUID values
will vary often depending on guest configuration, and so cloud providers
would need to provide some sort of tooling to export this CPUID page to the
guest owner so it can be verified and accounted for in attestation, which
doesn't seem likely to get much uptake (and is probably at least partly why
the CPUID page contents aren't included in the measurement for SNP).


take care,
Gerd


Gerd Hoffmann
 

Hi,

One issue with that is that the contents of the CPUID page are not part
of guest measurement that will be checked later during attestation (only
the metadata such as page type/location is recorded in the measurement).

[ more details snipped ]
Thanks, that makes sense.

That said, for the !SNP case, additional handling *could* be added to make
use of the CPUID page, but in that case it wouldn't be validated by firmware,
so isn't much better security-wise than asking KVM.
Well, the intention would be more to (a) be able to test the code
without SNP hardware (for example in public CI) and (b) avoid trapping
into kvm if we don't have to.

It is clearly not a priority though, we can look into that once all the
SNP bits are merged in edk2 and qemu.

take care,
Gerd