On Wed, 9 Sep 2020 at 16:49, Tom Lendacky <thomas.lenda...@amd.com> wrote: > > On 9/9/20 7:44 AM, Laszlo Ersek wrote: > > On 09/09/20 10:27, Ard Biesheuvel wrote: > >> (adding Laszlo and Brijesh) > >> > >> On Tue, 8 Sep 2020 at 20:46, Borislav Petkov <b...@alien8.de> wrote: > >>> > >>> + Ard so that he can ack the efi bits. > >>> > >>> On Mon, Sep 07, 2020 at 03:16:12PM +0200, Joerg Roedel wrote: > >>>> From: Tom Lendacky <thomas.lenda...@amd.com> > >>>> > >>>> Calling down to EFI runtime services can result in the firmware > >>>> performing VMGEXIT calls. The firmware is likely to use the GHCB of > >>>> the OS (e.g., for setting EFI variables), > > > > I've had to stare at this for a while. > > > > Because, normally a VMGEXIT is supposed to occur like this: > > > > - guest does something privileged > > - resultant non-automatic exit (NAE) injects a #VC exception > > - exception handler figures out what that privileged thing was > > - exception handler submits request to hypervisor via GHCB contents plus > > VMGEXIT instruction > > > > Point being, the agent that "owns" the exception handler is supposed to > > pre-allocate or otherwise provide the GHCB too, for information passing. > > > > So... what is the particular NAE that occurs during the execution of > > UEFI runtime services (at OS runtime)? > > > > And assuming it occurs, I'm unsure if the exception handler (IDT) at > > that point is owned (temporarily?) by the firmware. > > > > - If the #VC handler comes from the firmware, then I don't know why it > > would use the OS's GHCB. > > > > - If the #VC handler comes from the OS, then I don't understand why the > > commit message says "firmware performing VMGEXIT", given that in this > > case it would be the OS's #VC handler executing VMGEXIT. > > > > So, I think the above commit message implies a VMGEXIT *without* a NAE / > > #VC context. (Because, I fail to interpret the commit message in a NAE / > > #VC context in any way; see above.) > > Correct. > > > > > OK, so let's see where the firmware performs a VMGEXIT *outside* of an > > exception handler, *while* at OS runtime. There seems to be one, in file > > "OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c": > > Again, correct. Basically this is what is invoked when setting UEFI variables. > > > > >> VOID > >> QemuFlashPtrWrite ( > >> IN volatile UINT8 *Ptr, > >> IN UINT8 Value > >> ) > >> { > >> if (MemEncryptSevEsIsEnabled ()) { > >> MSR_SEV_ES_GHCB_REGISTER Msr; > >> GHCB *Ghcb; > >> > >> Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB); > >> Ghcb = Msr.Ghcb; > >> > >> // > >> // Writing to flash is emulated by the hypervisor through the use of > >> write > >> // protection. This won't work for an SEV-ES guest because the write > >> won't > >> // be recognized as a true MMIO write, which would result in the > >> required > >> // #VC exception. Instead, use the the VMGEXIT MMIO write support > >> directly > >> // to perform the update. > >> // > >> VmgInit (Ghcb); > >> Ghcb->SharedBuffer[0] = Value; > >> Ghcb->SaveArea.SwScratch = (UINT64) (UINTN) Ghcb->SharedBuffer; > >> VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, (UINT64) (UINTN) Ptr, 1); > >> VmgDone (Ghcb); > >> } else { > >> *Ptr = Value; > >> } > >> } > > > > This function *does* run at OS runtime (as a part of non-volatile UEFI > > variable writes). > > > > And note that, wherever MSR_SEV_ES_GHCB points to at the moment, is used > > as GHCB. > > > > If the guest kernel allocates its own GHCB and writes the allocation > > address to MSR_SEV_ES_GHCB, then indeed the firmware will use the GHCB > > of the OS. > > > > I reviewed edk2 commit 437eb3f7a8db > > ("OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with > > SEV-ES", 2020-08-17), but I admit I never thought of the guest OS > > changing MSR_SEV_ES_GHCB. I'm sorry about that. > > > > As long as this driver is running before OS runtime (i.e., during the > > DXE and BDS phases), MSR_SEV_ES_GHCB is supposed to carry the value we > > set in "OvmfPkg/PlatformPei/AmdSev.c": > > > >> STATIC > >> VOID > >> AmdSevEsInitialize ( > >> VOID > >> ) > >> { > >> VOID *GhcbBase; > >> PHYSICAL_ADDRESS GhcbBasePa; > >> UINTN GhcbPageCount, PageCount; > >> RETURN_STATUS PcdStatus, DecryptStatus; > >> IA32_DESCRIPTOR Gdtr; > >> VOID *Gdt; > >> > >> if (!MemEncryptSevEsIsEnabled ()) { > >> return; > >> } > >> > >> PcdStatus = PcdSetBoolS (PcdSevEsIsEnabled, TRUE); > >> ASSERT_RETURN_ERROR (PcdStatus); > >> > >> // > >> // Allocate GHCB and per-CPU variable pages. > >> // Since the pages must survive across the UEFI to OS transition > >> // make them reserved. > >> // > >> GhcbPageCount = mMaxCpuCount * 2; > >> GhcbBase = AllocateReservedPages (GhcbPageCount); > >> ASSERT (GhcbBase != NULL); > >> > >> GhcbBasePa = (PHYSICAL_ADDRESS)(UINTN) GhcbBase; > >> > >> // > >> // Each vCPU gets two consecutive pages, the first is the GHCB and the > >> // second is the per-CPU variable page. Loop through the allocation and > >> // only clear the encryption mask for the GHCB pages. > >> // > >> for (PageCount = 0; PageCount < GhcbPageCount; PageCount += 2) { > >> DecryptStatus = MemEncryptSevClearPageEncMask ( > >> 0, > >> GhcbBasePa + EFI_PAGES_TO_SIZE (PageCount), > >> 1, > >> TRUE > >> ); > >> ASSERT_RETURN_ERROR (DecryptStatus); > >> } > >> > >> ZeroMem (GhcbBase, EFI_PAGES_TO_SIZE (GhcbPageCount)); > >> > >> PcdStatus = PcdSet64S (PcdGhcbBase, GhcbBasePa); > >> ASSERT_RETURN_ERROR (PcdStatus); > >> PcdStatus = PcdSet64S (PcdGhcbSize, EFI_PAGES_TO_SIZE (GhcbPageCount)); > >> ASSERT_RETURN_ERROR (PcdStatus); > >> > >> DEBUG ((DEBUG_INFO, > >> "SEV-ES is enabled, %lu GHCB pages allocated starting at 0x%p\n", > >> (UINT64)GhcbPageCount, GhcbBase)); > >> > >> AsmWriteMsr64 (MSR_SEV_ES_GHCB, GhcbBasePa); > > > > So what is the *actual* problem at OS runtime: > > > > - Is it that MSR_SEV_ES_GHCB still points at this PEI-phase *reserved* > > memory allocation (and so when QemuFlashPtrWrite() tries to access it > > during OS runtime, it doesn't have a runtime mapping for it)? > > At this point the GHCB MSR points to the OS GHCB, which isn't mapped by > the page tables supplied by the OS and used by UEFI. > > > > > - Or is it that the OS actively changes MSR_SEV_ES_GHCB, pointing to a > > memory area that the OS owns -- and *that* area is what > > QemuFlashPtrWrite() cannot access at OS runtime? > > Correct. > > > > > The first problem statement does *not* seem to apply, given -- again -- > > that the commit message says, "firmware is likely to use the GHCB of the > > OS". > > > > So I think the second problem statement must apply. > > > > (I think the "reserved allocation" above is "reserved" only because we > > want to keep the OS out of it around the ExitBootServices() transition.) > > > > Back to the email: > > > > On 09/09/20 10:27, Ard Biesheuvel wrote: > >> On Tue, 8 Sep 2020 at 20:46, Borislav Petkov <b...@alien8.de> wrote: > >>> On Mon, Sep 07, 2020 at 03:16:12PM +0200, Joerg Roedel wrote: > >>>> so each GHCB in the system needs to be identity > >>>> mapped in the EFI page tables, as unencrypted, to avoid page faults. > > > > Not sure I agree about this, but at least it seems to confirm my > > understanding -- apparently the idea is, for the OS, to satisfy > > QemuFlashPtrWrite() in the firmware, by putting the "expected" mapping > > -- for wherever MSR_SEV_ES_GHCB is going to point to -- in place. > > > >>>> > >>>> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com> > >>>> [ jroe...@suse.de: Moved GHCB mapping loop to sev-es.c ] > >>>> Signed-off-by: Joerg Roedel <jroe...@suse.de> > >> > >> > >> This looks like it is papering over a more fundamental issue: any > >> memory region that the firmware itself needs to access during the > >> execution of runtime services needs to be described in the UEFI memory > >> map, with the appropriate annotations so that the OS knows it should > >> include these in the EFI runtime page tables. So why has this been > >> omitted in this case? > > > > So yeah, the issue seems to be that the QemuFlashFvbServicesRuntimeDxe > > driver does not *own* the GHCB that it attempts to use at OS runtime. It > > doesn't know where MSR_SEV_ES_GHCB is going to point. > > > > Is QemuFlashFvbServicesRuntimeDxe permitted to change MSR_SEV_ES_GHCB > > *temporarily* at OS runtime? > > > > Because, in that case: > > > > - QemuFlashFvbServicesRuntimeDxe should allocate a Runtime Services Data > > block for GHCB when it starts up (if SEV-ES is active), > > > > - QemuFlashFvbServicesRuntimeDxe should register a SetVirtualAddressMap > > handler, and use EfiConvertPointer() from UefiRuntimeLib to convert > > the "runtime GHCB" address to virtual address, in that handler, > > > > - QemuFlashPtrWrite() should call EfiAtRuntime() from UefiRuntimeLib, > > and if the latter returns TRUE, then (a) use the runtime-converted > > address for populating the GHCB, and (b) temporarily swap > > MSR_SEV_ES_GHCB with the address of the self-allocated GHCB. (The MSR > > needs a *physical* address, so QemuFlashFvbServicesRuntimeDxe would > > have to remember / retain the original (physical) allocation address > > too.) > > > > If QemuFlashFvbServicesRuntimeDxe is not permitted to change > > MSR_SEV_ES_GHCB even temporarily (at OS runtime), then I think the > > approach proposed in this (guest kernel) patch is valid. > > > > Let me skim the code below... > > > >> > >> > >> > >>>> --- > >>>> arch/x86/boot/compressed/sev-es.c | 1 + > >>>> arch/x86/include/asm/sev-es.h | 2 ++ > >>>> arch/x86/kernel/sev-es.c | 30 ++++++++++++++++++++++++++++++ > >>>> arch/x86/platform/efi/efi_64.c | 10 ++++++++++ > >>>> 4 files changed, 43 insertions(+) > >>>> > >>>> diff --git a/arch/x86/boot/compressed/sev-es.c > >>>> b/arch/x86/boot/compressed/sev-es.c > >>>> index 45702b866c33..0a9a248ca33d 100644 > >>>> --- a/arch/x86/boot/compressed/sev-es.c > >>>> +++ b/arch/x86/boot/compressed/sev-es.c > >>>> @@ -12,6 +12,7 @@ > >>>> */ > >>>> #include "misc.h" > >>>> > >>>> +#include <asm/pgtable_types.h> > >>>> #include <asm/sev-es.h> > >>>> #include <asm/trapnr.h> > >>>> #include <asm/trap_pf.h> > >>>> diff --git a/arch/x86/include/asm/sev-es.h > >>>> b/arch/x86/include/asm/sev-es.h > >>>> index e919f09ae33c..cf1d957c7091 100644 > >>>> --- a/arch/x86/include/asm/sev-es.h > >>>> +++ b/arch/x86/include/asm/sev-es.h > >>>> @@ -102,11 +102,13 @@ static __always_inline void > >>>> sev_es_nmi_complete(void) > >>>> if (static_branch_unlikely(&sev_es_enable_key)) > >>>> __sev_es_nmi_complete(); > >>>> } > >>>> +extern int __init sev_es_efi_map_ghcbs(pgd_t *pgd); > >>>> #else > >>>> static inline void sev_es_ist_enter(struct pt_regs *regs) { } > >>>> static inline void sev_es_ist_exit(void) { } > >>>> static inline int sev_es_setup_ap_jump_table(struct real_mode_header > >>>> *rmh) { return 0; } > >>>> static inline void sev_es_nmi_complete(void) { } > >>>> +static inline int sev_es_efi_map_ghcbs(pgd_t *pgd) { return 0; } > >>>> #endif > >>>> > >>>> #endif > >>>> diff --git a/arch/x86/kernel/sev-es.c b/arch/x86/kernel/sev-es.c > >>>> index 9ab3a4dfecd8..4e2b7e4d9b87 100644 > >>>> --- a/arch/x86/kernel/sev-es.c > >>>> +++ b/arch/x86/kernel/sev-es.c > >>>> @@ -491,6 +491,36 @@ int sev_es_setup_ap_jump_table(struct > >>>> real_mode_header *rmh) > >>>> return 0; > >>>> } > >>>> > >>>> +/* > >>>> + * This is needed by the OVMF UEFI firmware which will use whatever it > >>>> finds in > >>>> + * the GHCB MSR as its GHCB to talk to the hypervisor. So make sure the > >>>> per-cpu > >>>> + * runtime GHCBs used by the kernel are also mapped in the EFI > >>>> page-table. > > > > Yup, this pretty much confirms my suspicion that QemuFlashPtrWrite() is > > at the center of this. > > > > (BTW, I don't think that the runtime services data allocation, in > > QemuFlashFvbServicesRuntimeDxe, for OS runtime GHCB purposes, would have > > to be "per CPU". Refer to "Table 35. Rules for Reentry Into Runtime > > Services" in the UEFI spec -- if one processor is executing > > SetVariable(), then no other processor must enter SetVariable(). And so > > we'll have *at most* one VCPU in QemuFlashPtrWrite(), at any time.) > > > >>>> + */ > >>>> +int __init sev_es_efi_map_ghcbs(pgd_t *pgd) > >>>> +{ > >>>> + struct sev_es_runtime_data *data; > >>>> + unsigned long address, pflags; > >>>> + int cpu; > >>>> + u64 pfn; > >>>> + > >>>> + if (!sev_es_active()) > >>>> + return 0; > >>>> + > >>>> + pflags = _PAGE_NX | _PAGE_RW; > >>>> + > >>>> + for_each_possible_cpu(cpu) { > >>>> + data = per_cpu(runtime_data, cpu); > >>>> + > >>>> + address = __pa(&data->ghcb_page); > >>>> + pfn = address >> PAGE_SHIFT; > >>>> + > >>>> + if (kernel_map_pages_in_pgd(pgd, pfn, address, 1, pflags)) > >>>> + return 1; > >>>> + } > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> static enum es_result vc_handle_msr(struct ghcb *ghcb, struct > >>>> es_em_ctxt *ctxt) > >>>> { > >>>> struct pt_regs *regs = ctxt->regs; > >>>> diff --git a/arch/x86/platform/efi/efi_64.c > >>>> b/arch/x86/platform/efi/efi_64.c > >>>> index 6af4da1149ba..8f5759df7776 100644 > >>>> --- a/arch/x86/platform/efi/efi_64.c > >>>> +++ b/arch/x86/platform/efi/efi_64.c > >>>> @@ -47,6 +47,7 @@ > >>>> #include <asm/realmode.h> > >>>> #include <asm/time.h> > >>>> #include <asm/pgalloc.h> > >>>> +#include <asm/sev-es.h> > >>>> > >>>> /* > >>>> * We allocate runtime services regions top-down, starting from -4G, > >>>> i.e. > >>>> @@ -229,6 +230,15 @@ int __init efi_setup_page_tables(unsigned long > >>>> pa_memmap, unsigned num_pages) > >>>> return 1; > >>>> } > >>>> > >>>> + /* > >>>> + * When SEV-ES is active, the GHCB as set by the kernel will be > >>>> used > >>>> + * by firmware. Create a 1:1 unencrypted mapping for each GHCB. > >>>> + */ > >>>> + if (sev_es_efi_map_ghcbs(pgd)) { > >>>> + pr_err("Failed to create 1:1 mapping for the GHCBs!\n"); > >>>> + return 1; > >>>> + } > >>>> + > >>>> /* > >>>> * When making calls to the firmware everything needs to be 1:1 > >>>> * mapped and addressable with 32-bit pointers. Map the kernel > > > > Good point! > > > > And it even makes me wonder if the QemuFlashFvbServicesRuntimeDxe > > approach, with the runtime services data type memory allocation, is > > feasible at all. Namely, a page's encryption status, under SEV, is > > controlled through the PTE. > > > > And for this particular UEFI runtime area, it would *not* suffice for > > the OS to just virt-map it. The OS would also have to *decrypt* the area > > (mark the PTE as "plaintext"). > > > > In other words, it would be an "unprecedented" PTE for the OS to set up: > > the PTE would not only map the GVA to GPA, but also mark the area as > > "plaintext". > > > > Otherwise -- if the OS covers *just* the virt-mapping --, > > QemuFlashFvbServicesRuntimeDxe would populate its own "runtime GHCB" > > area just fine, but the actual data hitting the host RAM would be > > encrypted. And so the hypervisor could not interpret the GHCB. > > > > *If* QemuFlashFvbServicesRuntimeDxe should not change the kernel-owned > > PTE at runtime, even temporarily, for marking the GHCB as "plaintext", > > then the problem is indeed only solvable in the guest kernel, in my > > opinion. > > > > There simply isn't an "architected annotation" for telling the kernel, > > "virt-map this runtime services data type memory range, *and* mark it as > > plaintext at the same time". > > > > This would be necessary, as both actions affect the exact same PTE, and > > the firmware is not really allowed to touch the PTE at runtime. But we > > don't have such a hint. > > > > > > To summarize: for QemuFlashFvbServicesRuntimeDxe to allocate UEFI > > Runtime Services Data type memory, for its own runtime GHCB, two > > permissions are necessary (together), at OS runtime: > > > > - QemuFlashFvbServicesRuntimeDxe must be allowed to swap MSR_SEV_ES_GHCB > > temporarily (before executing VMGEXIT), > > > > - QemuFlashFvbServicesRuntimeDxe must be allowed to change the OS-owned > > PTE temporarily (for remapping the GHCB as plaintext, before writing > > to it). > > > > Amazing summarization Laszlo! >
Agreed. And thanks a lot for taking the time to do the analysis. Based on the above, Acked-by: Ard Biesheuvel <a...@kernel.org> Thanks,