| Message ID | 1402837982-24959-7-git-send-email-namit@cs.technion.ac.il (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Il 15/06/2014 15:13, Nadav Amit ha scritto: > From: Nadav Amit <nadav.amit@gmail.com> > > When the guest sets DR6 and DR7, KVM asserts the high 32-bits are clear, and > otherwise injects a #GP exception. This exception should only be injected only > if running in long-mode. > > Signed-off-by: Nadav Amit <namit@cs.technion.ac.il> > --- > arch/x86/kvm/x86.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 57eac30..71fe841 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -756,6 +756,15 @@ static void kvm_update_dr7(struct kvm_vcpu *vcpu) > vcpu->arch.switch_db_regs |= KVM_DEBUGREG_BP_ENABLED; > } > > +static bool is_64_bit_mode(struct kvm_vcpu *vcpu) > +{ > + int cs_db, cs_l; > + if (!is_long_mode(vcpu)) > + return false; > + kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l); > + return cs_l; > +} > + > static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val) > { > switch (dr) { > @@ -769,7 +778,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val) > return 1; /* #UD */ > /* fall through */ > case 6: > - if (val & 0xffffffff00000000ULL) > + if ((val & 0xffffffff00000000ULL) && is_64_bit_mode(vcpu)) > return -1; /* #GP */ > vcpu->arch.dr6 = (val & DR6_VOLATILE) | DR6_FIXED_1; > kvm_update_dr6(vcpu); > @@ -779,7 +788,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val) > return 1; /* #UD */ > /* fall through */ > default: /* 7 */ > - if (val & 0xffffffff00000000ULL) > + if ((val & 0xffffffff00000000ULL) && is_64_bit_mode(vcpu)) > return -1; /* #GP */ > vcpu->arch.dr7 = (val & DR7_VOLATILE) | DR7_FIXED_1; > kvm_update_dr7(vcpu); > Do you get this if the input register has bit 31 set? Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/16/14, 1:17 PM, Paolo Bonzini wrote: > Il 15/06/2014 15:13, Nadav Amit ha scritto: >> From: Nadav Amit <nadav.amit@gmail.com> >> >> When the guest sets DR6 and DR7, KVM asserts the high 32-bits are >> clear, and >> otherwise injects a #GP exception. This exception should only be >> injected only >> if running in long-mode. >> >> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il> >> --- >> arch/x86/kvm/x86.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 57eac30..71fe841 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -756,6 +756,15 @@ static void kvm_update_dr7(struct kvm_vcpu *vcpu) >> vcpu->arch.switch_db_regs |= KVM_DEBUGREG_BP_ENABLED; >> } >> >> +static bool is_64_bit_mode(struct kvm_vcpu *vcpu) >> +{ >> + int cs_db, cs_l; >> + if (!is_long_mode(vcpu)) >> + return false; >> + kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l); >> + return cs_l; >> +} >> + >> static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long >> val) >> { >> switch (dr) { >> @@ -769,7 +778,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int >> dr, unsigned long val) >> return 1; /* #UD */ >> /* fall through */ >> case 6: >> - if (val & 0xffffffff00000000ULL) >> + if ((val & 0xffffffff00000000ULL) && is_64_bit_mode(vcpu)) >> return -1; /* #GP */ >> vcpu->arch.dr6 = (val & DR6_VOLATILE) | DR6_FIXED_1; >> kvm_update_dr6(vcpu); >> @@ -779,7 +788,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int >> dr, unsigned long val) >> return 1; /* #UD */ >> /* fall through */ >> default: /* 7 */ >> - if (val & 0xffffffff00000000ULL) >> + if ((val & 0xffffffff00000000ULL) && is_64_bit_mode(vcpu)) >> return -1; /* #GP */ >> vcpu->arch.dr7 = (val & DR7_VOLATILE) | DR7_FIXED_1; >> kvm_update_dr7(vcpu); >> > > Do you get this if the input register has bit 31 set? No. To be frank, the scenario may be considered a bit synthetic: the guest assigns a value to a general-purpose register in 64-bit mode, setting the high 32-bits to some non-zero value. Then, later, in 32-bit mode, the guest performs MOV DR instruction. In between the two assignments, the general purpose register is unmodified, so the high 32-bits of the general purpose registers are still set. Note that this scenario does not occur when MOV DR is emulated, but when handle_dr() is called. In this case, the entire 64-bits of the general purpose register used for MOV DR are read, regardless to the execution mode of the guest. Nadav -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 16/06/2014 12:33, Nadav Amit ha scritto: >> >> Do you get this if the input register has bit 31 set? > No. To be frank, the scenario may be considered a bit synthetic: the > guest assigns a value to a general-purpose register in 64-bit mode, > setting the high 32-bits to some non-zero value. Then, later, in 32-bit > mode, the guest performs MOV DR instruction. In between the two > assignments, the general purpose register is unmodified, so the high > 32-bits of the general purpose registers are still set. > > Note that this scenario does not occur when MOV DR is emulated, but when > handle_dr() is called. In this case, the entire 64-bits of the general > purpose register used for MOV DR are read, regardless to the execution > mode of the guest. I wonder if the same bug happens elsewhere. For example, kvm_emulate_hypercall doesn't look at CS.L/CS.DB, which is really a corner case but arguably also a bug. kvm_hv_hypercall instead does it right. Perhaps we need a variant of kvm_register_read that (on 64-bit hosts) checks EFER/CS.L/CS.DB and masks the returned value accordingly. You could call it kvm_register_readl. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/16/14, 2:09 PM, Paolo Bonzini wrote: > Il 16/06/2014 12:33, Nadav Amit ha scritto: >>> >>> Do you get this if the input register has bit 31 set? >> No. To be frank, the scenario may be considered a bit synthetic: the >> guest assigns a value to a general-purpose register in 64-bit mode, >> setting the high 32-bits to some non-zero value. Then, later, in 32-bit >> mode, the guest performs MOV DR instruction. In between the two >> assignments, the general purpose register is unmodified, so the high >> 32-bits of the general purpose registers are still set. >> >> Note that this scenario does not occur when MOV DR is emulated, but when >> handle_dr() is called. In this case, the entire 64-bits of the general >> purpose register used for MOV DR are read, regardless to the execution >> mode of the guest. > > I wonder if the same bug happens elsewhere. For example, > kvm_emulate_hypercall doesn't look at CS.L/CS.DB, which is really a > corner case but arguably also a bug. kvm_hv_hypercall instead does it > right. > > Perhaps we need a variant of kvm_register_read that (on 64-bit hosts) > checks EFER/CS.L/CS.DB and masks the returned value accordingly. You > could call it kvm_register_readl. There are two questions that come in mind: 1. Should we ignore CS.DB? It would make it consistent with kvm_hv_hypercall and handle_dr. I think this is the proper behavior. 2. Reading CS.L once and masking all the registers (i.e., changing the is_long_mode in kvm_emulate_hypercall to is_64_bit_mode) is likely to be more efficient. Regards, Nadav -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 16/06/2014 13:53, Nadav Amit ha scritto: > On 6/16/14, 2:09 PM, Paolo Bonzini wrote: >> Il 16/06/2014 12:33, Nadav Amit ha scritto: >>>> >>>> Do you get this if the input register has bit 31 set? >>> No. To be frank, the scenario may be considered a bit synthetic: the >>> guest assigns a value to a general-purpose register in 64-bit mode, >>> setting the high 32-bits to some non-zero value. Then, later, in 32-bit >>> mode, the guest performs MOV DR instruction. In between the two >>> assignments, the general purpose register is unmodified, so the high >>> 32-bits of the general purpose registers are still set. >>> >>> Note that this scenario does not occur when MOV DR is emulated, but when >>> handle_dr() is called. In this case, the entire 64-bits of the general >>> purpose register used for MOV DR are read, regardless to the execution >>> mode of the guest. >> >> I wonder if the same bug happens elsewhere. For example, >> kvm_emulate_hypercall doesn't look at CS.L/CS.DB, which is really a >> corner case but arguably also a bug. kvm_hv_hypercall instead does it >> right. >> >> Perhaps we need a variant of kvm_register_read that (on 64-bit hosts) >> checks EFER/CS.L/CS.DB and masks the returned value accordingly. You >> could call it kvm_register_readl. > > There are two questions that come in mind: > 1. Should we ignore CS.DB? It would make it consistent with > kvm_hv_hypercall and handle_dr. I think this is the proper behavior. It depends on what you're using it for, but as a start yes. > 2. Reading CS.L once and masking all the registers (i.e., changing the > is_long_mode in kvm_emulate_hypercall to is_64_bit_mode) is likely to be > more efficient. Yes, for the case of kvm_emulate_hypercall. Then you can build kvm_register_readl on top of is_64bit_mode and fix this bug with that function. Did you check that handle_cr is unaffected? Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/16/14, 5:56 PM, Paolo Bonzini wrote: > Il 16/06/2014 13:53, Nadav Amit ha scritto: >> On 6/16/14, 2:09 PM, Paolo Bonzini wrote: >>> Il 16/06/2014 12:33, Nadav Amit ha scritto: >>>>> >>>>> Do you get this if the input register has bit 31 set? >>>> No. To be frank, the scenario may be considered a bit synthetic: the >>>> guest assigns a value to a general-purpose register in 64-bit mode, >>>> setting the high 32-bits to some non-zero value. Then, later, in 32-bit >>>> mode, the guest performs MOV DR instruction. In between the two >>>> assignments, the general purpose register is unmodified, so the high >>>> 32-bits of the general purpose registers are still set. >>>> >>>> Note that this scenario does not occur when MOV DR is emulated, but >>>> when >>>> handle_dr() is called. In this case, the entire 64-bits of the general >>>> purpose register used for MOV DR are read, regardless to the execution >>>> mode of the guest. >>> >>> I wonder if the same bug happens elsewhere. For example, >>> kvm_emulate_hypercall doesn't look at CS.L/CS.DB, which is really a >>> corner case but arguably also a bug. kvm_hv_hypercall instead does it >>> right. >>> >>> Perhaps we need a variant of kvm_register_read that (on 64-bit hosts) >>> checks EFER/CS.L/CS.DB and masks the returned value accordingly. You >>> could call it kvm_register_readl. >> >> There are two questions that come in mind: >> 1. Should we ignore CS.DB? It would make it consistent with >> kvm_hv_hypercall and handle_dr. I think this is the proper behavior. > > It depends on what you're using it for, but as a start yes. > >> 2. Reading CS.L once and masking all the registers (i.e., changing the >> is_long_mode in kvm_emulate_hypercall to is_64_bit_mode) is likely to be >> more efficient. > > Yes, for the case of kvm_emulate_hypercall. Then you can build > kvm_register_readl on top of is_64bit_mode and fix this bug with that > function. Did you check that handle_cr is unaffected? handle_cr is affected, and there are some other instances of affected register reads. Actually, most of the vmx instruction exit handling routines ignore long-mode and cs.l when considering the register operand size. I'll make the necessary changes and resubmit. Nadav -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
This patch-set resolves several emulator bugs. Each fix is independent of the others. The DR6 bug can occur during DR-access exit (regardless to unrestricted mode, MMIO and SPT). Changes in v2: Introduced kvm_register_readl and kvm_register_writel which consider long-mode and cs.l when reading the registers. Fixing the register read to respect 32/64 bit in hypercall handling, CR exit handling and VMX instructions handling. Thanks for re-reviewing the patch Nadav Amit (9): KVM: x86: bit-ops emulation ignores offset on 64-bit KVM: x86: Wrong emulation on 'xadd X, X' KVM: x86: Inter privilage level ret emulation is not implemeneted KVM: x86: emulation of dword cmov on long-mode should clear [63:32] KVM: x86: NOP emulation clears (incorrectly) the high 32-bits of RAX KVM: x86: check DR6/7 high-bits are clear only on long-mode KVM: x86: Hypercall handling does not considers opsize correctly KVM: vmx: handle_cr ignores 32/64-bit mode KVM: vmx: vmx instructions handling does not consider cs.l arch/x86/kvm/emulate.c | 31 ++++++++++++++++++++----------- arch/x86/kvm/vmx.c | 16 ++++++++-------- arch/x86/kvm/x86.c | 11 ++++++----- arch/x86/kvm/x86.h | 27 +++++++++++++++++++++++++++ 4 files changed, 61 insertions(+), 24 deletions(-)
Il 18/06/2014 16:19, Nadav Amit ha scritto: > This patch-set resolves several emulator bugs. Each fix is independent of the > others. The DR6 bug can occur during DR-access exit (regardless to > unrestricted mode, MMIO and SPT). > > > Changes in v2: > > Introduced kvm_register_readl and kvm_register_writel which consider long-mode > and cs.l when reading the registers. > > Fixing the register read to respect 32/64 bit in hypercall handling, CR exit > handling and VMX instructions handling. > > Thanks for re-reviewing the patch > > Nadav Amit (9): > KVM: x86: bit-ops emulation ignores offset on 64-bit > KVM: x86: Wrong emulation on 'xadd X, X' > KVM: x86: Inter privilage level ret emulation is not implemeneted > KVM: x86: emulation of dword cmov on long-mode should clear [63:32] > KVM: x86: NOP emulation clears (incorrectly) the high 32-bits of RAX > KVM: x86: check DR6/7 high-bits are clear only on long-mode > KVM: x86: Hypercall handling does not considers opsize correctly > KVM: vmx: handle_cr ignores 32/64-bit mode > KVM: vmx: vmx instructions handling does not consider cs.l > > arch/x86/kvm/emulate.c | 31 ++++++++++++++++++++----------- > arch/x86/kvm/vmx.c | 16 ++++++++-------- > arch/x86/kvm/x86.c | 11 ++++++----- > arch/x86/kvm/x86.h | 27 +++++++++++++++++++++++++++ > 4 files changed, 61 insertions(+), 24 deletions(-) > Thanks, looks good. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 57eac30..71fe841 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -756,6 +756,15 @@ static void kvm_update_dr7(struct kvm_vcpu *vcpu) vcpu->arch.switch_db_regs |= KVM_DEBUGREG_BP_ENABLED; } +static bool is_64_bit_mode(struct kvm_vcpu *vcpu) +{ + int cs_db, cs_l; + if (!is_long_mode(vcpu)) + return false; + kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l); + return cs_l; +} + static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val) { switch (dr) { @@ -769,7 +778,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val) return 1; /* #UD */ /* fall through */ case 6: - if (val & 0xffffffff00000000ULL) + if ((val & 0xffffffff00000000ULL) && is_64_bit_mode(vcpu)) return -1; /* #GP */ vcpu->arch.dr6 = (val & DR6_VOLATILE) | DR6_FIXED_1; kvm_update_dr6(vcpu); @@ -779,7 +788,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val) return 1; /* #UD */ /* fall through */ default: /* 7 */ - if (val & 0xffffffff00000000ULL) + if ((val & 0xffffffff00000000ULL) && is_64_bit_mode(vcpu)) return -1; /* #GP */ vcpu->arch.dr7 = (val & DR7_VOLATILE) | DR7_FIXED_1; kvm_update_dr7(vcpu);