On Tue, Apr 13, 2021 at 8:51 AM Paolo Bonzini <pbonzini@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Reviewed-by: Jim Mattson <jmattson@xxxxxxxxxx>
> > ---
> > 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;
> > }
> >
> >
>
