[edk2-devel] [PATCH RFC v2 17/28] OvmfPkg/ResetVector: Invalidate the GHCB page

Brijesh Singh posted 28 patches 1 month, 2 weeks ago

[edk2-devel] [PATCH RFC v2 17/28] OvmfPkg/ResetVector: Invalidate the GHCB page

Posted by Brijesh Singh 1 month, 2 weeks ago
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

When SEV-SNP is active, the GHCB page is mapped un-encrypted in the
initial page table built by the reset vector code. Just clearing the
encryption attribute from the page table is not enough. The page also
needs to be added as shared in the RMP table.

The GHCB page was part of the pre-validated memory range specified
through the SnpBootBlock GUID. To maintain the security guarantees,
we must invalidate the GHCB page before clearing the encryption
attribute from the page table, and add the page shared in the RMP
table.

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: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 OvmfPkg/ResetVector/Ia32/PageTables64.asm | 82 ++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
index 6bf4a3524a..9949fcdfba 100644
--- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
+++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
@@ -70,9 +70,78 @@ BITS    32
 %define GHCB_HYPERVISOR_FEATURES_REQUEST    128
 %define GHCB_HYPERVISOR_FEATURES_RESPONSE   129
 
+; GHCB Page Invalidate request and response protocol values
+;
+%define GHCB_PAGE_STATE_CHANGE_REQUEST    20
+%define GHCB_PAGE_STATE_CHANGE_RESPONSE   21
+%define GHCB_PAGE_STATE_SHARED            2
+
 ; GHCB request to terminate protocol values
 %define GHCB_GENERAL_TERMINATE_REQUEST    255
 
+; If its an SEV-SNP guest then use the page state change VMGEXIT to invalidate
+; the GHCB page.
+;
+; Modified:  EAX, EBX, ECX, EDX
+;
+InvalidateGHCBPage:
+    ; Check if it is SEV-SNP guest.
+    cmp     byte[SEV_ES_WORK_AREA_SNP], 1
+    jne     InvalidateGHCBPageDone
+
+    ; Check whether hypervisor features has SEV-SNP (BIT0) set to indicate that
+    ; hypervisor supports the page state change.
+    mov     eax, dword[SEV_ES_WORK_AREA_HYPERVISOR_FEATURES]
+    bt      eax, 0
+    jnc     TerminateSevGuestLaunch
+
+    ; Use PVALIDATE instruction to invalidate the page
+    mov     eax, GHCB_BASE
+    mov     ecx, 0
+    mov     edx, 0
+    DB      0xF2, 0x0F, 0x01, 0xFF
+    cmp     eax, 0
+    jnz     TerminateSevGuestLaunch
+
+    ; Ask hypervisor to change the page state to shared using the
+    ; Page State Change VMGEXIT.
+    ;
+    ; Setup GHCB MSR
+    ;   GHCB_MSR[55:52] = Page Operation
+    ;   GHCB_MSR[51:12] = Guest Physical Frame Number
+    ;   GHCB_MSR[11:0]  = Page State Change Request
+    ;
+    mov     eax, (GHCB_BASE >> 12)
+    shl     eax, 12
+    or      eax, GHCB_PAGE_STATE_CHANGE_REQUEST
+    mov     edx, (GHCB_PAGE_STATE_SHARED << 20)
+    mov     ecx, 0xc0010130
+    wrmsr
+
+    ;
+    ; Issue VMGEXIT - NASM doesn't support the vmmcall instruction in 32-bit
+    ; mode, so work around this by temporarily switching to 64-bit mode.
+    ;
+BITS    64
+    rep     vmmcall
+BITS    32
+
+    ;
+    ; Response GHCB MSR
+    ;   GHCB_MSR[51:12] = Guest Physical Frame Number
+    ;   GHCB_MSR[11:0]  = Page State Change Response
+    ;
+    mov     ecx, 0xc0010130
+    rdmsr
+    and     eax, 0xfff
+    cmp     eax, GHCB_PAGE_STATE_CHANGE_RESPONSE
+    jnz     TerminateSevGuestLaunch
+    cmp     edx, 0
+    jnz     TerminateSevGuestLaunch
+
+InvalidateGHCBPageDone:
+    OneTimeCallRet InvalidateGHCBPage
+
 ; Check if Secure Encrypted Virtualization (SEV) features are enabled.
 ;
 ; Register usage is tight in this routine, so multiple calls for the
@@ -450,6 +519,19 @@ clearGhcbMemoryLoop:
     ;
     OneTimeCall   CheckHypervisorFeatures
 
+    ;
+    ; The page table built above cleared the memory encryption mask from the
+    ; GHCB_BASE (aka made it shared). When SEV-SNP is enabled, to maintain
+    ; the security guarantees, the page state transition from private to
+    ; shared must go through the page invalidation steps. Invalidate the
+    ; memory range before loading the page table below.
+    ;
+    ; NOTE: the invalidation must happen after zeroing the GHCB memory. This
+    ;       is because, in the 32-bit mode all the access are considered private.
+    ;       The invalidation before the zero'ing will cause a #VC.
+    ;
+    OneTimeCall  InvalidateGHCBPage
+
 SetCr3:
     ;
     ; Set CR3 now that the paging structures are available
-- 
2.17.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74644): https://edk2.groups.io/g/devel/message/74644
Mute This Topic: https://groups.io/mt/82479067/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH RFC v2 17/28] OvmfPkg/ResetVector: Invalidate the GHCB page

Posted by Erdem Aktas via groups.io 1 month, 2 weeks ago
Hi Brijesh,
I have few naive questions inlined:

On Fri, Apr 30, 2021 at 4:52 AM Brijesh Singh <brijesh.singh@amd.com> wrote:
> +    ; Use PVALIDATE instruction to invalidate the page
> +    mov     eax, GHCB_BASE
> +    mov     ecx, 0
> +    mov     edx, 0
> +    DB      0xF2, 0x0F, 0x01, 0xFF
> +    cmp     eax, 0
> +    jnz     TerminateSevGuestLaunch
Any reason why the PVALIDATE return value (EFLAGS.CF) is not checked
here? IMO, this might lead some page replay attacks.

>
> +    ;
> +    ; The page table built above cleared the memory encryption mask from the
> +    ; GHCB_BASE (aka made it shared). When SEV-SNP is enabled, to maintain
> +    ; the security guarantees, the page state transition from private to
> +    ; shared must go through the page invalidation steps. Invalidate the
> +    ; memory range before loading the page table below.
> +    ;
> +    ; NOTE: the invalidation must happen after zeroing the GHCB memory. This
> +    ;       is because, in the 32-bit mode all the access are considered private.
> +    ;       The invalidation before the zero'ing will cause a #VC.
> +    ;
> +    OneTimeCall  InvalidateGHCBPage
I am not sure if this is a great idea.
1. Zeroing page content before paging is enabled. We are actually
writing 0s encrypted with a guest key.
2. invalidating the page and making it shared.
Doesn't this reveal a mapping of what 0's look like when a specific
page is encrypted?  And when the page is marked as shared, from the
guest and host perspective, it is not zeroed but filled with some data
that looks random. So what is the purpose of zeroing the page before
invalidation?

Thanks
-Erdem


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74702): https://edk2.groups.io/g/devel/message/74702
Mute This Topic: https://groups.io/mt/82479067/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH RFC v2 17/28] OvmfPkg/ResetVector: Invalidate the GHCB page

Posted by Brijesh Singh 1 month, 2 weeks ago
On 5/3/21 8:05 AM, Erdem Aktas wrote:
> Hi Brijesh,
> I have few naive questions inlined:
>
> On Fri, Apr 30, 2021 at 4:52 AM Brijesh Singh <brijesh.singh@amd.com> wrote:
>> +    ; Use PVALIDATE instruction to invalidate the page
>> +    mov     eax, GHCB_BASE
>> +    mov     ecx, 0
>> +    mov     edx, 0
>> +    DB      0xF2, 0x0F, 0x01, 0xFF
>> +    cmp     eax, 0
>> +    jnz     TerminateSevGuestLaunch
> Any reason why the PVALIDATE return value (EFLAGS.CF) is not checked
> here? IMO, this might lead some page replay attacks.

Ah, good catch. I will add this in next rev.


>
>> +    ;
>> +    ; The page table built above cleared the memory encryption mask from the
>> +    ; GHCB_BASE (aka made it shared). When SEV-SNP is enabled, to maintain
>> +    ; the security guarantees, the page state transition from private to
>> +    ; shared must go through the page invalidation steps. Invalidate the
>> +    ; memory range before loading the page table below.
>> +    ;
>> +    ; NOTE: the invalidation must happen after zeroing the GHCB memory. This
>> +    ;       is because, in the 32-bit mode all the access are considered private.
>> +    ;       The invalidation before the zero'ing will cause a #VC.
>> +    ;
>> +    OneTimeCall  InvalidateGHCBPage
> I am not sure if this is a great idea.
> 1. Zeroing page content before paging is enabled. We are actually
> writing 0s encrypted with a guest key.
> 2. invalidating the page and making it shared.
> Doesn't this reveal a mapping of what 0's look like when a specific
> page is encrypted?  And when the page is marked as shared, from the
> guest and host perspective, it is not zeroed but filled with some data
> that looks random. So what is the purpose of zeroing the page before
> invalidation?

I don't know why zeroing of the GHCB page is done here. The code to
zero'ing was added in ES. I will check with Tom to get a bit of histroy.
The GHCB is not established until we reach to Sec/SecMain.c, so,
theoretically we can remove the zero'ing and push it to Sec/SecMain.c.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74708): https://edk2.groups.io/g/devel/message/74708
Mute This Topic: https://groups.io/mt/82479067/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-