Re: [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled

[Laszlo Ersek <lersek@redhat.com>]

On 06/28/18 14:57, Laszlo Ersek wrote:
> On 06/27/18 19:49, Brijesh Singh wrote:
>>
>>
>> On 06/27/2018 11:59 AM, Laszlo Ersek wrote:
>>> On 06/27/18 18:34, Brijesh Singh wrote:
>>>> On 06/27/2018 07:54 AM, Laszlo Ersek wrote:
>>>>> On 06/26/18 21:46, Brijesh Singh wrote:
>>>
>>>>>> After that, any access
>>>>>> to the flash will end up going through the encryption engine. I did
>>>>>> try
>>>>>> hacking EDK2 to restore the C-bit
>>>>>
>>>>> (I continue to be annoyed that the memory encryption bit is not exposed
>>>>> in the GCD memory space attributes explicitly.)
>>>>>
>>>>>> but that was not sufficient because UEFI
>>>>>> runtime services are mapped as "encrypted" in OS page table
>>>>>
>>>>> What do you mean here? Runtime services *code* or runtime services
>>>>> *data*? Code must obviously be remain encrypted (otherwise we cannot
>>>>> execute it in SEV). Runtime Services Data should also be mapped as
>>>>> encrypted (it is normal RAM that is not used for guest<->hypervisor
>>>>> exchange).
>>>>
>>>> Sorry, I was meaning to say both the "code" and "data" are mapped as
>>>> encrypted by the OS.
>>>>
>>>>>> hence we end up accessing the flash as encrypted when OS requests to
>>>>>> update the variables.
>>>>>
>>>>> I don't understand the "hence" here; I don't see how the implication
>>>>> follows. runtime services code and data should be encrypted. Runtime
>>>>> MMIO should be un-encrypted.
>>>>>
>>>>> Ohh, wait, in MarkMemoryRangeForRuntimeAccess(), we use
>>>>> "EfiGcdMemoryTypeSystemMemory". I don't have a clue why that is a good
>>>>> idea. That should have been EfiGcdMemoryTypeMemoryMappedIo.
>>>>
>>>> Right, the memory is marked as 'system ram' and not 'mmio'.
>>>> Just to experiment, I did try changing it to 'mmio' to see if OS will
>>>> map this  region as "unencrypted" but ovmf fails with below error
>>>> message after changing it from systemRAM->mmio
>>>>
>>>> ConvertPages: failed to find range FFC00000 - FFFFFFFF
>>>> ASSERT_EFI_ERROR (Status = Not Found)
>>>> ASSERT [FvbServicesRuntimeDxe]
>>>> /home/amd/workdir/upstream/edk2/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServie.c(864):
>>>>
>>>> !EFI_ERROR (Status)
>>>
>>> This error occurs because (I think) you modified only the AddMemorySpace
>>> call. If you change the GCD type on that, then please update the
>>> subsequent AllocatePages as well, from EfiRuntimeServicesData to
>>> EfiMemoryMappedIO.
>>>
>>
>> Here is what I have.
>>
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
>> @@ -847,7 +847,7 @@ MarkMemoryRangeForRuntimeAccess (
>>                    );
>>
>>    Status = gDS->AddMemorySpace (
>> -                  EfiGcdMemoryTypeSystemMemory,
>> +                  EfiGcdMemoryTypeMemoryMappedIo,
>>                    BaseAddress,
>>                    Length,
>>                    EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
>> @@ -856,7 +856,7 @@ MarkMemoryRangeForRuntimeAccess (
>>
>>    Status = gBS->AllocatePages (
>>                    AllocateAddress,
>> -                  EfiRuntimeServicesData,
>> +                  EfiMemoryMappedIO,
>>                    EFI_SIZE_TO_PAGES (Length),
>>                    &BaseAddress
>>                    );
>>
>> I am still getting the error assertion failure. I can debug to see what
>> is going on.
> 
> Hmmm. Indeed, memory space added to GCD need not immediately show up in
> the UEFI memory map, for the UEFI memory allocation services to allocate
> from. IIRC, the PI spec says that *system memory* added like this *may*
> show up immediately:
> 
> """
> If the memory range specified by BaseAddress and Length is of type
> EfiGcdMemoryTypeSystemMemory or EfiGcdMemoryTypeMoreReliable, then the
> memory range may be automatically allocated for use by the UEFI memory
> services.
> """
> 
> and, indeed in edk2, it happens at once; but it doesn't apply to MMIO at
> all.
> 
> So, can you replace the AllocatePages call with the following:
> 
>   Status = gDS->AllocateMemorySpace (
>                   EfiGcdAllocateAddress,
>                   EfiGcdMemoryTypeMemoryMappedIo,
>                   0,                              // Alignment
>                   EFI_SIZE_TO_PAGES (Length),
>                   &BaseAddress,
>                   gImageHandle,
>                   NULL                            // DeviceHandle
>                   );
> 
>>> The spec says about the latter enum constant, "Used by system firmware
>>> to request that a memory-mapped IO region be mapped by the OS to a
>>> virtual address so it can be accessed by EFI runtime services." It seems
>>> appropriate (and I'm a bit confused why we haven't used the MMIO GCD and
>>> UEFI enum values for the memory type, all this time.)
>>>
>>>> Since this efi runtime data is mapped as C=1 by the OS, hence when OS
>>>> asks efi to update the runtime variable we end up accessing the memory
>>>> region with C=1 (runtime services are executed using OS pagetable).
>>>
>>> Indeed.
>>>
>>> (And, this is only a problem when SMM is not used, i.e. when the full
>>> variable driver stack is non-SMM, just DXE. In the SMM case, the SMM
>>> page tables are used, and the OS cannot interfere with that.)
>>>
>>
>> Good point, I will try it and let you know. As you say since SMM uses
>> UEFI page table
> 
> More correctly: SMM drivers use, at runtime as well, the page table in
> SMRAM that was created by the firmware.
> 
>> hence after fixing FtwNotificationEvent(..) we should be
>> good.
> 
> No, that's not precise -- FtwNotificationEvent() is not used in SMM *at
> all*.

Sigh, I misunderstood you. You meant, "after fixing DXE, we should be
good, because the the issue only affects DXE".

I mistook your statement as "after we fix DXE, both DXE and SMM will be
OK, because the fix affects both DXE and SMM". That was not a correct
statement (because the fix only affects DXE; SMM is unaffected and needs
no fix), which is why I attempted to correct it. Of course, you never
*said* that statement. :)

Sorry about the noise!
Laszlo