On Thu, Aug 15, 2019 at 06:36:44AM +0000, Nicusor CITU wrote:
> > > + void (*msr_intercept)(struct kvm_vcpu *vcpu, unsigned int msr,
> > > + bool enable);
> >
> > This should be toggle_wrmsr_intercept(), or toggle_msr_intercept()
> > with a paramter to control RDMSR vs. WRMSR.
>
> Ok, I can do that.
>
>
> > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > > index 6450c8c44771..0306c7ef3158 100644
> > > --- a/arch/x86/kvm/vmx/vmx.c
> > > +++ b/arch/x86/kvm/vmx/vmx.c
> > > @@ -7784,6 +7784,15 @@ static __exit void hardware_unsetup(void)
> > > free_kvm_area();
> > > }
> > >
> > > +static void vmx_msr_intercept(struct kvm_vcpu *vcpu, unsigned int
> > > msr,
> > > + bool enable)
> > > +{
> > > + struct vcpu_vmx *vmx = to_vmx(vcpu);
> > > + unsigned long *msr_bitmap = vmx->vmcs01.msr_bitmap;
Is KVMI intended to play nice with nested virtualization? Unconditionally
updating vmcs01.msr_bitmap is correct regardless of whether the vCPU is in
L1 or L2, but if the vCPU is currently in L2 then the effective bitmap,
i.e. vmcs02.msr_bitmap, won't be updated until the next nested VM-Enter.
> > > +
> > > + vmx_set_intercept_for_msr(msr_bitmap, msr, MSR_TYPE_W, enable);
> > > +}
> >
> > Unless I overlooked a check, this will allow userspace to disable
> > WRMSR interception for any MSR in the above range, i.e. userspace can
> > use KVM to gain full write access to pretty much all the interesting
> > MSRs. This needs to only disable interception if KVM had interception
> > disabled before introspection started modifying state.
>
> We only need to enable the MSR interception. We never disable it -
> please see kvmi_arch_cmd_control_msr().
In that case, drop @enable and use enable_wrmsr_intercept() or something
along those lines for kvm_x86_ops instead of toggle_wrmsr_intercept().
