| Message ID | 20190924074726.14770-1-juergh@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,Disco,CVE-2019-14821] KVM: coalesced_mmio: add bounds checking | expand |
On 9/24/19 12:47 AM, Juerg Haefliger wrote: > From: Matt Delco <delco@chromium.org> > > The first/last indexes are typically shared with a user app. > The app can change the 'last' index that the kernel uses > to store the next result. This change sanity checks the index > before using it for writing to a potentially arbitrary address. > > This fixes CVE-2019-14821. > > Cc: stable@vger.kernel.org > Fixes: 5f94c1741bdc ("KVM: Add coalesced MMIO support (common part)") > Signed-off-by: Matt Delco <delco@chromium.org> > Signed-off-by: Jim Mattson <jmattson@google.com> > Reported-by: syzbot+983c866c3dd6efa3662a@syzkaller.appspotmail.com > [Use READ_ONCE. - Paolo] > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > CVE-2019-14821 > > (cherry picked from commit b60fe990c6b07ef6d4df67bc0530c7c90a62623a) > Signed-off-by: Juerg Haefliger <juergh@canonical.com> Acked-by: Connor Kuehl <connor.kuehl@canonical.com> > --- > virt/kvm/coalesced_mmio.c | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c > index 6855cce3e528..f52ae1e52591 100644 > --- a/virt/kvm/coalesced_mmio.c > +++ b/virt/kvm/coalesced_mmio.c > @@ -40,7 +40,7 @@ static int coalesced_mmio_in_range(struct kvm_coalesced_mmio_dev *dev, > return 1; > } > > -static int coalesced_mmio_has_room(struct kvm_coalesced_mmio_dev *dev) > +static int coalesced_mmio_has_room(struct kvm_coalesced_mmio_dev *dev, u32 last) > { > struct kvm_coalesced_mmio_ring *ring; > unsigned avail; > @@ -52,7 +52,7 @@ static int coalesced_mmio_has_room(struct kvm_coalesced_mmio_dev *dev) > * there is always one unused entry in the buffer > */ > ring = dev->kvm->coalesced_mmio_ring; > - avail = (ring->first - ring->last - 1) % KVM_COALESCED_MMIO_MAX; > + avail = (ring->first - last - 1) % KVM_COALESCED_MMIO_MAX; > if (avail == 0) { > /* full */ > return 0; > @@ -67,25 +67,28 @@ 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 insert; > > if (!coalesced_mmio_in_range(dev, addr, len)) > return -EOPNOTSUPP; > > spin_lock(&dev->kvm->ring_lock); > > - if (!coalesced_mmio_has_room(dev)) { > + insert = READ_ONCE(ring->last); > + if (!coalesced_mmio_has_room(dev, insert) || > + insert >= KVM_COALESCED_MMIO_MAX) { > spin_unlock(&dev->kvm->ring_lock); > return -EOPNOTSUPP; > } > > /* 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; > + ring->coalesced_mmio[insert].phys_addr = addr; > + ring->coalesced_mmio[insert].len = len; > + memcpy(ring->coalesced_mmio[insert].data, val, len); > + ring->coalesced_mmio[insert].pio = dev->zone.pio; > smp_wmb(); > - ring->last = (ring->last + 1) % KVM_COALESCED_MMIO_MAX; > + ring->last = (insert + 1) % KVM_COALESCED_MMIO_MAX; > spin_unlock(&dev->kvm->ring_lock); > return 0; > } >
On 2019-09-24 09:47:26, Juerg Haefliger wrote: > From: Matt Delco <delco@chromium.org> > > The first/last indexes are typically shared with a user app. > The app can change the 'last' index that the kernel uses > to store the next result. This change sanity checks the index > before using it for writing to a potentially arbitrary address. > > This fixes CVE-2019-14821. > > Cc: stable@vger.kernel.org > Fixes: 5f94c1741bdc ("KVM: Add coalesced MMIO support (common part)") > Signed-off-by: Matt Delco <delco@chromium.org> > Signed-off-by: Jim Mattson <jmattson@google.com> > Reported-by: syzbot+983c866c3dd6efa3662a@syzkaller.appspotmail.com > [Use READ_ONCE. - Paolo] > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > CVE-2019-14821 > > (cherry picked from commit b60fe990c6b07ef6d4df67bc0530c7c90a62623a) > Signed-off-by: Juerg Haefliger <juergh@canonical.com> Acked-by: Tyler Hicks <tyhicks@canonical.com> Thanks! Tyler > --- > virt/kvm/coalesced_mmio.c | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c > index 6855cce3e528..f52ae1e52591 100644 > --- a/virt/kvm/coalesced_mmio.c > +++ b/virt/kvm/coalesced_mmio.c > @@ -40,7 +40,7 @@ static int coalesced_mmio_in_range(struct kvm_coalesced_mmio_dev *dev, > return 1; > } > > -static int coalesced_mmio_has_room(struct kvm_coalesced_mmio_dev *dev) > +static int coalesced_mmio_has_room(struct kvm_coalesced_mmio_dev *dev, u32 last) > { > struct kvm_coalesced_mmio_ring *ring; > unsigned avail; > @@ -52,7 +52,7 @@ static int coalesced_mmio_has_room(struct kvm_coalesced_mmio_dev *dev) > * there is always one unused entry in the buffer > */ > ring = dev->kvm->coalesced_mmio_ring; > - avail = (ring->first - ring->last - 1) % KVM_COALESCED_MMIO_MAX; > + avail = (ring->first - last - 1) % KVM_COALESCED_MMIO_MAX; > if (avail == 0) { > /* full */ > return 0; > @@ -67,25 +67,28 @@ 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 insert; > > if (!coalesced_mmio_in_range(dev, addr, len)) > return -EOPNOTSUPP; > > spin_lock(&dev->kvm->ring_lock); > > - if (!coalesced_mmio_has_room(dev)) { > + insert = READ_ONCE(ring->last); > + if (!coalesced_mmio_has_room(dev, insert) || > + insert >= KVM_COALESCED_MMIO_MAX) { > spin_unlock(&dev->kvm->ring_lock); > return -EOPNOTSUPP; > } > > /* 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; > + ring->coalesced_mmio[insert].phys_addr = addr; > + ring->coalesced_mmio[insert].len = len; > + memcpy(ring->coalesced_mmio[insert].data, val, len); > + ring->coalesced_mmio[insert].pio = dev->zone.pio; > smp_wmb(); > - ring->last = (ring->last + 1) % KVM_COALESCED_MMIO_MAX; > + ring->last = (insert + 1) % KVM_COALESCED_MMIO_MAX; > spin_unlock(&dev->kvm->ring_lock); > return 0; > } > -- > 2.20.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 2019-09-24 09:47:26 , Juerg Haefliger wrote: > From: Matt Delco <delco@chromium.org> > > The first/last indexes are typically shared with a user app. > The app can change the 'last' index that the kernel uses > to store the next result. This change sanity checks the index > before using it for writing to a potentially arbitrary address. > > This fixes CVE-2019-14821. > > Cc: stable@vger.kernel.org > Fixes: 5f94c1741bdc ("KVM: Add coalesced MMIO support (common part)") > Signed-off-by: Matt Delco <delco@chromium.org> > Signed-off-by: Jim Mattson <jmattson@google.com> > Reported-by: syzbot+983c866c3dd6efa3662a@syzkaller.appspotmail.com > [Use READ_ONCE. - Paolo] > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > CVE-2019-14821 > > (cherry picked from commit b60fe990c6b07ef6d4df67bc0530c7c90a62623a) > Signed-off-by: Juerg Haefliger <juergh@canonical.com> > --- > virt/kvm/coalesced_mmio.c | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c > index 6855cce3e528..f52ae1e52591 100644 > --- a/virt/kvm/coalesced_mmio.c > +++ b/virt/kvm/coalesced_mmio.c > @@ -40,7 +40,7 @@ static int coalesced_mmio_in_range(struct kvm_coalesced_mmio_dev *dev, > return 1; > } > > -static int coalesced_mmio_has_room(struct kvm_coalesced_mmio_dev *dev) > +static int coalesced_mmio_has_room(struct kvm_coalesced_mmio_dev *dev, u32 last) > { > struct kvm_coalesced_mmio_ring *ring; > unsigned avail; > @@ -52,7 +52,7 @@ static int coalesced_mmio_has_room(struct kvm_coalesced_mmio_dev *dev) > * there is always one unused entry in the buffer > */ > ring = dev->kvm->coalesced_mmio_ring; > - avail = (ring->first - ring->last - 1) % KVM_COALESCED_MMIO_MAX; > + avail = (ring->first - last - 1) % KVM_COALESCED_MMIO_MAX; > if (avail == 0) { > /* full */ > return 0; > @@ -67,25 +67,28 @@ 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 insert; > > if (!coalesced_mmio_in_range(dev, addr, len)) > return -EOPNOTSUPP; > > spin_lock(&dev->kvm->ring_lock); > > - if (!coalesced_mmio_has_room(dev)) { > + insert = READ_ONCE(ring->last); > + if (!coalesced_mmio_has_room(dev, insert) || > + insert >= KVM_COALESCED_MMIO_MAX) { > spin_unlock(&dev->kvm->ring_lock); > return -EOPNOTSUPP; > } > > /* 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; > + ring->coalesced_mmio[insert].phys_addr = addr; > + ring->coalesced_mmio[insert].len = len; > + memcpy(ring->coalesced_mmio[insert].data, val, len); > + ring->coalesced_mmio[insert].pio = dev->zone.pio; > smp_wmb(); > - ring->last = (ring->last + 1) % KVM_COALESCED_MMIO_MAX; > + ring->last = (insert + 1) % KVM_COALESCED_MMIO_MAX; > spin_unlock(&dev->kvm->ring_lock); > return 0; > } > -- > 2.20.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c index 6855cce3e528..f52ae1e52591 100644 --- a/virt/kvm/coalesced_mmio.c +++ b/virt/kvm/coalesced_mmio.c @@ -40,7 +40,7 @@ static int coalesced_mmio_in_range(struct kvm_coalesced_mmio_dev *dev, return 1; } -static int coalesced_mmio_has_room(struct kvm_coalesced_mmio_dev *dev) +static int coalesced_mmio_has_room(struct kvm_coalesced_mmio_dev *dev, u32 last) { struct kvm_coalesced_mmio_ring *ring; unsigned avail; @@ -52,7 +52,7 @@ static int coalesced_mmio_has_room(struct kvm_coalesced_mmio_dev *dev) * there is always one unused entry in the buffer */ ring = dev->kvm->coalesced_mmio_ring; - avail = (ring->first - ring->last - 1) % KVM_COALESCED_MMIO_MAX; + avail = (ring->first - last - 1) % KVM_COALESCED_MMIO_MAX; if (avail == 0) { /* full */ return 0; @@ -67,25 +67,28 @@ 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 insert; if (!coalesced_mmio_in_range(dev, addr, len)) return -EOPNOTSUPP; spin_lock(&dev->kvm->ring_lock); - if (!coalesced_mmio_has_room(dev)) { + insert = READ_ONCE(ring->last); + if (!coalesced_mmio_has_room(dev, insert) || + insert >= KVM_COALESCED_MMIO_MAX) { spin_unlock(&dev->kvm->ring_lock); return -EOPNOTSUPP; } /* 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; + ring->coalesced_mmio[insert].phys_addr = addr; + ring->coalesced_mmio[insert].len = len; + memcpy(ring->coalesced_mmio[insert].data, val, len); + ring->coalesced_mmio[insert].pio = dev->zone.pio; smp_wmb(); - ring->last = (ring->last + 1) % KVM_COALESCED_MMIO_MAX; + ring->last = (insert + 1) % KVM_COALESCED_MMIO_MAX; spin_unlock(&dev->kvm->ring_lock); return 0; }