| Message ID | 20200408064059.8957-3-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Fix out-of-bounds memslot access | expand |
On Tue, 7 Apr 2020 23:40:59 -0700 Sean Christopherson <sean.j.christopherson@intel.com> wrote: > Return the index of the last valid slot from gfn_to_memslot_approx() if > its binary search loop yielded an out-of-bounds index. The index can > be out-of-bounds if the specified gfn is less than the base of the > lowest memslot (which is also the last valid memslot). > > Note, the sole caller, kvm_s390_get_cmma(), ensures used_slots is > non-zero. > This also should be cc:stable, with the dependency expressed as mentioned by Christian. > Fixes: afdad61615cc3 ("KVM: s390: Fix storage attributes migration with memory slots") > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/s390/kvm/kvm-s390.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 19a81024fe16..5dcf9ff12828 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -1939,6 +1939,9 @@ static int gfn_to_memslot_approx(struct kvm_memslots *slots, gfn_t gfn) > start = slot + 1; > } > > + if (start >= slots->used_slots) > + return slots->used_slots - 1; > + > if (gfn >= memslots[start].base_gfn && > gfn < memslots[start].base_gfn + memslots[start].npages) { > atomic_set(&slots->lru_slot, start); Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On 08.04.20 08:40, Sean Christopherson wrote: > Return the index of the last valid slot from gfn_to_memslot_approx() if > its binary search loop yielded an out-of-bounds index. The index can > be out-of-bounds if the specified gfn is less than the base of the > lowest memslot (which is also the last valid memslot). > > Note, the sole caller, kvm_s390_get_cmma(), ensures used_slots is > non-zero. > > Fixes: afdad61615cc3 ("KVM: s390: Fix storage attributes migration with memory slots") > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/s390/kvm/kvm-s390.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 19a81024fe16..5dcf9ff12828 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -1939,6 +1939,9 @@ static int gfn_to_memslot_approx(struct kvm_memslots *slots, gfn_t gfn) > start = slot + 1; > } > > + if (start >= slots->used_slots) > + return slots->used_slots - 1; > + > if (gfn >= memslots[start].base_gfn && > gfn < memslots[start].base_gfn + memslots[start].npages) { > atomic_set(&slots->lru_slot, start); > Claudio, can you have a look?
On 08/04/20 09:10, Cornelia Huck wrote: > On Tue, 7 Apr 2020 23:40:59 -0700 > Sean Christopherson <sean.j.christopherson@intel.com> wrote: > >> Return the index of the last valid slot from gfn_to_memslot_approx() if >> its binary search loop yielded an out-of-bounds index. The index can >> be out-of-bounds if the specified gfn is less than the base of the >> lowest memslot (which is also the last valid memslot). >> >> Note, the sole caller, kvm_s390_get_cmma(), ensures used_slots is >> non-zero. >> > This also should be cc:stable, with the dependency expressed as > mentioned by Christian. > So, Cc: stable@vger.kernel.org # 4.19.x: 0774a964ef56: KVM: Fix out of range accesses to memslots Cc: stable@vger.kernel.org # 4.19.x Paolo
On Tue, 7 Apr 2020 23:40:59 -0700 Sean Christopherson <sean.j.christopherson@intel.com> wrote: > Return the index of the last valid slot from gfn_to_memslot_approx() > if its binary search loop yielded an out-of-bounds index. The index > can be out-of-bounds if the specified gfn is less than the base of the > lowest memslot (which is also the last valid memslot). > > Note, the sole caller, kvm_s390_get_cmma(), ensures used_slots is > non-zero. > > Fixes: afdad61615cc3 ("KVM: s390: Fix storage attributes migration > with memory slots") Signed-off-by: Sean Christopherson > <sean.j.christopherson@intel.com> --- > arch/s390/kvm/kvm-s390.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 19a81024fe16..5dcf9ff12828 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -1939,6 +1939,9 @@ static int gfn_to_memslot_approx(struct > kvm_memslots *slots, gfn_t gfn) start = slot + 1; > } > > + if (start >= slots->used_slots) > + return slots->used_slots - 1; > + > if (gfn >= memslots[start].base_gfn && > gfn < memslots[start].base_gfn + memslots[start].npages) > { atomic_set(&slots->lru_slot, start); on s390 memory always starts at 0; you can't even boot a system missing the first pages of physical memory, so this means this situation would never happen in practice. of course, a malicious userspace program could create an (unbootable) VM and trigger this bug, so the patch itself makes sense. Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
On 08/04/20 12:21, Claudio Imbrenda wrote: > on s390 memory always starts at 0; you can't even boot a system missing > the first pages of physical memory, so this means this situation would > never happen in practice. > > of course, a malicious userspace program could create an (unbootable) VM > and trigger this bug, so the patch itself makes sense. > > Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> What about using KVM just for isolation and not just to run a full-blown OS (that is, you might even only have the guest run in problem state)? Would that be feasible on s390? Thanks, Paolo
On 08.04.20 13:33, Paolo Bonzini wrote: > On 08/04/20 12:21, Claudio Imbrenda wrote: >> on s390 memory always starts at 0; you can't even boot a system missing >> the first pages of physical memory, so this means this situation would >> never happen in practice. >> >> of course, a malicious userspace program could create an (unbootable) VM >> and trigger this bug, so the patch itself makes sense. >> >> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > > What about using KVM just for isolation and not just to run a full-blown > OS (that is, you might even only have the guest run in problem state)? > Would that be feasible on s390? You always need 2 prefix pages. Otherwise the SIE instruction refuses to start. By default this starts as address 0. You might be also to set the prefix register to something else adn then this could maybe work.
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index 19a81024fe16..5dcf9ff12828 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -1939,6 +1939,9 @@ static int gfn_to_memslot_approx(struct kvm_memslots *slots, gfn_t gfn) start = slot + 1; } + if (start >= slots->used_slots) + return slots->used_slots - 1; + if (gfn >= memslots[start].base_gfn && gfn < memslots[start].base_gfn + memslots[start].npages) { atomic_set(&slots->lru_slot, start);
Return the index of the last valid slot from gfn_to_memslot_approx() if its binary search loop yielded an out-of-bounds index. The index can be out-of-bounds if the specified gfn is less than the base of the lowest memslot (which is also the last valid memslot). Note, the sole caller, kvm_s390_get_cmma(), ensures used_slots is non-zero. Fixes: afdad61615cc3 ("KVM: s390: Fix storage attributes migration with memory slots") Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/s390/kvm/kvm-s390.c | 3 +++ 1 file changed, 3 insertions(+)