Re: [edk2-devel] [PATCH v2 2/6] OvmfPkg/AmdSev: add Grub Firmware Volume Package

["Laszlo Ersek" <lersek@redhat.com>]

On 11/25/20 19:35, Laszlo Ersek wrote:
> On 11/25/20 18:09, James Bottomley wrote:
>> On Wed, 2020-11-25 at 08:02 -0800, James Bottomley wrote:
>>> On Wed, 2020-11-25 at 15:01 +0100, Laszlo Ersek wrote:
>>>> This upgrade gave me kernel 5.8.18-100.fc31.x86_64 in the guest --
>>>> and this one does *not* crash. From your boot log below, I see your
>>>> guest kernel is 5.5.0; I suggest upgrading it.
>>>
>>> Heh, that's easier said than done ... I always make my encrypted
>>> images too small to upgrade a kernel easily.  Anyway, after doing the
>>> remove and add stuff dance, I finally got it upgraded to the latest
>>> debian testing linux-image-5.8.0-3 it's still crashing although with
>>> a slightly different traceback.  It looks like there might be
>>> something additional in the fedora 5.8 kernel that fixes this.  I'm
>>> going to try out upstream kernels next.
>>
>> I've got the upstream kernel booting through OVMF with a qemu -kernel
>> command line.  I also have a fix: it's not to delete the dummy variable
>> which was part of the ancient x86 anti bricking code (which is also why
>> arm64 doesn't have the problem).
>>
>> If you remove the set variable in arch/x86/platform/efi/quirks.c:
>>
>> /*
>>  * Deleting the dummy variable which kicks off garbage collection
>> */
>> void efi_delete_dummy_variable(void)
>> {
>> 	efi.set_variable_nonblocking((efi_char16_t *)efi_dummy_name,
>> 				     &EFI_DUMMY_GUID,
>> 				     EFI_VARIABLE_NON_VOLATILE |
>> 				     EFI_VARIABLE_BOOTSERVICE_ACCESS |
>> 				     EFI_VARIABLE_RUNTIME_ACCESS, 0, NULL);
>> }
>>
>> The kernel will boot.  I'm not sure why we have this deletion
>> unconditionally in efi_enter_virtual_mode, but removing the call with
>> the patch below allows the kernel to boot.
> 
> I think commit 2ecb7402cfc7 ("efi/x86: Do not clean dummy variable in
> kexec path", 2019-10-07) is related (part of v5.4), but it's not
> sufficient to prevent the boot crash. (That removal only covered the
> kexec path, and not the normal boot path.)
> 
>>
>> However, once the kernel has booted, any attempt to write to an EFI
>> variable results in this:
>>
>> [  975.440240] [Firmware Bug]: Page fault caused by firmware at PA: 0x7e450020
>>
>> And then the efi runtime gets disabled.
> 
> Blech, that doesn't look good. We still get a page fault somewhere in
> the firmware, it just doesn't kill the kernel outright. That kind of
> suggests the crash on the boot path *is* firmware-originated, it's just
> that the kernel is unable to mask the problem that early.

Actually, I do think we have a bug in the firmware. I need to verify it
(and if verification confirms it, to send a patch for it). Consider the
following snippet in file
"MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c", function
RegisterVariablePolicy():

    // Reallocate and copy the table.
    NewTable = AllocatePool( NewSize );
    if (NewTable == NULL) {
      return EFI_OUT_OF_RESOURCES;
    }
    CopyMem( NewTable, mPolicyTable, mCurrentTableUsage );
    mCurrentTableSize = NewSize;
    if (mPolicyTable != NULL) {
      FreePool( mPolicyTable );
    }
    mPolicyTable = NewTable;


* In the SMM build of OVMF, this code is linked (via
"MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf") into the
following SMM driver:

  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf

For that module (well, for SMM drivers in general), the AllocatePool()
function comes from the following MemoryAllocationLib instance:

  MdePkg/Library/SmmMemoryAllocationLib/SmmMemoryAllocationLib.inf

and so the AllocatePool() call ultimately boils down to allocating *SMRAM*:

  gSmst->SmmAllocatePool()

Which is fine.


* However, in the non-SMM build, the same code snippet is linked (via
"MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.inf")
into the following driver:

  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf

In this module (well, in runtime DXE drivers in general), the
AllocatePool() call function comes from the following
MemoryAllocationLib instance:

  MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf

And that is *wrong*. Because there, AllocatePool() allocates Boot
Services Data type memory, via

  gBS->AllocatePool() *plus* "EfiBootServicesData"

Therefore, even though VariablePolicyLibVirtualAddressCallback() in file
"MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRuntimeDxe.c"
attempts to convert "mPolicyTable" to a runtime virtual address, in the
SMM-less build, that conversion fails -- the original memory that the
pointer points at, pre-conversion, is not runtime, but boot time memory.


The fix *should be* to call AllocateRuntimePool() from the
MemoryAllocationLib class. In the SMM instance, that will make no
difference; compare, in
"MdePkg/Library/SmmMemoryAllocationLib/MemoryAllocationLib.c":

VOID *
EFIAPI
AllocatePool (
  IN UINTN  AllocationSize
  )
{
  return InternalAllocatePool (EfiRuntimeServicesData, AllocationSize);
}

VOID *
EFIAPI
AllocateRuntimePool (
  IN UINTN  AllocationSize
  )
{
  return InternalAllocatePool (EfiRuntimeServicesData, AllocationSize);
}

The InternalAllocatePool() calls are identical, in the SMM instance.


Whereas in the UEFI instance, in
"MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c", it does
make a difference; compare:

VOID *
EFIAPI
AllocatePool (
  IN UINTN  AllocationSize
  )
{
  return InternalAllocatePool (EfiBootServicesData, AllocationSize);
}

VOID *
EFIAPI
AllocateRuntimePool (
  IN UINTN  AllocationSize
  )
{
  return InternalAllocatePool (EfiRuntimeServicesData, AllocationSize);
}

The "MemoryType" parameter of InternalAllocatePool(), which gets
forwarded to gBS->AllocatePool(), changes from "EfiBootServicesData" to
"EfiRuntimeServicesData".

Let me test this.

(BTW I see some more pool allocations in
"MdeModulePkg/Library/VariablePolicyHelperLib" to... I guess I'll have
to check those as well.)

Thanks
Laszlo