diff mbox

[2/2] arm/arm64: KVM: Ensure memslots are within KVM_PHYS_SIZE

Message ID 1411674174-30672-3-git-send-email-christoffer.dall@linaro.org
State New
Headers show

Commit Message

Christoffer Dall Sept. 25, 2014, 7:42 p.m. UTC 
When creating or moving a memslot, make sure the IPA space is within the
addressable range of the guest.  Otherwise, user space can create too
large a memslot and KVM would try to access potentially unallocated page
table entries when inserting entries in the Stage-2 page tables.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 arch/arm/kvm/mmu.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Catalin Marinas Sept. 30, 2014, 12:46 p.m. UTC | #1 
On Thu, Sep 25, 2014 at 08:42:54PM +0100, Christoffer Dall wrote:
> When creating or moving a memslot, make sure the IPA space is within the
> addressable range of the guest.  Otherwise, user space can create too
> large a memslot and KVM would try to access potentially unallocated page
> table entries when inserting entries in the Stage-2 page tables.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  arch/arm/kvm/mmu.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 4532f5f..52a311a 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -975,6 +975,9 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		goto out_unlock;
>  	}
>  
> +	/* Userspace should not be able to register out-of-bounds IPAs */

I think "userspace" is a bit misleading (should be "guests").

> +	VM_BUG_ON(fault_ipa >= KVM_PHYS_SIZE);

Can guests not generate IPA addresses higher than KVM_PHYS_SIZE? I don't
see why this wouldn't be possible when PARange > 40.
Christoffer Dall Oct. 6, 2014, 1:47 p.m. UTC | #2 
On Tue, Sep 30, 2014 at 01:46:51PM +0100, Catalin Marinas wrote:
> On Thu, Sep 25, 2014 at 08:42:54PM +0100, Christoffer Dall wrote:
> > When creating or moving a memslot, make sure the IPA space is within the
> > addressable range of the guest.  Otherwise, user space can create too
> > large a memslot and KVM would try to access potentially unallocated page
> > table entries when inserting entries in the Stage-2 page tables.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  arch/arm/kvm/mmu.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> > index 4532f5f..52a311a 100644
> > --- a/arch/arm/kvm/mmu.c
> > +++ b/arch/arm/kvm/mmu.c
> > @@ -975,6 +975,9 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >  		goto out_unlock;
> >  	}
> >  
> > +	/* Userspace should not be able to register out-of-bounds IPAs */
> 
> I think "userspace" is a bit misleading (should be "guests").
> 

see below

> > +	VM_BUG_ON(fault_ipa >= KVM_PHYS_SIZE);
> 
> Can guests not generate IPA addresses higher than KVM_PHYS_SIZE? I don't
> see why this wouldn't be possible when PARange > 40.
> 
Guests can, but higher in this function we resolve the the gfn (IPA) to
a memslot (hva).  That only succeeds if userspace actually managed to
register a memslot with such an IPA, which we prevent in the other part
of this patch.  So if we get here, it's a bug, because we should have
entered the section above and taken the goto out_unlock.

Makes sense?

Thanks,
-Christoffer
Catalin Marinas Oct. 6, 2014, 4:02 p.m. UTC | #3 
On Mon, Oct 06, 2014 at 02:47:01PM +0100, Christoffer Dall wrote:
> On Tue, Sep 30, 2014 at 01:46:51PM +0100, Catalin Marinas wrote:
> > On Thu, Sep 25, 2014 at 08:42:54PM +0100, Christoffer Dall wrote:
> > > When creating or moving a memslot, make sure the IPA space is within the
> > > addressable range of the guest.  Otherwise, user space can create too
> > > large a memslot and KVM would try to access potentially unallocated page
> > > table entries when inserting entries in the Stage-2 page tables.
> > > 
> > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > > ---
> > >  arch/arm/kvm/mmu.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> > > index 4532f5f..52a311a 100644
> > > --- a/arch/arm/kvm/mmu.c
> > > +++ b/arch/arm/kvm/mmu.c
> > > @@ -975,6 +975,9 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
> > >  		goto out_unlock;
> > >  	}
> > >  
> > > +	/* Userspace should not be able to register out-of-bounds IPAs */
> > 
> > I think "userspace" is a bit misleading (should be "guests").
> 
> see below
> 
> > > +	VM_BUG_ON(fault_ipa >= KVM_PHYS_SIZE);
> > 
> > Can guests not generate IPA addresses higher than KVM_PHYS_SIZE? I don't
> > see why this wouldn't be possible when PARange > 40.
> > 
> Guests can, but higher in this function we resolve the the gfn (IPA) to
> a memslot (hva).  That only succeeds if userspace actually managed to
> register a memslot with such an IPA, which we prevent in the other part
> of this patch.  So if we get here, it's a bug, because we should have
> entered the section above and taken the goto out_unlock.

OK. I got it now.
diff mbox

Patch

diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 4532f5f..52a311a 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -975,6 +975,9 @@  int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		goto out_unlock;
 	}
 
+	/* Userspace should not be able to register out-of-bounds IPAs */
+	VM_BUG_ON(fault_ipa >= KVM_PHYS_SIZE);
+
 	ret = user_mem_abort(vcpu, fault_ipa, memslot, hva, fault_status);
 	if (ret == 0)
 		ret = 1;
@@ -1201,6 +1204,11 @@  int kvm_arch_prepare_memory_region(struct kvm *kvm,
 				   struct kvm_userspace_memory_region *mem,
 				   enum kvm_mr_change change)
 {
+	if (change == KVM_MR_CREATE || change == KVM_MR_MOVE) {
+		if (memslot->base_gfn + memslot->npages >=
+		    (KVM_PHYS_SIZE >> PAGE_SHIFT))
+			return -EFAULT;
+	}
 	return 0;
 }