| Message ID | 20180921173617.122426-1-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] KVM: nVMX: Clear reserved bits of #DB exit qualification | expand |
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 >
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."
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.
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.
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>
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
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; } }
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(-)