* [PATCH] KVM: VMX: Don't use vcpu->run->internal.ndata as an array index
@ 2021-04-13 15:47 Reiji Watanabe
2021-04-13 15:51 ` Paolo Bonzini
0 siblings, 1 reply; 3+ messages in thread
From: Reiji Watanabe @ 2021-04-13 15:47 UTC (permalink / raw)
To: kvm, Paolo Bonzini; +Cc: Reiji Watanabe, Jim Mattson
__vmx_handle_exit() uses vcpu->run->internal.ndata as an index for
an array access. Since vcpu->run is (can be) mapped to a user address
space with a writer permission, the 'ndata' could be updated by the
user process at anytime (the user process can set it to outside the
bounds of the array).
So, it is not safe that __vmx_handle_exit() uses the 'ndata' that way.
Fixes: 1aa561b1a4c0 ("kvm: x86: Add "last CPU" to some KVM_EXIT information")
Signed-off-by: Reiji Watanabe <reijiw@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
---
arch/x86/kvm/vmx/vmx.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 32cf8287d4a7..29b40e092d13 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6027,19 +6027,19 @@ static int __vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
exit_reason.basic != EXIT_REASON_PML_FULL &&
exit_reason.basic != EXIT_REASON_APIC_ACCESS &&
exit_reason.basic != EXIT_REASON_TASK_SWITCH)) {
+ int ndata = 3;
+
vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_DELIVERY_EV;
- vcpu->run->internal.ndata = 3;
vcpu->run->internal.data[0] = vectoring_info;
vcpu->run->internal.data[1] = exit_reason.full;
vcpu->run->internal.data[2] = vcpu->arch.exit_qualification;
if (exit_reason.basic == EXIT_REASON_EPT_MISCONFIG) {
- vcpu->run->internal.ndata++;
- vcpu->run->internal.data[3] =
+ vcpu->run->internal.data[ndata++] =
vmcs_read64(GUEST_PHYSICAL_ADDRESS);
}
- vcpu->run->internal.data[vcpu->run->internal.ndata++] =
- vcpu->arch.last_vmentry_cpu;
+ vcpu->run->internal.data[ndata++] = vcpu->arch.last_vmentry_cpu;
+ vcpu->run->internal.ndata = ndata;
return 0;
}
--
2.31.1.295.g9ea45b61b8-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: VMX: Don't use vcpu->run->internal.ndata as an array index
2021-04-13 15:47 [PATCH] KVM: VMX: Don't use vcpu->run->internal.ndata as an array index Reiji Watanabe
@ 2021-04-13 15:51 ` Paolo Bonzini
2021-04-13 16:28 ` Reiji Watanabe
0 siblings, 1 reply; 3+ messages in thread
From: Paolo Bonzini @ 2021-04-13 15:51 UTC (permalink / raw)
To: Reiji Watanabe, kvm; +Cc: Jim Mattson
On 13/04/21 17:47, Reiji Watanabe wrote:
> __vmx_handle_exit() uses vcpu->run->internal.ndata as an index for
> an array access. Since vcpu->run is (can be) mapped to a user address
> space with a writer permission, the 'ndata' could be updated by the
> user process at anytime (the user process can set it to outside the
> bounds of the array).
> So, it is not safe that __vmx_handle_exit() uses the 'ndata' that way.
>
> Fixes: 1aa561b1a4c0 ("kvm: x86: Add "last CPU" to some KVM_EXIT information")
> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> Reviewed-by: Jim Mattson <jmattson@google.com>
> ---
> arch/x86/kvm/vmx/vmx.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
Ouch. In theory it's an internal error, but we've seen it happen on
problematic hardware. Should we consider it a security issue?
Paolo
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 32cf8287d4a7..29b40e092d13 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6027,19 +6027,19 @@ static int __vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
> exit_reason.basic != EXIT_REASON_PML_FULL &&
> exit_reason.basic != EXIT_REASON_APIC_ACCESS &&
> exit_reason.basic != EXIT_REASON_TASK_SWITCH)) {
> + int ndata = 3;
> +
> vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_DELIVERY_EV;
> - vcpu->run->internal.ndata = 3;
> vcpu->run->internal.data[0] = vectoring_info;
> vcpu->run->internal.data[1] = exit_reason.full;
> vcpu->run->internal.data[2] = vcpu->arch.exit_qualification;
> if (exit_reason.basic == EXIT_REASON_EPT_MISCONFIG) {
> - vcpu->run->internal.ndata++;
> - vcpu->run->internal.data[3] =
> + vcpu->run->internal.data[ndata++] =
> vmcs_read64(GUEST_PHYSICAL_ADDRESS);
> }
> - vcpu->run->internal.data[vcpu->run->internal.ndata++] =
> - vcpu->arch.last_vmentry_cpu;
> + vcpu->run->internal.data[ndata++] = vcpu->arch.last_vmentry_cpu;
> + vcpu->run->internal.ndata = ndata;
> return 0;
> }
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: VMX: Don't use vcpu->run->internal.ndata as an array index
2021-04-13 15:51 ` Paolo Bonzini
@ 2021-04-13 16:28 ` Reiji Watanabe
0 siblings, 0 replies; 3+ messages in thread
From: Reiji Watanabe @ 2021-04-13 16:28 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kvm, Jim Mattson
On Tue, Apr 13, 2021 at 8:51 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 13/04/21 17:47, Reiji Watanabe wrote:
> > __vmx_handle_exit() uses vcpu->run->internal.ndata as an index for
> > an array access. Since vcpu->run is (can be) mapped to a user address
> > space with a writer permission, the 'ndata' could be updated by the
> > user process at anytime (the user process can set it to outside the
> > bounds of the array).
> > So, it is not safe that __vmx_handle_exit() uses the 'ndata' that way.
> >
> > Fixes: 1aa561b1a4c0 ("kvm: x86: Add "last CPU" to some KVM_EXIT information")
> > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> > Reviewed-by: Jim Mattson <jmattson@google.com>
> > ---
> > arch/x86/kvm/vmx/vmx.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
>
> Ouch. In theory it's an internal error, but we've seen it happen on
> problematic hardware. Should we consider it a security issue?
>
> Paolo
A user application could intentionally create the case (with a
simple guest). So, I would think this is a security issue.
Thanks,
Reiji
>
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 32cf8287d4a7..29b40e092d13 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -6027,19 +6027,19 @@ static int __vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
> > exit_reason.basic != EXIT_REASON_PML_FULL &&
> > exit_reason.basic != EXIT_REASON_APIC_ACCESS &&
> > exit_reason.basic != EXIT_REASON_TASK_SWITCH)) {
> > + int ndata = 3;
> > +
> > vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> > vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_DELIVERY_EV;
> > - vcpu->run->internal.ndata = 3;
> > vcpu->run->internal.data[0] = vectoring_info;
> > vcpu->run->internal.data[1] = exit_reason.full;
> > vcpu->run->internal.data[2] = vcpu->arch.exit_qualification;
> > if (exit_reason.basic == EXIT_REASON_EPT_MISCONFIG) {
> > - vcpu->run->internal.ndata++;
> > - vcpu->run->internal.data[3] =
> > + vcpu->run->internal.data[ndata++] =
> > vmcs_read64(GUEST_PHYSICAL_ADDRESS);
> > }
> > - vcpu->run->internal.data[vcpu->run->internal.ndata++] =
> > - vcpu->arch.last_vmentry_cpu;
> > + vcpu->run->internal.data[ndata++] = vcpu->arch.last_vmentry_cpu;
> > + vcpu->run->internal.ndata = ndata;
> > return 0;
> > }
> >
> >
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-04-13 16:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-13 15:47 [PATCH] KVM: VMX: Don't use vcpu->run->internal.ndata as an array index Reiji Watanabe
2021-04-13 15:51 ` Paolo Bonzini
2021-04-13 16:28 ` Reiji Watanabe
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.