Re: [PATCH 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State

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

Hi Laszlo,

Apologies for late response. I needed to do some investigation on SMM
before responding to you.


On 2/22/18 5:20 AM, Laszlo Ersek wrote:
> Hi Brijesh!
>
> (adding Paolo and Mike; more comments below)
>
> On 02/21/18 17:52, Brijesh Singh wrote:
>> When OVMF is built with SMM, SMMSaved State area (SMM_DEFAULT_SMBASE +
>> SMRAM_SAVE_STATE_MAP_OFFSET) contains data which need to be accessed by
>> both guest and hypervisor. Since the data need to be accessed by both
>> hence we must map the SMMSaved State area as unencrypted (i.e C-bit
>> cleared).
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>  OvmfPkg/AmdSevDxe/AmdSevDxe.inf |  4 ++++
>>  OvmfPkg/AmdSevDxe/AmdSevDxe.c   | 19 +++++++++++++++++++
>>  2 files changed, 23 insertions(+)
>>
>> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>> index 41635a57a454..162ed98a2fbe 100644
>> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>> @@ -29,6 +29,7 @@ [Packages]
>>    MdePkg/MdePkg.dec
>>    MdeModulePkg/MdeModulePkg.dec
>>    OvmfPkg/OvmfPkg.dec
>> +  UefiCpuPkg/UefiCpuPkg.dec
>>  
>>  [LibraryClasses]
>>    BaseLib
>> @@ -41,3 +42,6 @@ [LibraryClasses]
>>  
>>  [Depex]
>>    TRUE
>> +
>> +[FeaturePcd]
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
>> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
>> index e472096320ea..5ec727456526 100644
>> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
>> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
>> @@ -25,6 +25,8 @@
>>  #include <Library/UefiBootServicesTableLib.h>
>>  #include <Library/DxeServicesTableLib.h>
>>  #include <Library/MemEncryptSevLib.h>
>> +#include <Register/SmramSaveStateMap.h>
>> +#include <Register/QemuSmramSaveStateMap.h>
>>  
>>  EFI_STATUS
>>  EFIAPI
>> @@ -71,5 +73,22 @@ AmdSevDxeEntryPoint (
>>      FreePool (AllDescMap);
>>    }
>>  
>> +  //
>> +  // When SMM is enabled, clear the C-bit from SMM Saved State Area
>> +  //
>> +  if (FeaturePcdGet (PcdSmmSmramRequire)) {
>> +    EFI_PHYSICAL_ADDRESS  SmmSavedStateAreaAddress;
>> +
>> +    SmmSavedStateAreaAddress = SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET;
>> +
>> +    Status = MemEncryptSevClearPageEncMask (
>> +               0,
>> +               SmmSavedStateAreaAddress,
>> +               EFI_SIZE_TO_PAGES (sizeof(QEMU_SMRAM_SAVE_STATE_MAP)),
>> +               FALSE
>> +               );
>> +    ASSERT_EFI_ERROR (Status);
>> +  }
>> +
>>    return EFI_SUCCESS;
>>  }
>>
> First, this SMBASE address is only correct before SMBASE relocation.
>
> - What guarantees that AmdSevDxe is dispatched, and the new code is
>   executed, before PiSmmCpuDxeSmm performs the SMBASE relocation?
>
> - Where/when do we clear encryption for the state map that is used after
>   SMBASE relocation? If we don't do that at all, then why do things
>   work? (I see a whole lot of "mAddressEncMask" in PiSmmCpuDxeSmm; some
>   help would be appreciated.)

mAddressEncMask in PiSmmCpuDxeSmm basically ensure that newly created
pagetables have the C-bit set for all the pages -- its very similar to
what we do when creating non SMM page table. By default we consider all
the pages encrypted, if we do not apply the mAddressEncMask to newly
created page then we will fail to execute the code because SEV hw needs
code to be encrypted.

Currently we do not clear the C-bit of relocated SMBASE saved state
area. After you asked this question, I did a bit investigation to see
how I am booting the SMM enabled SEV guest without clearing the C-bit of
relocated SMBASE saved area. The SMBASE is mapped with C=1 in guest,
before entering in SMM mode HV saves some values in SMMSaved area --
this is done using HV key, guest BIOS never reads or writes to SMMSaved
area hence we are getting lucky. If guest BIOS ever writes or read the
value from relocated SMMSaved area then it will get garbage.

We should consider clearing the relocated SMMSavedArea but there are two
questions:

1) Where/When we should clear the C-bit of relocated SMMSavedArea ? I
was looking at multiple function provided by SmmFeatureLib but could not
find a right place for it. Most of time the SMMfeatureLib functions are
called in non-SMM context and we do not have access to SMM pagetable to
clear the C-bit of SMMSavedArea. How about if we introduce a new
function in SmmfeatureLib which gets called just after core creates a
new SMM page table ?

2) The C-bit works on page-aligned boundary but SmmSaveArea offset does
not start with page-aligned boundary. We could still go ahead and clear
the full page -- we may leak some information to HV in that case. But
the real issue is looking carefully I see that some portion of page may
contain code and hence clearing the full page will cause a runtime
issues whenever that code gets executed. (I hacked EDKII to do this and
could see that if full page is cleared then sometime later we get crash
because someone was trying to execute a code which was mapped with C=0).

One option which I was thinking is: since access to relocated
SMMSavedState is controlled via SmmFeatureLib hence we could simply map
the page with C=0 as we enter in the function  and restore the C-bit on
exit. This will ensure that any access happening to SmmSavedArea is done
with C=0. I have also tried this and it seems to work fine.  I was not
able to find any code in EDKII which trigger a code path which requires
access to SmmSavedArea hence I hacked something to test the flow. I
think this is clean approach and does not require any changes in EDKII
core and should work just fine.

> Second, after SMBASE relocation, when/where do we restore the C bit on
> the original (default) SMBASE? I think we should do that, otherwise
> we'll have an info leak there.

Good point, I will see where we can restore the C-bit of original SMBASE.

>
> Third, why is it OK to pass Flush=FALSE? The documentation says, "mostly
> TRUE except MMIO addresses". The default SMBASE points into memory, not
> MMIO; there could be normal data there. PiSmmCpuDxeSmm backs up the area
> before SMBASE relocation, and restores it after. See SmmRelocateBases()
> in "UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c":

Very good point,  the flush should be TRUE. I think its my copy/paste.

>   //
>   // Backup original contents at address 0x38000
>   //
>   CopyMem (BakBuf, U8Ptr, sizeof (BakBuf));
>   CopyMem (&BakBuf2, CpuStatePtr, sizeof (BakBuf2));
>
> I wonder if we should introduce new SmmCpuFeaturesLib APIs, for managing
> memory encryption settings around SMBASE relocation.

See my above response, If you think its acceptable approach then I don't
see us needing any changes in SmmFeatureLib.

> - PiSmmCpuDxeSemm could call these APIs in "strategic places".
> - The SmmCpuFeaturesLib instances in UefiCpuPkg and QuarkSocPkg would do
>   nothing.
> - The instace under OvmfPkg would handle the C-bit, dependent on SEV
>   presence.
>
> The lib class already has a function called
> SmmCpuFeaturesSmmRelocationComplete(); we might be able to use that (too).
>
> Thanks,
> Laszlo