linux-kernel - Re: [PATCH 00/22] KVM: x86: Fix multiple #PF RO infinite loop bugs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1e6955c8-3672-41e7-ba8a-f2a205a601d9@redhat.com>
Date: Wed, 14 Aug 2024 19:58:58 +0200
From: Paolo Bonzini <pbonzini@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
 Peter Gonda <pgonda@...gle.com>, Michael Roth <michael.roth@....com>,
 Vishal Annapurve <vannapurve@...gle.com>,
 Ackerly Tng <ackerleytng@...gle.com>
Subject: Re: [PATCH 00/22] KVM: x86: Fix multiple #PF RO infinite loop bugs

On 8/9/24 21:02, Sean Christopherson wrote:
> The folks doing TDX enabling ran into a problem where exposing a read-only
> memslot to a TDX guest put it into an infinite loop.  The most immediate
> issue is that KVM never creates MMIO SPTEs for RO memslots, because except
> for TDX (which isn't officially supported yet), such SPTEs can't distinguish
> between reads and writes, i.e. would trigger MMIO on everything and thus
> defeat the purpose of having a RX memslot.
> 
> That breaks TDX, SEV-ES, and SNP, i.e. VM types that rely on MMIO caching
> to reflect MMIO faults into the guest as #VC/#VE, as the guest never sees
> the fault, KVM refuses to emulate, the guest loops indefinitely.  That's
> patch 1.
> 
> Patches 2-4 fix an amusing number of other bugs that made it difficult to
> figure out the true root cause.
> 
> The rest is a bunch of cleanups to consolidate all of the unprotect+retry
> paths (there are four-ish).
> 
> As a bonus, adding RET_PF_WRITE_PROTECTED obviates the need for
> kvm_lookup_pfn()[*].
> 
> [*] https://lore.kernel.org/all/63c41e25-2523-4397-96b4-557394281443@redhat.com

Nice!  For now I've placed it in kvm/queue as this is clearly 6.12 
material.  It will be replaced by the v2 of course before graduating to 
kvm/next.

Thanks,

Paolo

> Sean Christopherson (22):
>    KVM: x86: Disallow read-only memslots for SEV-ES and SEV-SNP (and TDX)
>    KVM: VMX: Set PFERR_GUEST_{FINAL,PAGE}_MASK if and only if the GVA is
>      valid
>    KVM: x86/mmu: Trigger unprotect logic only on write-protection page
>      faults
>    KVM: x86/mmu: Skip emulation on page fault iff 1+ SPs were unprotected
>    KVM: x86: Retry to-be-emulated insn in "slow" unprotect path iff sp is
>      zapped
>    KVM: x86: Get RIP from vCPU state when storing it to last_retry_eip
>    KVM: x86: Store gpa as gpa_t, not unsigned long, when unprotecting for
>      retry
>    KVM: x86/mmu: Apply retry protection to "fast nTDP unprotect" path
>    KVM: x86/mmu: Try "unprotect for retry" iff there are indirect SPs
>    KVM: x86/mmu: Replace PFERR_NESTED_GUEST_PAGE with a more descriptive
>      helper
>    KVM: x86: Move EMULTYPE_ALLOW_RETRY_PF to x86_emulate_instruction()
>    KVM: x86: Fold retry_instruction() into x86_emulate_instruction()
>    KVM: x86/mmu: Don't try to unprotect an INVALID_GPA
>    KVM: x86/mmu: Always walk guest PTEs with WRITE access when
>      unprotecting
>    KVM: x86/mmu: Move event re-injection unprotect+retry into common path
>    KVM: x86: Remove manual pfn lookup when retrying #PF after failed
>      emulation
>    KVM: x86: Check EMULTYPE_WRITE_PF_TO_SP before unprotecting gfn
>    KVM: x86: Apply retry protection to "unprotect on failure" path
>    KVM: x86: Update retry protection fields when forcing retry on
>      emulation failure
>    KVM: x86: Rename
>      reexecute_instruction()=>kvm_unprotect_and_retry_on_failure()
>    KVM: x86/mmu: Subsume kvm_mmu_unprotect_page() into the and_retry()
>      version
>    KVM: x86/mmu: Detect if unprotect will do anything based on
>      invalid_list
> 
>   arch/x86/include/asm/kvm_host.h |  16 ++-
>   arch/x86/kvm/mmu/mmu.c          | 175 ++++++++++++++++++++++----------
>   arch/x86/kvm/mmu/mmu_internal.h |   3 +
>   arch/x86/kvm/mmu/mmutrace.h     |   1 +
>   arch/x86/kvm/mmu/paging_tmpl.h  |   2 +-
>   arch/x86/kvm/mmu/tdp_mmu.c      |   6 +-
>   arch/x86/kvm/vmx/vmx.c          |   5 +-
>   arch/x86/kvm/x86.c              | 133 +++++++-----------------
>   include/linux/kvm_host.h        |   7 ++
>   virt/kvm/kvm_main.c             |   5 +-
>   10 files changed, 184 insertions(+), 169 deletions(-)
> 
> 
> base-commit: 332d2c1d713e232e163386c35a3ba0c1b90df83f