kvm: Ensure writes to the coalesced MMIO ring are within bounds
diff mbox series

Message ID 20190918131545.6405-1-will@kernel.org
State New
Headers show
Series
  • kvm: Ensure writes to the coalesced MMIO ring are within bounds
Related show

Commit Message

Will Deacon Sept. 18, 2019, 1:15 p.m. UTC
When records are written to the coalesced MMIO ring in response to a
vCPU MMIO exit, the 'ring->last' field is used to index the ring buffer
page. Although we hold the 'kvm->ring_lock' at this point, the ring
structure is mapped directly into the host userspace and can therefore
be modified to point at arbitrary pages within the kernel.

Since this shouldn't happen in normal operation, simply bound the index
by KVM_COALESCED_MMIO_MAX to contain the accesses within the ring buffer
page.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: <stable@kernel.org> # 5.2.y
Fixes: 5f94c1741bdc ("KVM: Add coalesced MMIO support (common part)")
Reported-by: Bill Creasey <bcreasey@google.com>
Signed-off-by: Will Deacon <will@kernel.org>
---

I think there are some other fixes kicking around for this, but they
still rely on 'ring->last' being stable, which isn't necessarily the
case. I'll send the -stable backport for kernels prior to 5.2 once this
hits mainline.

 virt/kvm/coalesced_mmio.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

gregkh@linuxfoundation.org Sept. 18, 2019, 1:30 p.m. UTC | #1
On Wed, Sep 18, 2019 at 02:15:45PM +0100, Will Deacon wrote:
> When records are written to the coalesced MMIO ring in response to a
> vCPU MMIO exit, the 'ring->last' field is used to index the ring buffer
> page. Although we hold the 'kvm->ring_lock' at this point, the ring
> structure is mapped directly into the host userspace and can therefore
> be modified to point at arbitrary pages within the kernel.
> 
> Since this shouldn't happen in normal operation, simply bound the index
> by KVM_COALESCED_MMIO_MAX to contain the accesses within the ring buffer
> page.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: <stable@kernel.org> # 5.2.y
> Fixes: 5f94c1741bdc ("KVM: Add coalesced MMIO support (common part)")
> Reported-by: Bill Creasey <bcreasey@google.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
> 
> I think there are some other fixes kicking around for this, but they
> still rely on 'ring->last' being stable, which isn't necessarily the
> case. I'll send the -stable backport for kernels prior to 5.2 once this
> hits mainline.
> 
>  virt/kvm/coalesced_mmio.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
> index 5294abb3f178..09b3e4421550 100644
> --- a/virt/kvm/coalesced_mmio.c
> +++ b/virt/kvm/coalesced_mmio.c
> @@ -67,6 +67,7 @@ static int coalesced_mmio_write(struct kvm_vcpu *vcpu,
>  {
>  	struct kvm_coalesced_mmio_dev *dev = to_mmio(this);
>  	struct kvm_coalesced_mmio_ring *ring = dev->kvm->coalesced_mmio_ring;
> +	u32 last;
>  
>  	if (!coalesced_mmio_in_range(dev, addr, len))
>  		return -EOPNOTSUPP;
> @@ -79,13 +80,13 @@ static int coalesced_mmio_write(struct kvm_vcpu *vcpu,
>  	}
>  
>  	/* copy data in first free entry of the ring */
> -
> -	ring->coalesced_mmio[ring->last].phys_addr = addr;
> -	ring->coalesced_mmio[ring->last].len = len;
> -	memcpy(ring->coalesced_mmio[ring->last].data, val, len);
> -	ring->coalesced_mmio[ring->last].pio = dev->zone.pio;
> +	last = ring->last % KVM_COALESCED_MMIO_MAX;
> +	ring->coalesced_mmio[last].phys_addr = addr;
> +	ring->coalesced_mmio[last].len = len;
> +	memcpy(ring->coalesced_mmio[last].data, val, len);
> +	ring->coalesced_mmio[last].pio = dev->zone.pio;
>  	smp_wmb();
> -	ring->last = (ring->last + 1) % KVM_COALESCED_MMIO_MAX;
> +	ring->last = (last + 1) % KVM_COALESCED_MMIO_MAX;
>  	spin_unlock(&dev->kvm->ring_lock);
>  	return 0;
>  }

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Paolo Bonzini Sept. 18, 2019, 1:41 p.m. UTC | #2
On 18/09/19 15:15, Will Deacon wrote:
> When records are written to the coalesced MMIO ring in response to a
> vCPU MMIO exit, the 'ring->last' field is used to index the ring buffer
> page. Although we hold the 'kvm->ring_lock' at this point, the ring
> structure is mapped directly into the host userspace and can therefore
> be modified to point at arbitrary pages within the kernel.
> 
> Since this shouldn't happen in normal operation, simply bound the index
> by KVM_COALESCED_MMIO_MAX to contain the accesses within the ring buffer
> page.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: <stable@kernel.org> # 5.2.y
> Fixes: 5f94c1741bdc ("KVM: Add coalesced MMIO support (common part)")
> Reported-by: Bill Creasey <bcreasey@google.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
> 
> I think there are some other fixes kicking around for this, but they
> still rely on 'ring->last' being stable, which isn't necessarily the
> case. I'll send the -stable backport for kernels prior to 5.2 once this
> hits mainline.

Google's patch, which checks if ring->last is not in range and fails
with -EOPNOTSUPP if not, is slightly better.  I'll send it in a second
and Cc you (and also send it as a pull request to Linus).

Paolo

>  virt/kvm/coalesced_mmio.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
> index 5294abb3f178..09b3e4421550 100644
> --- a/virt/kvm/coalesced_mmio.c
> +++ b/virt/kvm/coalesced_mmio.c
> @@ -67,6 +67,7 @@ static int coalesced_mmio_write(struct kvm_vcpu *vcpu,
>  {
>  	struct kvm_coalesced_mmio_dev *dev = to_mmio(this);
>  	struct kvm_coalesced_mmio_ring *ring = dev->kvm->coalesced_mmio_ring;
> +	u32 last;
>  
>  	if (!coalesced_mmio_in_range(dev, addr, len))
>  		return -EOPNOTSUPP;
> @@ -79,13 +80,13 @@ static int coalesced_mmio_write(struct kvm_vcpu *vcpu,
>  	}
>  
>  	/* copy data in first free entry of the ring */
> -
> -	ring->coalesced_mmio[ring->last].phys_addr = addr;
> -	ring->coalesced_mmio[ring->last].len = len;
> -	memcpy(ring->coalesced_mmio[ring->last].data, val, len);
> -	ring->coalesced_mmio[ring->last].pio = dev->zone.pio;
> +	last = ring->last % KVM_COALESCED_MMIO_MAX;
> +	ring->coalesced_mmio[last].phys_addr = addr;
> +	ring->coalesced_mmio[last].len = len;
> +	memcpy(ring->coalesced_mmio[last].data, val, len);
> +	ring->coalesced_mmio[last].pio = dev->zone.pio;
>  	smp_wmb();
> -	ring->last = (ring->last + 1) % KVM_COALESCED_MMIO_MAX;
> +	ring->last = (last + 1) % KVM_COALESCED_MMIO_MAX;
>  	spin_unlock(&dev->kvm->ring_lock);
>  	return 0;
>  }
>
Will Deacon Sept. 18, 2019, 1:59 p.m. UTC | #3
On Wed, Sep 18, 2019 at 03:41:40PM +0200, Paolo Bonzini wrote:
> On 18/09/19 15:15, Will Deacon wrote:
> > When records are written to the coalesced MMIO ring in response to a
> > vCPU MMIO exit, the 'ring->last' field is used to index the ring buffer
> > page. Although we hold the 'kvm->ring_lock' at this point, the ring
> > structure is mapped directly into the host userspace and can therefore
> > be modified to point at arbitrary pages within the kernel.
> > 
> > Since this shouldn't happen in normal operation, simply bound the index
> > by KVM_COALESCED_MMIO_MAX to contain the accesses within the ring buffer
> > page.
> > 
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: <stable@kernel.org> # 5.2.y
> > Fixes: 5f94c1741bdc ("KVM: Add coalesced MMIO support (common part)")
> > Reported-by: Bill Creasey <bcreasey@google.com>
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> > 
> > I think there are some other fixes kicking around for this, but they
> > still rely on 'ring->last' being stable, which isn't necessarily the
> > case. I'll send the -stable backport for kernels prior to 5.2 once this
> > hits mainline.
> 
> Google's patch, which checks if ring->last is not in range and fails
> with -EOPNOTSUPP if not, is slightly better.  I'll send it in a second
> and Cc you (and also send it as a pull request to Linus).

Okey doke, as long as it gets fixed! My minor concerns with the error-checking
variant are:

  * Whether or not you need a READ_ONCE to prevent the compiler potentially
    reloading 'ring->last' after validation

  * Whether or not this could be part of a spectre-v1 gadget

so, given that I don't think the malicious host deserves an error code if it
starts writing the 'last' index, I went with the "obviously safe" version.
But up to you.

Will
Paolo Bonzini Sept. 18, 2019, 2:11 p.m. UTC | #4
On 18/09/19 15:59, Will Deacon wrote:
> Okey doke, as long as it gets fixed! My minor concerns with the error-checking
> variant are:
> 
>   * Whether or not you need a READ_ONCE to prevent the compiler potentially
>     reloading 'ring->last' after validation

Yes, it certainly needs READ_ONCE.  I had already added it locally, indeed.

>   * Whether or not this could be part of a spectre-v1 gadget

I think not, the spectrev1 gadget require a load that is indexed from
the contents of ring->coalesced_mmio[insert], but there's none.

Paolo

> so, given that I don't think the malicious host deserves an error code if it
> starts writing the 'last' index, I went with the "obviously safe" version.
> But up to you.
> 
> Will
>

Patch
diff mbox series

diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
index 5294abb3f178..09b3e4421550 100644
--- a/virt/kvm/coalesced_mmio.c
+++ b/virt/kvm/coalesced_mmio.c
@@ -67,6 +67,7 @@  static int coalesced_mmio_write(struct kvm_vcpu *vcpu,
 {
 	struct kvm_coalesced_mmio_dev *dev = to_mmio(this);
 	struct kvm_coalesced_mmio_ring *ring = dev->kvm->coalesced_mmio_ring;
+	u32 last;
 
 	if (!coalesced_mmio_in_range(dev, addr, len))
 		return -EOPNOTSUPP;
@@ -79,13 +80,13 @@  static int coalesced_mmio_write(struct kvm_vcpu *vcpu,
 	}
 
 	/* copy data in first free entry of the ring */
-
-	ring->coalesced_mmio[ring->last].phys_addr = addr;
-	ring->coalesced_mmio[ring->last].len = len;
-	memcpy(ring->coalesced_mmio[ring->last].data, val, len);
-	ring->coalesced_mmio[ring->last].pio = dev->zone.pio;
+	last = ring->last % KVM_COALESCED_MMIO_MAX;
+	ring->coalesced_mmio[last].phys_addr = addr;
+	ring->coalesced_mmio[last].len = len;
+	memcpy(ring->coalesced_mmio[last].data, val, len);
+	ring->coalesced_mmio[last].pio = dev->zone.pio;
 	smp_wmb();
-	ring->last = (ring->last + 1) % KVM_COALESCED_MMIO_MAX;
+	ring->last = (last + 1) % KVM_COALESCED_MMIO_MAX;
 	spin_unlock(&dev->kvm->ring_lock);
 	return 0;
 }