| Message ID | 20190829205635.20189-2-krish.sadhukhan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nVMX: Check GUEST_DEBUGCTL and GUEST_DR7 on vmentry of nested guests | expand |
On Thu, Aug 29, 2019 at 2:25 PM Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote: > > According to section "Checks on Guest Control Registers, Debug Registers, and > and MSRs" in Intel SDM vol 3C, the following checks are performed on vmentry > of nested guests: > > If the "load debug controls" VM-entry control is 1, bits reserved in the > IA32_DEBUGCTL MSR must be 0 in the field for that register. The first > processors to support the virtual-machine extensions supported only the > 1-setting of this control and thus performed this check unconditionally. > > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> > --- > arch/x86/kvm/vmx/nested.c | 4 ++++ > arch/x86/kvm/x86.h | 6 ++++++ > 2 files changed, 10 insertions(+) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index 46af3a5e9209..0b234e95e0ed 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -2677,6 +2677,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu, > !nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4)) > return -EINVAL; > > + if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS) && > + !kvm_debugctl_valid(vmcs12->guest_ia32_debugctl)) > + return -EINVAL; > + > if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) && > !kvm_pat_valid(vmcs12->guest_ia32_pat)) > return -EINVAL; > diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h > index a470ff0868c5..28ba6d0c359f 100644 > --- a/arch/x86/kvm/x86.h > +++ b/arch/x86/kvm/x86.h > @@ -354,6 +354,12 @@ static inline bool kvm_pat_valid(u64 data) > return (data | ((data & 0x0202020202020202ull) << 1)) == data; > } > > +static inline bool kvm_debugctl_valid(u64 data) > +{ > + /* Bits 2, 3, 4, 5, 13 and [31:16] are reserved */ > + return ((data & 0xFFFFFFFFFFFF203Cull) ? false : true); > +} This should actually be consistent with the constraints in kvm_set_msr_common: case MSR_IA32_DEBUGCTLMSR: if (!data) { /* We support the non-activated case already */ break; } else if (data & ~(DEBUGCTLMSR_LBR | DEBUGCTLMSR_BTF)) { /* Values other than LBR and BTF are vendor-specific, thus reserved and should throw a #GP */ return 1; } Also, as I said earlier... I'd rather see this built on an interface like: bool kvm_valid_msr_value(u32 msr_index, u64 value); Strange that we allow IA32_DEBUGCTL.BTF, since kvm_vcpu_do_singlestep ignores it. And vLBR still isn't a thing, is it? It's a bit scary to me that we allow any architecturally legal IA32_DEBUGCTL bits to be set today. There's probably a CVE in there somewhere.
On 08/29/2019 03:12 PM, Jim Mattson wrote: > On Thu, Aug 29, 2019 at 2:25 PM Krish Sadhukhan > <krish.sadhukhan@oracle.com> wrote: >> According to section "Checks on Guest Control Registers, Debug Registers, and >> and MSRs" in Intel SDM vol 3C, the following checks are performed on vmentry >> of nested guests: >> >> If the "load debug controls" VM-entry control is 1, bits reserved in the >> IA32_DEBUGCTL MSR must be 0 in the field for that register. The first >> processors to support the virtual-machine extensions supported only the >> 1-setting of this control and thus performed this check unconditionally. >> >> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> >> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> >> --- >> arch/x86/kvm/vmx/nested.c | 4 ++++ >> arch/x86/kvm/x86.h | 6 ++++++ >> 2 files changed, 10 insertions(+) >> >> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c >> index 46af3a5e9209..0b234e95e0ed 100644 >> --- a/arch/x86/kvm/vmx/nested.c >> +++ b/arch/x86/kvm/vmx/nested.c >> @@ -2677,6 +2677,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu, >> !nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4)) >> return -EINVAL; >> >> + if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS) && >> + !kvm_debugctl_valid(vmcs12->guest_ia32_debugctl)) >> + return -EINVAL; >> + >> if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) && >> !kvm_pat_valid(vmcs12->guest_ia32_pat)) >> return -EINVAL; >> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h >> index a470ff0868c5..28ba6d0c359f 100644 >> --- a/arch/x86/kvm/x86.h >> +++ b/arch/x86/kvm/x86.h >> @@ -354,6 +354,12 @@ static inline bool kvm_pat_valid(u64 data) >> return (data | ((data & 0x0202020202020202ull) << 1)) == data; >> } >> >> +static inline bool kvm_debugctl_valid(u64 data) >> +{ >> + /* Bits 2, 3, 4, 5, 13 and [31:16] are reserved */ >> + return ((data & 0xFFFFFFFFFFFF203Cull) ? false : true); >> +} > This should actually be consistent with the constraints in kvm_set_msr_common: > > case MSR_IA32_DEBUGCTLMSR: > if (!data) { > /* We support the non-activated case already */ > break; > } else if (data & ~(DEBUGCTLMSR_LBR | DEBUGCTLMSR_BTF)) { > /* Values other than LBR and BTF are vendor-specific, > thus reserved and should throw a #GP */ > return 1; > } > > Also, as I said earlier... > > I'd rather see this built on an interface like: > > bool kvm_valid_msr_value(u32 msr_index, u64 value); Yes, I forgot to do it. Will send a patch for this... > > Strange that we allow IA32_DEBUGCTL.BTF, since kvm_vcpu_do_singlestep > ignores it. And vLBR still isn't a thing, is it? Yes, DEBUGCTLMSR_LBR isn't used. Good catch ! > > It's a bit scary to me that we allow any architecturally legal > IA32_DEBUGCTL bits to be set today. There's probably a CVE in there > somewhere. Is it appropriate to disable those two bits as well, then ?
On Fri, Aug 30, 2019 at 4:26 PM Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote: > > > > On 08/29/2019 03:12 PM, Jim Mattson wrote: > > On Thu, Aug 29, 2019 at 2:25 PM Krish Sadhukhan > > <krish.sadhukhan@oracle.com> wrote: > >> According to section "Checks on Guest Control Registers, Debug Registers, and > >> and MSRs" in Intel SDM vol 3C, the following checks are performed on vmentry > >> of nested guests: > >> > >> If the "load debug controls" VM-entry control is 1, bits reserved in the > >> IA32_DEBUGCTL MSR must be 0 in the field for that register. The first > >> processors to support the virtual-machine extensions supported only the > >> 1-setting of this control and thus performed this check unconditionally. > >> > >> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > >> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> > >> --- > >> arch/x86/kvm/vmx/nested.c | 4 ++++ > >> arch/x86/kvm/x86.h | 6 ++++++ > >> 2 files changed, 10 insertions(+) > >> > >> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > >> index 46af3a5e9209..0b234e95e0ed 100644 > >> --- a/arch/x86/kvm/vmx/nested.c > >> +++ b/arch/x86/kvm/vmx/nested.c > >> @@ -2677,6 +2677,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu, > >> !nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4)) > >> return -EINVAL; > >> > >> + if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS) && > >> + !kvm_debugctl_valid(vmcs12->guest_ia32_debugctl)) > >> + return -EINVAL; > >> + > >> if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) && > >> !kvm_pat_valid(vmcs12->guest_ia32_pat)) > >> return -EINVAL; > >> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h > >> index a470ff0868c5..28ba6d0c359f 100644 > >> --- a/arch/x86/kvm/x86.h > >> +++ b/arch/x86/kvm/x86.h > >> @@ -354,6 +354,12 @@ static inline bool kvm_pat_valid(u64 data) > >> return (data | ((data & 0x0202020202020202ull) << 1)) == data; > >> } > >> > >> +static inline bool kvm_debugctl_valid(u64 data) > >> +{ > >> + /* Bits 2, 3, 4, 5, 13 and [31:16] are reserved */ > >> + return ((data & 0xFFFFFFFFFFFF203Cull) ? false : true); > >> +} > > This should actually be consistent with the constraints in kvm_set_msr_common: > > > > case MSR_IA32_DEBUGCTLMSR: > > if (!data) { > > /* We support the non-activated case already */ > > break; > > } else if (data & ~(DEBUGCTLMSR_LBR | DEBUGCTLMSR_BTF)) { > > /* Values other than LBR and BTF are vendor-specific, > > thus reserved and should throw a #GP */ > > return 1; > > } > > > > Also, as I said earlier... > > > > I'd rather see this built on an interface like: > > > > bool kvm_valid_msr_value(u32 msr_index, u64 value); > > Yes, I forgot to do it. Will send a patch for this... > > > > > Strange that we allow IA32_DEBUGCTL.BTF, since kvm_vcpu_do_singlestep > > ignores it. And vLBR still isn't a thing, is it? > > Yes, DEBUGCTLMSR_LBR isn't used. > Good catch ! > > > > > It's a bit scary to me that we allow any architecturally legal > > IA32_DEBUGCTL bits to be set today. There's probably a CVE in there > > somewhere. > Is it appropriate to disable those two bits as well, then ? IA32_DEBUGCTL.BTF is just broken, and should be fixed. IA32_DEBUGCTL.LBR is probably a long way from actually working, but IIRC, Windows gets cranky if it can't set the bit.
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 46af3a5e9209..0b234e95e0ed 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -2677,6 +2677,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu, !nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4)) return -EINVAL; + if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS) && + !kvm_debugctl_valid(vmcs12->guest_ia32_debugctl)) + return -EINVAL; + if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) && !kvm_pat_valid(vmcs12->guest_ia32_pat)) return -EINVAL; diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h index a470ff0868c5..28ba6d0c359f 100644 --- a/arch/x86/kvm/x86.h +++ b/arch/x86/kvm/x86.h @@ -354,6 +354,12 @@ static inline bool kvm_pat_valid(u64 data) return (data | ((data & 0x0202020202020202ull) << 1)) == data; } +static inline bool kvm_debugctl_valid(u64 data) +{ + /* Bits 2, 3, 4, 5, 13 and [31:16] are reserved */ + return ((data & 0xFFFFFFFFFFFF203Cull) ? false : true); +} + void kvm_load_guest_xcr0(struct kvm_vcpu *vcpu); void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu);
According to section "Checks on Guest Control Registers, Debug Registers, and and MSRs" in Intel SDM vol 3C, the following checks are performed on vmentry of nested guests: If the "load debug controls" VM-entry control is 1, bits reserved in the IA32_DEBUGCTL MSR must be 0 in the field for that register. The first processors to support the virtual-machine extensions supported only the 1-setting of this control and thus performed this check unconditionally. Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> --- arch/x86/kvm/vmx/nested.c | 4 ++++ arch/x86/kvm/x86.h | 6 ++++++ 2 files changed, 10 insertions(+)