Re: [PATCH 1/4] KVM: nVMX: Check GUEST_DEBUGCTL on vmentry of nested guests

From: Krish Sadhukhan <krish.sadhukhan@oracle.com>
To: Jim Mattson <jmattson@google.com>
Cc: "kvm list" <kvm@vger.kernel.org>,
	"Radim Krčmář" <rkrcmar@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>
Subject: Re: [PATCH 1/4] KVM: nVMX: Check GUEST_DEBUGCTL on vmentry of nested guests
Date: Fri, 30 Aug 2019 16:26:50 -0700	[thread overview]
Message-ID: <e35e7c1f-e5c8-5f98-771e-302cf8dfba7f@oracle.com> (raw)
In-Reply-To: <CALMp9eQxdF5tJLWaWu+0t0NjhSiJfowo1U6MDkjB_zYNRKiyKw@mail.gmail.com>

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 ?