On Thu, Aug 15, 2019 at 06:36:44AM +0000, Nicusor CITU wrote:

quoted

quoted

+	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.

quoted

quoted

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.

quoted

quoted

+
+	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().