[PATCH] KVM: VMX: Judge MMIO based on PFN rather than HVA in EPT violation

[PATCH] KVM: VMX: Judge MMIO based on PFN rather than HVA in EPT violation

[Sheng Yang]

One page can be unmapped from userspace, then HVA seems legal, but in fact,
PFN is illegal.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/x86/kvm/vmx.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 9913a1d..a4fa1b5 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3061,7 +3061,7 @@ static int handle_ept_violation(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	u64 exit_qualification;
 	enum emulation_result er;
 	gpa_t gpa;
-	unsigned long hva;
+	pfn_t pfn;
 	int gla_validity;
 	int r;
 
@@ -3086,8 +3086,8 @@ static int handle_ept_violation(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	}
 
 	gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
-	hva = gfn_to_hva(vcpu->kvm, gpa >> PAGE_SHIFT);
-	if (!kvm_is_error_hva(hva)) {
+	pfn = gfn_to_pfn(vcpu->kvm, gpa >> PAGE_SHIFT);
+	if (!is_error_pfn(pfn)) {
 		r = kvm_mmu_page_fault(vcpu, gpa & PAGE_MASK, 0);
 		if (r < 0) {
 			printk(KERN_ERR "EPT: Not enough memory!\n");
-- 
1.5.4.5

Re: [PATCH] KVM: VMX: Judge MMIO based on PFN rather than HVA in EPT violation

[Marcelo Tosatti]

Hi Sheng,

On Tue, Feb 10, 2009 at 05:43:41PM +0800, Sheng Yang wrote:
> One page can be unmapped from userspace, then HVA seems legal, but in fact,
> PFN is illegal.
> 
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  arch/x86/kvm/vmx.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 9913a1d..a4fa1b5 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -3061,7 +3061,7 @@ static int handle_ept_violation(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  	u64 exit_qualification;
>  	enum emulation_result er;
>  	gpa_t gpa;
> -	unsigned long hva;
> +	pfn_t pfn;
>  	int gla_validity;
>  	int r;
>  
> @@ -3086,8 +3086,8 @@ static int handle_ept_violation(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  	}
>  
>  	gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
> -	hva = gfn_to_hva(vcpu->kvm, gpa >> PAGE_SHIFT);
> -	if (!kvm_is_error_hva(hva)) {
> +	pfn = gfn_to_pfn(vcpu->kvm, gpa >> PAGE_SHIFT);
> +	if (!is_error_pfn(pfn)) {
>  		r = kvm_mmu_page_fault(vcpu, gpa & PAGE_MASK, 0);
>  		if (r < 0) {
>  			printk(KERN_ERR "EPT: Not enough memory!\n");

Forgot to drop the additional reference acquired by gfn_to_pfn ?

Hum, gfn_to_hva assumes the slot information is accurate, which is not
the case if you simply munmap a page in the middle of a slot... we
should provide a helper in userspace, and make sure nothing bad can
happen otherwise. 

Anyway, why don't you just let kvm_mmu_page_fault handle instruction
emulation instead of opencoding it in handle_ept_violation ?

Re: [PATCH] KVM: VMX: Judge MMIO based on PFN rather than HVA in EPT violation

[Sheng Yang]

On Tuesday 10 February 2009 20:14:32 Marcelo Tosatti wrote:
> Hi Sheng,
>
> On Tue, Feb 10, 2009 at 05:43:41PM +0800, Sheng Yang wrote:
> > One page can be unmapped from userspace, then HVA seems legal, but in
> > fact, PFN is illegal.
> >
> > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > ---
> >  arch/x86/kvm/vmx.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 9913a1d..a4fa1b5 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -3061,7 +3061,7 @@ static int handle_ept_violation(struct kvm_vcpu
> > *vcpu, struct kvm_run *kvm_run) u64 exit_qualification;
> >  	enum emulation_result er;
> >  	gpa_t gpa;
> > -	unsigned long hva;
> > +	pfn_t pfn;
> >  	int gla_validity;
> >  	int r;
> >
> > @@ -3086,8 +3086,8 @@ static int handle_ept_violation(struct kvm_vcpu
> > *vcpu, struct kvm_run *kvm_run) }
> >
> >  	gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
> > -	hva = gfn_to_hva(vcpu->kvm, gpa >> PAGE_SHIFT);
> > -	if (!kvm_is_error_hva(hva)) {
> > +	pfn = gfn_to_pfn(vcpu->kvm, gpa >> PAGE_SHIFT);
> > +	if (!is_error_pfn(pfn)) {
> >  		r = kvm_mmu_page_fault(vcpu, gpa & PAGE_MASK, 0);
> >  		if (r < 0) {
> >  			printk(KERN_ERR "EPT: Not enough memory!\n");
>
> Forgot to drop the additional reference acquired by gfn_to_pfn ?

Sadly, yeah... :(
>
> Hum, gfn_to_hva assumes the slot information is accurate, which is not
> the case if you simply munmap a page in the middle of a slot... we
> should provide a helper in userspace, and make sure nothing bad can
> happen otherwise.

Yes. But maintain a list in kernel seems a little complex for it. I checked 
the callings of gfn_to_hva, seems most of them are safe with a inaccurate 
result(not sure about host_largepage_backed, seems this would be affected if 
one 4k page was unmapped inside a large page).
>
> Anyway, why don't you just let kvm_mmu_page_fault handle instruction
> emulation instead of opencoding it in handle_ept_violation ?

It didn't work as expect. Checking it.

-- 
regards
Yang, Sheng

Re: [PATCH] KVM: VMX: Judge MMIO based on PFN rather than HVA in EPT violation

[Sheng Yang]

On Wednesday 11 February 2009 10:43:43 Sheng Yang wrote:
> On Tuesday 10 February 2009 20:14:32 Marcelo Tosatti wrote:
> > Hi Sheng,
> >
> > On Tue, Feb 10, 2009 at 05:43:41PM +0800, Sheng Yang wrote:
> > > One page can be unmapped from userspace, then HVA seems legal, but in
> > > fact, PFN is illegal.
> > >
> > > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > > ---
> > >  arch/x86/kvm/vmx.c |    6 +++---
> > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > > index 9913a1d..a4fa1b5 100644
> > > --- a/arch/x86/kvm/vmx.c
> > > +++ b/arch/x86/kvm/vmx.c
> > > @@ -3061,7 +3061,7 @@ static int handle_ept_violation(struct kvm_vcpu
> > > *vcpu, struct kvm_run *kvm_run) u64 exit_qualification;
> > >  	enum emulation_result er;
> > >  	gpa_t gpa;
> > > -	unsigned long hva;
> > > +	pfn_t pfn;
> > >  	int gla_validity;
> > >  	int r;
> > >
> > > @@ -3086,8 +3086,8 @@ static int handle_ept_violation(struct kvm_vcpu
> > > *vcpu, struct kvm_run *kvm_run) }
> > >
> > >  	gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
> > > -	hva = gfn_to_hva(vcpu->kvm, gpa >> PAGE_SHIFT);
> > > -	if (!kvm_is_error_hva(hva)) {
> > > +	pfn = gfn_to_pfn(vcpu->kvm, gpa >> PAGE_SHIFT);
> > > +	if (!is_error_pfn(pfn)) {
> > >  		r = kvm_mmu_page_fault(vcpu, gpa & PAGE_MASK, 0);
> > >  		if (r < 0) {
> > >  			printk(KERN_ERR "EPT: Not enough memory!\n");
> >
> > Forgot to drop the additional reference acquired by gfn_to_pfn ?
>
> Sadly, yeah... :(
>
> > Hum, gfn_to_hva assumes the slot information is accurate, which is not
> > the case if you simply munmap a page in the middle of a slot... we
> > should provide a helper in userspace, and make sure nothing bad can
> > happen otherwise.
>
> Yes. But maintain a list in kernel seems a little complex for it. I checked
> the callings of gfn_to_hva, seems most of them are safe with a inaccurate
> result(not sure about host_largepage_backed, seems this would be affected
> if one 4k page was unmapped inside a large page).
>
> > Anyway, why don't you just let kvm_mmu_page_fault handle instruction
> > emulation instead of opencoding it in handle_ept_violation ?
>
> It didn't work as expect. Checking it.

Oh, seems it didn't work due to my fault... Update the patch, thanks for 
reminder. :)

-- 
regards
Yang, Sheng