[PATCH 0/3] KVM: x86: guest.MAXPHYADDR fix and related cleanup

[PATCH 0/3] KVM: x86: guest.MAXPHYADDR fix and related cleanup

[Sean Christopherson]

Fix a bug exposed by syzkaller where running with guest.MAXPHYADDR < 32
leads KVM injecting a garbage exception due to mishandling an illegal GPA
check when "translating" a non-nested GPA.

Clean up tangentially related code in load_pdptrs() to discourage reading
guest memory using a potentially-nested GPA.

Sean Christopherson (3):
  Revert "KVM: x86: mmu: Add guest physical address check in
    translate_gpa()"
  KVM: x86: Subsume nested GPA read helper into load_pdptrs()
  KVM: x86: Simplify retrieving the page offset when loading PDTPRs

 arch/x86/include/asm/kvm_host.h |  3 --
 arch/x86/kvm/mmu/mmu.c          |  6 ----
 arch/x86/kvm/x86.c              | 56 +++++++++++----------------------
 3 files changed, 18 insertions(+), 47 deletions(-)

-- 
2.33.0.259.gc128427fd7-goog

[PATCH 1/3] Revert "KVM: x86: mmu: Add guest physical address check in translate_gpa()"

[Sean Christopherson]

Revert a misguided illegal GPA check when "translating" a non-nested GPA.
The check is woefully incomplete as it does not fill in @exception as
expected by all callers, which leads to KVM attempting to inject a bogus
exception, potentially exposing kernel stack information in the process.

 WARNING: CPU: 0 PID: 8469 at arch/x86/kvm/x86.c:525 exception_type+0x98/0xb0 arch/x86/kvm/x86.c:525
 CPU: 1 PID: 8469 Comm: syz-executor531 Not tainted 5.14.0-rc7-syzkaller #0
 RIP: 0010:exception_type+0x98/0xb0 arch/x86/kvm/x86.c:525
 Call Trace:
  x86_emulate_instruction+0xef6/0x1460 arch/x86/kvm/x86.c:7853
  kvm_mmu_page_fault+0x2f0/0x1810 arch/x86/kvm/mmu/mmu.c:5199
  handle_ept_misconfig+0xdf/0x3e0 arch/x86/kvm/vmx/vmx.c:5336
  __vmx_handle_exit arch/x86/kvm/vmx/vmx.c:6021 [inline]
  vmx_handle_exit+0x336/0x1800 arch/x86/kvm/vmx/vmx.c:6038
  vcpu_enter_guest+0x2a1c/0x4430 arch/x86/kvm/x86.c:9712
  vcpu_run arch/x86/kvm/x86.c:9779 [inline]
  kvm_arch_vcpu_ioctl_run+0x47d/0x1b20 arch/x86/kvm/x86.c:10010
  kvm_vcpu_ioctl+0x49e/0xe50 arch/x86/kvm/../../../virt/kvm/kvm_main.c:3652

The bug has escaped notice because practically speaking the GPA check is
useless.  The GPA check in question only comes into play when KVM is
walking guest page tables (or "translating" CR3), and KVM already handles
illegal GPA checks by setting reserved bits in rsvd_bits_mask for each
PxE, or in the case of CR3 for loading PTDPTRs, manually checks for an
illegal CR3.  This particular failure doesn't hit the existing reserved
bits checks because syzbot sets guest.MAXPHYADDR=1, and IA32 architecture
simply doesn't allow for such an absurd MAXPHADDR, e.g. 32-bit paging
doesn't define any reserved PA bits checks, which KVM emulates by only
incorporating the reserved PA bits into the "high" bits, i.e. bits 63:32.

Simply remove the bogus check.  There is zero meaningful value and no
architectural justification for supporting guest.MAXPHYADDR < 32, and
properly filling the exception would introduce non-trivial complexity.

This reverts commit ec7771ab471ba6a945350353617e2e3385d0e013.

Fixes: ec7771ab471b ("KVM: x86: mmu: Add guest physical address check in translate_gpa()")
Cc: stable@vger.kernel.org
Reported-by: syzbot+200c08e88ae818f849ce@syzkaller.appspotmail.com
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/mmu/mmu.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 4853c033e6ce..4b7908187d05 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -334,12 +334,6 @@ static bool check_mmio_spte(struct kvm_vcpu *vcpu, u64 spte)
 static gpa_t translate_gpa(struct kvm_vcpu *vcpu, gpa_t gpa, u32 access,
                                   struct x86_exception *exception)
 {
-	/* Check if guest physical address doesn't exceed guest maximum */
-	if (kvm_vcpu_is_illegal_gpa(vcpu, gpa)) {
-		exception->error_code |= PFERR_RSVD_MASK;
-		return UNMAPPED_GVA;
-	}
-
         return gpa;
 }
 
-- 
2.33.0.259.gc128427fd7-goog

Re: [PATCH 1/3] Revert "KVM: x86: mmu: Add guest physical address check in translate_gpa()"

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> Revert a misguided illegal GPA check when "translating" a non-nested GPA.
> The check is woefully incomplete as it does not fill in @exception as
> expected by all callers, which leads to KVM attempting to inject a bogus
> exception, potentially exposing kernel stack information in the process.
>
>  WARNING: CPU: 0 PID: 8469 at arch/x86/kvm/x86.c:525 exception_type+0x98/0xb0 arch/x86/kvm/x86.c:525
>  CPU: 1 PID: 8469 Comm: syz-executor531 Not tainted 5.14.0-rc7-syzkaller #0
>  RIP: 0010:exception_type+0x98/0xb0 arch/x86/kvm/x86.c:525
>  Call Trace:
>   x86_emulate_instruction+0xef6/0x1460 arch/x86/kvm/x86.c:7853
>   kvm_mmu_page_fault+0x2f0/0x1810 arch/x86/kvm/mmu/mmu.c:5199
>   handle_ept_misconfig+0xdf/0x3e0 arch/x86/kvm/vmx/vmx.c:5336
>   __vmx_handle_exit arch/x86/kvm/vmx/vmx.c:6021 [inline]
>   vmx_handle_exit+0x336/0x1800 arch/x86/kvm/vmx/vmx.c:6038
>   vcpu_enter_guest+0x2a1c/0x4430 arch/x86/kvm/x86.c:9712
>   vcpu_run arch/x86/kvm/x86.c:9779 [inline]
>   kvm_arch_vcpu_ioctl_run+0x47d/0x1b20 arch/x86/kvm/x86.c:10010
>   kvm_vcpu_ioctl+0x49e/0xe50 arch/x86/kvm/../../../virt/kvm/kvm_main.c:3652
>
> The bug has escaped notice because practically speaking the GPA check is
> useless.  The GPA check in question only comes into play when KVM is
> walking guest page tables (or "translating" CR3), and KVM already handles
> illegal GPA checks by setting reserved bits in rsvd_bits_mask for each
> PxE, or in the case of CR3 for loading PTDPTRs, manually checks for an
> illegal CR3.  This particular failure doesn't hit the existing reserved
> bits checks because syzbot sets guest.MAXPHYADDR=1, and IA32 architecture
> simply doesn't allow for such an absurd MAXPHADDR, e.g. 32-bit paging

"MAXPHYADDR"

> doesn't define any reserved PA bits checks, which KVM emulates by only
> incorporating the reserved PA bits into the "high" bits, i.e. bits 63:32.
>
> Simply remove the bogus check.  There is zero meaningful value and no
> architectural justification for supporting guest.MAXPHYADDR < 32, and
> properly filling the exception would introduce non-trivial complexity.
>
> This reverts commit ec7771ab471ba6a945350353617e2e3385d0e013.
>
> Fixes: ec7771ab471b ("KVM: x86: mmu: Add guest physical address check in translate_gpa()")
> Cc: stable@vger.kernel.org
> Reported-by: syzbot+200c08e88ae818f849ce@syzkaller.appspotmail.com
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/mmu/mmu.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 4853c033e6ce..4b7908187d05 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -334,12 +334,6 @@ static bool check_mmio_spte(struct kvm_vcpu *vcpu, u64 spte)
>  static gpa_t translate_gpa(struct kvm_vcpu *vcpu, gpa_t gpa, u32 access,
>                                    struct x86_exception *exception)
>  {
> -	/* Check if guest physical address doesn't exceed guest maximum */
> -	if (kvm_vcpu_is_illegal_gpa(vcpu, gpa)) {
> -		exception->error_code |= PFERR_RSVD_MASK;
> -		return UNMAPPED_GVA;
> -	}
> -
>          return gpa;
>  }

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

I'm, however, wondering if it would also make sense to forbid setting
nonsensical MAXPHYADDR, something like (compile-only tested):

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index fe03bd978761..42e71ac8ff31 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -73,25 +73,6 @@ static inline struct kvm_cpuid_entry2 *cpuid_entry2_find(
 	return NULL;
 }
 
-static int kvm_check_cpuid(struct kvm_cpuid_entry2 *entries, int nent)
-{
-	struct kvm_cpuid_entry2 *best;
-
-	/*
-	 * The existing code assumes virtual address is 48-bit or 57-bit in the
-	 * canonical address checks; exit if it is ever changed.
-	 */
-	best = cpuid_entry2_find(entries, nent, 0x80000008, 0);
-	if (best) {
-		int vaddr_bits = (best->eax & 0xff00) >> 8;
-
-		if (vaddr_bits != 48 && vaddr_bits != 57 && vaddr_bits != 0)
-			return -EINVAL;
-	}
-
-	return 0;
-}
-
 void kvm_update_pv_runtime(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best;
@@ -208,20 +189,48 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 	kvm_mmu_after_set_cpuid(vcpu);
 }
 
-int cpuid_query_maxphyaddr(struct kvm_vcpu *vcpu)
+static int __cpuid_query_maxphyaddr(struct kvm_cpuid_entry2 *entries, int nent)
 {
 	struct kvm_cpuid_entry2 *best;
 
-	best = kvm_find_cpuid_entry(vcpu, 0x80000000, 0);
+	best = cpuid_entry2_find(entries, nent, 0x80000000, 0);
 	if (!best || best->eax < 0x80000008)
 		goto not_found;
-	best = kvm_find_cpuid_entry(vcpu, 0x80000008, 0);
+	best = cpuid_entry2_find(entries, nent, 0x80000008, 0);
 	if (best)
 		return best->eax & 0xff;
 not_found:
 	return 36;
 }
 
+int cpuid_query_maxphyaddr(struct kvm_vcpu *vcpu)
+{
+	return __cpuid_query_maxphyaddr(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent);
+}
+
+static int kvm_check_cpuid(struct kvm_cpuid_entry2 *entries, int nent)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	/* guest.MAXPHYADDR < 32 is completely nonsensical */
+	if (__cpuid_query_maxphyaddr(entries, nent) < 32)
+		return -EINVAL;
+
+	/*
+	 * The existing code assumes virtual address is 48-bit or 57-bit in the
+	 * canonical address checks; exit if it is ever changed.
+	 */
+	best = cpuid_entry2_find(entries, nent, 0x80000008, 0);
+	if (best) {
+		int vaddr_bits = (best->eax & 0xff00) >> 8;
+
+		if (vaddr_bits != 48 && vaddr_bits != 57 && vaddr_bits != 0)
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
 /*
  * This "raw" version returns the reserved GPA bits without any adjustments for
  * encryption technologies that usurp bits.  The raw mask should be used if and

-- 
Vitaly

Re: [PATCH 1/3] Revert "KVM: x86: mmu: Add guest physical address check in translate_gpa()"

[Sean Christopherson]

On Wed, Sep 01, 2021, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > Revert a misguided illegal GPA check when "translating" a non-nested GPA.
> > The check is woefully incomplete as it does not fill in @exception as
> > expected by all callers, which leads to KVM attempting to inject a bogus
> > exception, potentially exposing kernel stack information in the process.
> >
> >  WARNING: CPU: 0 PID: 8469 at arch/x86/kvm/x86.c:525 exception_type+0x98/0xb0 arch/x86/kvm/x86.c:525
> >  CPU: 1 PID: 8469 Comm: syz-executor531 Not tainted 5.14.0-rc7-syzkaller #0
> >  RIP: 0010:exception_type+0x98/0xb0 arch/x86/kvm/x86.c:525
> >  Call Trace:
> >   x86_emulate_instruction+0xef6/0x1460 arch/x86/kvm/x86.c:7853
> >   kvm_mmu_page_fault+0x2f0/0x1810 arch/x86/kvm/mmu/mmu.c:5199
> >   handle_ept_misconfig+0xdf/0x3e0 arch/x86/kvm/vmx/vmx.c:5336
> >   __vmx_handle_exit arch/x86/kvm/vmx/vmx.c:6021 [inline]
> >   vmx_handle_exit+0x336/0x1800 arch/x86/kvm/vmx/vmx.c:6038
> >   vcpu_enter_guest+0x2a1c/0x4430 arch/x86/kvm/x86.c:9712
> >   vcpu_run arch/x86/kvm/x86.c:9779 [inline]
> >   kvm_arch_vcpu_ioctl_run+0x47d/0x1b20 arch/x86/kvm/x86.c:10010
> >   kvm_vcpu_ioctl+0x49e/0xe50 arch/x86/kvm/../../../virt/kvm/kvm_main.c:3652
> >
> > The bug has escaped notice because practically speaking the GPA check is
> > useless.  The GPA check in question only comes into play when KVM is
> > walking guest page tables (or "translating" CR3), and KVM already handles
> > illegal GPA checks by setting reserved bits in rsvd_bits_mask for each
> > PxE, or in the case of CR3 for loading PTDPTRs, manually checks for an
> > illegal CR3.  This particular failure doesn't hit the existing reserved
> > bits checks because syzbot sets guest.MAXPHYADDR=1, and IA32 architecture
> > simply doesn't allow for such an absurd MAXPHADDR, e.g. 32-bit paging
> 
> "MAXPHYADDR"

Gah, I'm pretty sure I mistype that 50% of the time.

> I'm, however, wondering if it would also make sense to forbid setting
> nonsensical MAXPHYADDR, something like (compile-only tested):

That crossed my mind as well, but I actually think letting userspace provide
garbage is a good thing in this case.  From a kernel-safety perspective, KVM
does not and _should_ not make any assumptions about guest.MAXPHYADDR.  IMO, the
added test coverage gaind by allowing truly outrageous values outweighs the
potential danger, e.g. this bogus code would likely have gone unnoticed forever.

Re: [PATCH 1/3] Revert "KVM: x86: mmu: Add guest physical address check in translate_gpa()"

[Paolo Bonzini]

On 31/08/21 18:42, Sean Christopherson wrote:
> Revert a misguided illegal GPA check when "translating" a non-nested GPA.
> The check is woefully incomplete as it does not fill in @exception as
> expected by all callers, which leads to KVM attempting to inject a bogus
> exception, potentially exposing kernel stack information in the process.
> 
>   WARNING: CPU: 0 PID: 8469 at arch/x86/kvm/x86.c:525 exception_type+0x98/0xb0 arch/x86/kvm/x86.c:525
>   CPU: 1 PID: 8469 Comm: syz-executor531 Not tainted 5.14.0-rc7-syzkaller #0
>   RIP: 0010:exception_type+0x98/0xb0 arch/x86/kvm/x86.c:525
>   Call Trace:
>    x86_emulate_instruction+0xef6/0x1460 arch/x86/kvm/x86.c:7853
>    kvm_mmu_page_fault+0x2f0/0x1810 arch/x86/kvm/mmu/mmu.c:5199
>    handle_ept_misconfig+0xdf/0x3e0 arch/x86/kvm/vmx/vmx.c:5336
>    __vmx_handle_exit arch/x86/kvm/vmx/vmx.c:6021 [inline]
>    vmx_handle_exit+0x336/0x1800 arch/x86/kvm/vmx/vmx.c:6038
>    vcpu_enter_guest+0x2a1c/0x4430 arch/x86/kvm/x86.c:9712
>    vcpu_run arch/x86/kvm/x86.c:9779 [inline]
>    kvm_arch_vcpu_ioctl_run+0x47d/0x1b20 arch/x86/kvm/x86.c:10010
>    kvm_vcpu_ioctl+0x49e/0xe50 arch/x86/kvm/../../../virt/kvm/kvm_main.c:3652
> 
> The bug has escaped notice because practically speaking the GPA check is
> useless.  The GPA check in question only comes into play when KVM is
> walking guest page tables (or "translating" CR3), and KVM already handles
> illegal GPA checks by setting reserved bits in rsvd_bits_mask for each
> PxE, or in the case of CR3 for loading PTDPTRs, manually checks for an
> illegal CR3.  This particular failure doesn't hit the existing reserved
> bits checks because syzbot sets guest.MAXPHYADDR=1, and IA32 architecture
> simply doesn't allow for such an absurd MAXPHADDR, e.g. 32-bit paging
> doesn't define any reserved PA bits checks, which KVM emulates by only
> incorporating the reserved PA bits into the "high" bits, i.e. bits 63:32.
> 
> Simply remove the bogus check.  There is zero meaningful value and no
> architectural justification for supporting guest.MAXPHYADDR < 32, and
> properly filling the exception would introduce non-trivial complexity.
> 
> This reverts commit ec7771ab471ba6a945350353617e2e3385d0e013.
> 
> Fixes: ec7771ab471b ("KVM: x86: mmu: Add guest physical address check in translate_gpa()")
> Cc: stable@vger.kernel.org
> Reported-by: syzbot+200c08e88ae818f849ce@syzkaller.appspotmail.com
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/mmu/mmu.c | 6 ------
>   1 file changed, 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 4853c033e6ce..4b7908187d05 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -334,12 +334,6 @@ static bool check_mmio_spte(struct kvm_vcpu *vcpu, u64 spte)
>   static gpa_t translate_gpa(struct kvm_vcpu *vcpu, gpa_t gpa, u32 access,
>                                     struct x86_exception *exception)
>   {
> -	/* Check if guest physical address doesn't exceed guest maximum */
> -	if (kvm_vcpu_is_illegal_gpa(vcpu, gpa)) {
> -		exception->error_code |= PFERR_RSVD_MASK;
> -		return UNMAPPED_GVA;
> -	}
> -
>           return gpa;
>   }
>   
> 

Queued this one only, for now.  Thanks,

Paolo

[PATCH 2/3] KVM: x86: Subsume nested GPA read helper into load_pdptrs()

[Sean Christopherson]

Open code the call to mmu->translate_gpa() when loading nested PDPTRs and
kill off the existing helper, kvm_read_guest_page_mmu(), to discourage
incorrect use.  Reading guest memory straight from an L2 GPA is extremely
rare (as evidenced by the lack of users), as very few constructs in x86
specify physical addresses, even fewer are virtualized by KVM, and even
fewer yet require emulation of L2 by L0 KVM.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/kvm_host.h |  3 --
 arch/x86/kvm/x86.c              | 56 +++++++++++----------------------
 2 files changed, 18 insertions(+), 41 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 09b256db394a..ec26a929b94b 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1703,9 +1703,6 @@ void kvm_requeue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code)
 void kvm_inject_page_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault);
 bool kvm_inject_emulated_page_fault(struct kvm_vcpu *vcpu,
 				    struct x86_exception *fault);
-int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
-			    gfn_t gfn, void *data, int offset, int len,
-			    u32 access);
 bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
 bool kvm_require_dr(struct kvm_vcpu *vcpu, int dr);
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 86539c1686fa..8bd76698be52 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -790,30 +790,6 @@ bool kvm_require_dr(struct kvm_vcpu *vcpu, int dr)
 }
 EXPORT_SYMBOL_GPL(kvm_require_dr);
 
-/*
- * This function will be used to read from the physical memory of the currently
- * running guest. The difference to kvm_vcpu_read_guest_page is that this function
- * can read from guest physical or from the guest's guest physical memory.
- */
-int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
-			    gfn_t ngfn, void *data, int offset, int len,
-			    u32 access)
-{
-	struct x86_exception exception;
-	gfn_t real_gfn;
-	gpa_t ngpa;
-
-	ngpa     = gfn_to_gpa(ngfn);
-	real_gfn = mmu->translate_gpa(vcpu, ngpa, access, &exception);
-	if (real_gfn == UNMAPPED_GVA)
-		return -EFAULT;
-
-	real_gfn = gpa_to_gfn(real_gfn);
-
-	return kvm_vcpu_read_guest_page(vcpu, real_gfn, data, offset, len);
-}
-EXPORT_SYMBOL_GPL(kvm_read_guest_page_mmu);
-
 static inline u64 pdptr_rsvd_bits(struct kvm_vcpu *vcpu)
 {
 	return vcpu->arch.reserved_gpa_bits | rsvd_bits(5, 8) | rsvd_bits(1, 2);
@@ -825,34 +801,38 @@ static inline u64 pdptr_rsvd_bits(struct kvm_vcpu *vcpu)
 int load_pdptrs(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, unsigned long cr3)
 {
 	gfn_t pdpt_gfn = cr3 >> PAGE_SHIFT;
-	unsigned offset = ((cr3 & (PAGE_SIZE-1)) >> 5) << 2;
+	unsigned offset = (((cr3 & (PAGE_SIZE-1)) >> 5) << 2) * sizeof(u64);
+	gpa_t real_gpa;
 	int i;
 	int ret;
 	u64 pdpte[ARRAY_SIZE(mmu->pdptrs)];
 
-	ret = kvm_read_guest_page_mmu(vcpu, mmu, pdpt_gfn, pdpte,
-				      offset * sizeof(u64), sizeof(pdpte),
-				      PFERR_USER_MASK|PFERR_WRITE_MASK);
-	if (ret < 0) {
-		ret = 0;
-		goto out;
-	}
+	/*
+	 * If the MMU is nested, CR3 holds an L2 GPA and needs to be translated
+	 * to an L1 GPA.
+	 */
+	real_gpa = mmu->translate_gpa(vcpu, gfn_to_gpa(pdpt_gfn),
+				      PFERR_USER_MASK | PFERR_WRITE_MASK, NULL);
+	if (real_gpa == UNMAPPED_GVA)
+		return 0;
+
+	ret = kvm_vcpu_read_guest_page(vcpu, gpa_to_gfn(real_gpa), pdpte,
+				       offset, sizeof(pdpte));
+	if (ret < 0)
+		return 0;
+
 	for (i = 0; i < ARRAY_SIZE(pdpte); ++i) {
 		if ((pdpte[i] & PT_PRESENT_MASK) &&
 		    (pdpte[i] & pdptr_rsvd_bits(vcpu))) {
-			ret = 0;
-			goto out;
+			return 0;
 		}
 	}
-	ret = 1;
 
 	memcpy(mmu->pdptrs, pdpte, sizeof(mmu->pdptrs));
 	kvm_register_mark_dirty(vcpu, VCPU_EXREG_PDPTR);
 	vcpu->arch.pdptrs_from_userspace = false;
 
-out:
-
-	return ret;
+	return 1;
 }
 EXPORT_SYMBOL_GPL(load_pdptrs);
 
-- 
2.33.0.259.gc128427fd7-goog

[PATCH 3/3] KVM: x86: Simplify retrieving the page offset when loading PDTPRs

[Sean Christopherson]

Replace impressively complex "logic" for computing the page offset from
CR3 when loading PDPTRs.  Unlike other paging modes, the address held in
CR3 for PAE paging is 32-byte aligned, i.e. occupies bits 31:5, thus bits
11:5 need to be used as the offset from the gfn when reading PDPTRs.

The existing calculation originated in commit 1342d3536d6a ("[PATCH] KVM:
MMU: Load the pae pdptrs on cr3 change like the processor does"), which
read the PDPTRs from guest memory as individual 8-byte loads.  At the
time, the so called "offset" was the base index of PDPTR0 as a _u64_, not
a byte offset.  Naming aside, the computation was useful and arguably
simplified the overall flow.

Unfortunately, when commit 195aefde9cc2 ("KVM: Add general accessors to
read and write guest memory") added accessors with offsets at byte
granularity, the cleverness of the original code was lost and KVM was
left with convoluted code for a simple operation.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8bd76698be52..aa41ed693b0a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -801,7 +801,6 @@ static inline u64 pdptr_rsvd_bits(struct kvm_vcpu *vcpu)
 int load_pdptrs(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, unsigned long cr3)
 {
 	gfn_t pdpt_gfn = cr3 >> PAGE_SHIFT;
-	unsigned offset = (((cr3 & (PAGE_SIZE-1)) >> 5) << 2) * sizeof(u64);
 	gpa_t real_gpa;
 	int i;
 	int ret;
@@ -816,8 +815,9 @@ int load_pdptrs(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, unsigned long cr3)
 	if (real_gpa == UNMAPPED_GVA)
 		return 0;
 
+	/* Note the offset, PDPTRs are 32 byte aligned when using PAE paging. */
 	ret = kvm_vcpu_read_guest_page(vcpu, gpa_to_gfn(real_gpa), pdpte,
-				       offset, sizeof(pdpte));
+				       cr3 & GENMASK(11, 5), sizeof(pdpte));
 	if (ret < 0)
 		return 0;
 
-- 
2.33.0.259.gc128427fd7-goog

Re: [PATCH 0/3] KVM: x86: guest.MAXPHYADDR fix and related cleanup

[Paolo Bonzini]

On 31/08/21 18:42, Sean Christopherson wrote:
> Fix a bug exposed by syzkaller where running with guest.MAXPHYADDR < 32
> leads KVM injecting a garbage exception due to mishandling an illegal GPA
> check when "translating" a non-nested GPA.
> 
> Clean up tangentially related code in load_pdptrs() to discourage reading
> guest memory using a potentially-nested GPA.
> 
> Sean Christopherson (3):
>    Revert "KVM: x86: mmu: Add guest physical address check in
>      translate_gpa()"
>    KVM: x86: Subsume nested GPA read helper into load_pdptrs()
>    KVM: x86: Simplify retrieving the page offset when loading PDTPRs
> 
>   arch/x86/include/asm/kvm_host.h |  3 --
>   arch/x86/kvm/mmu/mmu.c          |  6 ----
>   arch/x86/kvm/x86.c              | 56 +++++++++++----------------------
>   3 files changed, 18 insertions(+), 47 deletions(-)
> 

Queued 2 and 3 too now, thanks.

Paolo