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

["Brijesh Singh" <brijesh.singh@amd.com>]

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.