[PATCH] kvm: Ensure writes to the coalesced MMIO ring are within bounds

[PATCH] kvm: Ensure writes to the coalesced MMIO ring are within bounds

[Will Deacon]

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;
 }
-- 
2.23.0.351.gc4317032e6-goog

Re: [PATCH] kvm: Ensure writes to the coalesced MMIO ring are within bounds

[Greg Kroah-Hartman]

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>

Re: [PATCH] kvm: Ensure writes to the coalesced MMIO ring are within bounds

[Paolo Bonzini]

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;
>  }
> 

Re: [PATCH] kvm: Ensure writes to the coalesced MMIO ring are within bounds

[Will Deacon]

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

Re: [PATCH] kvm: Ensure writes to the coalesced MMIO ring are within bounds

[Paolo Bonzini]

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
>