Re: [PATCH] KVM: VMX: Don't use vcpu->run->internal.ndata as an array index

[Reiji Watanabe <reijiw@google.com>]

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;
> >       }
> >
> >
>