Two fixes for what are effectively the same bug. The binary search used
for memslot lookup doesn't check the resolved index and can access memory
beyond the end of the memslot array.
I split the s390 specific change to a separate patch because it's subtly
different, and to simplify backporting. The KVM wide fix can be applied
to stable trees as is, but AFAICT the s390 change would need to be paired
with the !used_slots check from commit 774a964ef56 ("KVM: Fix out of range
accesses to memslots"). This is why I tagged only the KVM wide patch for
stable.
Sean Christopherson (2):
KVM: Check validity of resolved slot when searching memslots
KVM: s390: Return last valid slot if approx index is out-of-bounds
arch/s390/kvm/kvm-s390.c | 3 +++
include/linux/kvm_host.h | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
--
2.24.1
Check that the resolved slot (somewhat confusingly named 'start') is a
valid/allocated slot before doing the final comparison to see if the
specified gfn resides in the associated slot. The resolved slot can be
invalid if the binary search loop terminated because the search index
was incremented beyond the number of used slots.
This bug has existed since the binary search algorithm was introduced,
but went unnoticed because KVM statically allocated memory for the max
number of slots, i.e. the access would only be truly out-of-bounds if
all possible slots were allocated and the specified gfn was less than
the base of the lowest memslot. Commit 36947254e5f98 ("KVM: Dynamically
size memslot array based on number of used slots") eliminated the "all
possible slots allocated" condition and made the bug embarrasingly easy
to hit.
Fixes: 9c1a5d38780e6 ("kvm: optimize GFN to memslot lookup with large slots amount")
Reported-by: [email protected]
Cc: [email protected]
Signed-off-by: Sean Christopherson <[email protected]>
---
include/linux/kvm_host.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 6d58beb65454..01276e3d01b9 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1048,7 +1048,7 @@ search_memslots(struct kvm_memslots *slots, gfn_t gfn)
start = slot + 1;
}
- if (gfn >= memslots[start].base_gfn &&
+ if (start < slots->used_slots && gfn >= memslots[start].base_gfn &&
gfn < memslots[start].base_gfn + memslots[start].npages) {
atomic_set(&slots->lru_slot, start);
return &memslots[start];
--
2.24.1
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 <[email protected]>
---
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);
--
2.24.1
On 08.04.20 08:40, Sean Christopherson wrote:
> Two fixes for what are effectively the same bug. The binary search used
> for memslot lookup doesn't check the resolved index and can access memory
> beyond the end of the memslot array.
>
> I split the s390 specific change to a separate patch because it's subtly
> different, and to simplify backporting. The KVM wide fix can be applied
> to stable trees as is, but AFAICT the s390 change would need to be paired
> with the !used_slots check from commit 774a964ef56 ("KVM: Fix out of range
> accesses to memslots"). This is why I tagged only the KVM wide patch for
> stable.
You can specify dependencies like
see
Documentation/process/stable-kernel-rules.rst
-----------snip---------------
Additionally, some patches submitted via :ref:`option_1` may have additional
patch prerequisites which can be cherry-picked. This can be specified in the
following format in the sign-off area:
.. code-block:: none
Cc: <[email protected]> # 3.3.x: a1f84a3: sched: Check for idle
Cc: <[email protected]> # 3.3.x: 1b9508f: sched: Rate-limit newidle
Cc: <[email protected]> # 3.3.x: fd21073: sched: Fix affinity logic
Cc: <[email protected]> # 3.3.x
Signed-off-by: Ingo Molnar <[email protected]>
The tag sequence has the meaning of:
.. code-block:: none
git cherry-pick a1f84a3
git cherry-pick 1b9508f
git cherry-pick fd21073
git cherry-pick <this commit>
-----------snip---------------
On Tue, 7 Apr 2020 23:40:58 -0700
Sean Christopherson <[email protected]> wrote:
> Check that the resolved slot (somewhat confusingly named 'start') is a
> valid/allocated slot before doing the final comparison to see if the
> specified gfn resides in the associated slot. The resolved slot can be
> invalid if the binary search loop terminated because the search index
> was incremented beyond the number of used slots.
>
> This bug has existed since the binary search algorithm was introduced,
> but went unnoticed because KVM statically allocated memory for the max
> number of slots, i.e. the access would only be truly out-of-bounds if
> all possible slots were allocated and the specified gfn was less than
> the base of the lowest memslot. Commit 36947254e5f98 ("KVM: Dynamically
> size memslot array based on number of used slots") eliminated the "all
> possible slots allocated" condition and made the bug embarrasingly easy
> to hit.
>
> Fixes: 9c1a5d38780e6 ("kvm: optimize GFN to memslot lookup with large slots amount")
> Reported-by: [email protected]
> Cc: [email protected]
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> include/linux/kvm_host.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 6d58beb65454..01276e3d01b9 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1048,7 +1048,7 @@ search_memslots(struct kvm_memslots *slots, gfn_t gfn)
> start = slot + 1;
> }
>
> - if (gfn >= memslots[start].base_gfn &&
> + if (start < slots->used_slots && gfn >= memslots[start].base_gfn &&
> gfn < memslots[start].base_gfn + memslots[start].npages) {
> atomic_set(&slots->lru_slot, start);
> return &memslots[start];
Reviewed-by: Cornelia Huck <[email protected]>
On Tue, 7 Apr 2020 23:40:59 -0700
Sean Christopherson <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
On 08.04.20 08:40, Sean Christopherson wrote:
> Two fixes for what are effectively the same bug. The binary search used
> for memslot lookup doesn't check the resolved index and can access memory
> beyond the end of the memslot array.
>
> I split the s390 specific change to a separate patch because it's subtly
> different, and to simplify backporting. The KVM wide fix can be applied
> to stable trees as is, but AFAICT the s390 change would need to be paired
> with the !used_slots check from commit 774a964ef56 ("KVM: Fix out of range
I cannot find the commit id 774a964ef56
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 <[email protected]>
> ---
> 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 Wed, 8 Apr 2020 09:24:27 +0200
Christian Borntraeger <[email protected]> wrote:
> On 08.04.20 08:40, Sean Christopherson wrote:
> > Two fixes for what are effectively the same bug. The binary search used
> > for memslot lookup doesn't check the resolved index and can access memory
> > beyond the end of the memslot array.
> >
> > I split the s390 specific change to a separate patch because it's subtly
> > different, and to simplify backporting. The KVM wide fix can be applied
> > to stable trees as is, but AFAICT the s390 change would need to be paired
> > with the !used_slots check from commit 774a964ef56 ("KVM: Fix out of range
>
> I cannot find the commit id 774a964ef56
>
It's 0774a964ef561b7170d8d1b1bfe6f88002b6d219 in my tree.
On 08/04/20 09:10, Cornelia Huck wrote:
> On Tue, 7 Apr 2020 23:40:59 -0700
> Sean Christopherson <[email protected]> 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: [email protected] # 4.19.x: 0774a964ef56: KVM: Fix out of range accesses to memslots
Cc: [email protected] # 4.19.x
Paolo
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 <[email protected]>
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 <[email protected]>
>
> 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.
On Tue, 7 Apr 2020 23:40:59 -0700
Sean Christopherson <[email protected]> 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
> <[email protected]> ---
> 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 <[email protected]>
On Wed, Apr 08, 2020 at 10:10:04AM +0200, Cornelia Huck wrote:
> On Wed, 8 Apr 2020 09:24:27 +0200
> Christian Borntraeger <[email protected]> wrote:
>
> > On 08.04.20 08:40, Sean Christopherson wrote:
> > > Two fixes for what are effectively the same bug. The binary search used
> > > for memslot lookup doesn't check the resolved index and can access memory
> > > beyond the end of the memslot array.
> > >
> > > I split the s390 specific change to a separate patch because it's subtly
> > > different, and to simplify backporting. The KVM wide fix can be applied
> > > to stable trees as is, but AFAICT the s390 change would need to be paired
> > > with the !used_slots check from commit 774a964ef56 ("KVM: Fix out of range
> >
> > I cannot find the commit id 774a964ef56
> >
>
> It's 0774a964ef561b7170d8d1b1bfe6f88002b6d219 in my tree.
Argh, I botched the copy. Thanks for hunting it down!