Message ID | 20210413154739.490299-1-reijiw@google.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: VMX: Don't use vcpu->run->internal.ndata as an array index | expand |
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; > } > >
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; > > } > > > > >
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; }