Messages in this thread | | | Subject | Re: [PATCH] kvm: Ensure writes to the coalesced MMIO ring are within bounds | From | Paolo Bonzini <> | Date | Wed, 18 Sep 2019 15:41:40 +0200 |
| |
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; > } >
| |