[v2] KVM: nVMX: Clear reserved bits of #DB exit qualification
diff mbox series

Message ID 20180921173617.122426-1-jmattson@google.com
State New
Headers show
Series
  • [v2] KVM: nVMX: Clear reserved bits of #DB exit qualification
Related show

Commit Message

Jim Mattson Sept. 21, 2018, 5:36 p.m. UTC
According to volume 3 of the SDM, bits 63:15 and 12:4 of the exit
qualification field for debug exceptions are reserved (cleared to
0). However, the SDM is incorrect about bit 16 (corresponding to
DR6.RTM). This bit should be set if a debug exception (#DB) or a
breakpoint exception (#BP) occurred inside an RTM region while
advanced debugging of RTM transactional regions was enabled. Note that
this is the opposite of DR6.RTM, which "indicates (when clear) that a
debug exception (#DB) or breakpoint exception (#BP) occurred inside an
RTM region while advanced debugging of RTM transactional regions was
enabled."

There is still an issue with stale DR6 bits potentially being
misreported for the current debug exception.  DR6 should not have been
modified before vectoring the #DB exception, and the "new DR6 bits"
should be available somewhere, but it was and they aren't.

Fixes: b96fb439774e1 ("KVM: nVMX: fixes to nested virt interrupt injection")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/include/asm/kvm_host.h | 1 +
 arch/x86/kvm/vmx.c              | 7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

Comments

Sean Christopherson Sept. 21, 2018, 5:57 p.m. UTC | #1
On Fri, Sep 21, 2018 at 10:36:17AM -0700, Jim Mattson wrote:
> According to volume 3 of the SDM, bits 63:15 and 12:4 of the exit
> qualification field for debug exceptions are reserved (cleared to
> 0). However, the SDM is incorrect about bit 16 (corresponding to
> DR6.RTM). This bit should be set if a debug exception (#DB) or a
> breakpoint exception (#BP) occurred inside an RTM region while
> advanced debugging of RTM transactional regions was enabled. Note that
> this is the opposite of DR6.RTM, which "indicates (when clear) that a
> debug exception (#DB) or breakpoint exception (#BP) occurred inside an
> RTM region while advanced debugging of RTM transactional regions was
> enabled."
> 
> There is still an issue with stale DR6 bits potentially being
> misreported for the current debug exception.  DR6 should not have been
> modified before vectoring the #DB exception, and the "new DR6 bits"
> should be available somewhere, but it was and they aren't.
> 
> Fixes: b96fb439774e1 ("KVM: nVMX: fixes to nested virt interrupt injection")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/include/asm/kvm_host.h | 1 +
>  arch/x86/kvm/vmx.c              | 7 +++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 09b2e3e2cf1b..1c09a0d1771f 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -177,6 +177,7 @@ enum {
>  
>  #define DR6_BD		(1 << 13)
>  #define DR6_BS		(1 << 14)
> +#define DR6_BT		(1 << 15)
>  #define DR6_RTM		(1 << 16)
>  #define DR6_FIXED_1	0xfffe0ff0
>  #define DR6_INIT	0xffff0ff0
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 06412ba46aa3..13e41e12945b 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -3297,10 +3297,13 @@ static int nested_vmx_check_exception(struct kvm_vcpu *vcpu, unsigned long *exit
>  		}
>  	} else {
>  		if (vmcs12->exception_bitmap & (1u << nr)) {
> -			if (nr == DB_VECTOR)
> +			if (nr == DB_VECTOR) {
>  				*exit_qual = vcpu->arch.dr6;
> -			else
> +				*exit_qual &= ~(DR6_FIXED_1 | DR6_BT);
> +				*exit_qual ^= DR6_RTM;

Is there any chance that DR6_RTM could be cleared in arch.dr6 without
HLE/RTM being exposed to the guest?  I.e. should we explicitly clear
DR6_RTM if !HLE && !RTM?

> +			} else {
>  				*exit_qual = 0;
> +			}
>  			return 1;
>  		}
>  	}
> -- 
> 2.19.0.444.g18242da7ef-goog
>
Jim Mattson Sept. 21, 2018, 5:59 p.m. UTC | #2
On Fri, Sep 21, 2018 at 10:57 AM, Sean Christopherson
<sean.j.christopherson@intel.com> wrote:

> Is there any chance that DR6_RTM could be cleared in arch.dr6 without
> HLE/RTM being exposed to the guest?  I.e. should we explicitly clear
> DR6_RTM if !HLE && !RTM?

That would be a bug. Section 17.2.3 of the SDM (volume 3) says: "This
bit is always 1 if the processor does not support RTM."
Sean Christopherson Sept. 21, 2018, 6:51 p.m. UTC | #3
On Fri, Sep 21, 2018 at 10:59:36AM -0700, Jim Mattson wrote:
> On Fri, Sep 21, 2018 at 10:57 AM, Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> 
> > Is there any chance that DR6_RTM could be cleared in arch.dr6 without
> > HLE/RTM being exposed to the guest?  I.e. should we explicitly clear
> > DR6_RTM if !HLE && !RTM?
> 
> That would be a bug. Section 17.2.3 of the SDM (volume 3) says: "This
> bit is always 1 if the processor does not support RTM."

My question was more from a software perspective, e.g. it looks like
arch.dr6.RTM can be cleared via kvm_vcpu_ioctl_x86_set_debugregs()
and a #DB injected via kvm_arch_vcpu_ioctl_set_guest_debug().  Not
saying such behavior wouldn't be a bug elsewhere, just wondering if
we should be paranoid here.
Jim Mattson Sept. 21, 2018, 7:03 p.m. UTC | #4
On Fri, Sep 21, 2018 at 11:51 AM, Sean Christopherson
<sean.j.christopherson@intel.com> wrote:

> My question was more from a software perspective, e.g. it looks like
> arch.dr6.RTM can be cleared via kvm_vcpu_ioctl_x86_set_debugregs()
> and a #DB injected via kvm_arch_vcpu_ioctl_set_guest_debug().  Not
> saying such behavior wouldn't be a bug elsewhere, just wondering if
> we should be paranoid here.

My main concern with this code is that stale DR6 bits leak into the
exit qualification for the current #DB exception intercept. Possible
misreporting of bit 16 based on faulty userspace emulation of the vCPU
state seems like small potatoes.
Sean Christopherson Sept. 21, 2018, 9:57 p.m. UTC | #5
On Fri, Sep 21, 2018 at 10:36:17AM -0700, Jim Mattson wrote:
> According to volume 3 of the SDM, bits 63:15 and 12:4 of the exit
> qualification field for debug exceptions are reserved (cleared to
> 0). However, the SDM is incorrect about bit 16 (corresponding to
> DR6.RTM). This bit should be set if a debug exception (#DB) or a
> breakpoint exception (#BP) occurred inside an RTM region while
> advanced debugging of RTM transactional regions was enabled. Note that
> this is the opposite of DR6.RTM, which "indicates (when clear) that a
> debug exception (#DB) or breakpoint exception (#BP) occurred inside an
> RTM region while advanced debugging of RTM transactional regions was
> enabled."
> 
> There is still an issue with stale DR6 bits potentially being
> misreported for the current debug exception.  DR6 should not have been
> modified before vectoring the #DB exception, and the "new DR6 bits"
> should be available somewhere, but it was and they aren't.
> 
> Fixes: b96fb439774e1 ("KVM: nVMX: fixes to nested virt interrupt injection")
> Signed-off-by: Jim Mattson <jmattson@google.com>

Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
Paolo Bonzini Oct. 1, 2018, 1:24 p.m. UTC | #6
On 21/09/2018 23:57, Sean Christopherson wrote:
> On Fri, Sep 21, 2018 at 10:36:17AM -0700, Jim Mattson wrote:
>> According to volume 3 of the SDM, bits 63:15 and 12:4 of the exit
>> qualification field for debug exceptions are reserved (cleared to
>> 0). However, the SDM is incorrect about bit 16 (corresponding to
>> DR6.RTM). This bit should be set if a debug exception (#DB) or a
>> breakpoint exception (#BP) occurred inside an RTM region while
>> advanced debugging of RTM transactional regions was enabled. Note that
>> this is the opposite of DR6.RTM, which "indicates (when clear) that a
>> debug exception (#DB) or breakpoint exception (#BP) occurred inside an
>> RTM region while advanced debugging of RTM transactional regions was
>> enabled."
>>
>> There is still an issue with stale DR6 bits potentially being
>> misreported for the current debug exception.  DR6 should not have been
>> modified before vectoring the #DB exception, and the "new DR6 bits"
>> should be available somewhere, but it was and they aren't.
>>
>> Fixes: b96fb439774e1 ("KVM: nVMX: fixes to nested virt interrupt injection")
>> Signed-off-by: Jim Mattson <jmattson@google.com>
> 
> Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 

Queued, thanks.

Paolo

Patch
diff mbox series

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 09b2e3e2cf1b..1c09a0d1771f 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -177,6 +177,7 @@  enum {
 
 #define DR6_BD		(1 << 13)
 #define DR6_BS		(1 << 14)
+#define DR6_BT		(1 << 15)
 #define DR6_RTM		(1 << 16)
 #define DR6_FIXED_1	0xfffe0ff0
 #define DR6_INIT	0xffff0ff0
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 06412ba46aa3..13e41e12945b 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3297,10 +3297,13 @@  static int nested_vmx_check_exception(struct kvm_vcpu *vcpu, unsigned long *exit
 		}
 	} else {
 		if (vmcs12->exception_bitmap & (1u << nr)) {
-			if (nr == DB_VECTOR)
+			if (nr == DB_VECTOR) {
 				*exit_qual = vcpu->arch.dr6;
-			else
+				*exit_qual &= ~(DR6_FIXED_1 | DR6_BT);
+				*exit_qual ^= DR6_RTM;
+			} else {
 				*exit_qual = 0;
+			}
 			return 1;
 		}
 	}