On 12/01/2014 08:27 AM, Paolo Bonzini wrote: > > > On 29/11/2014 16:27, Eugene Korenevsky wrote: >> Signed-off-by: Eugene Korenevsky <ekorenevsky@xxxxxxxxx> >> --- >> >> Notes: >> This patch adds checks on Guest RIP specified in Intel Software Developer Manual. >> >> The following checks are performed on processors that support Intel 64 architecture: >> - Bits 63:32 must be 0 if the "IA-32e mode guest" VM-entry control is 0 or if >> the L bit (bit 13) in the access-rights field for CS is 0. >> - If the processor supports N < 64 linear-address bits, bits 63:N must be identical >> if the "IA-32e mode guest" VM-entry control is 1 and the L bit in the access-rights >> field for CS is 1. (No check applies if the processor supports 64 linear-address >> bits.) >> >> arch/x86/kvm/vmx.c | 27 ++++++++++++++++++++++++++- >> 1 file changed, 26 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index 6a951d8..e2da83b 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -3828,6 +3828,28 @@ static bool cs_ss_rpl_check(struct kvm_vcpu *vcpu) >> (ss.selector & SELECTOR_RPL_MASK)); >> } >> >> +#ifdef CONFIG_X86_64 >> +static bool rip_valid(struct kvm_vcpu *vcpu) >> +{ >> + unsigned long rip; >> + struct kvm_segment cs; >> + bool longmode; >> + >> + /* RIP must be canonical in long mode >> + * Bits 63:32 of RIP must be zero in other processor modes */ >> + longmode = false; >> + if (vm_entry_controls_get(to_vmx(vcpu)) & VM_ENTRY_IA32E_MODE) { >> + vmx_get_segment(vcpu, &cs, VCPU_SREG_CS); >> + longmode = (cs.l != 0); >> + } >> + rip = kvm_register_read(vcpu, VCPU_REGS_RIP); >> + if (longmode) >> + return !is_noncanonical_address(rip); > > This check is off by one. It is checking bits 63:47 instead of bits > 63:48 (this quirk is intentionally part of the specification, so that > you can reenter a guest at 0x800000000000 after e.g. a VMCALL vmexit and > cause a general protection fault). Seriously? Intel did that for vmcall but not sysret? > > However, I am not sure how this can occur. A #GP should have been > injected as part of the instruction that caused RIP to become invalid. > Perhaps you should check in nested_vmx_run instead? For syscall/sysret, at least, if you put a syscall at the highest possible non-negative canonical address, the sysret will fault on the way back. --Andy > > Paolo > >> + else >> + return (rip >> 32) == 0; >> +} >> +#endif >> + >> /* >> * Check if guest state is valid. Returns true if valid, false if >> * not. >> @@ -3873,8 +3895,11 @@ static bool guest_state_valid(struct kvm_vcpu *vcpu) >> if (!ldtr_valid(vcpu)) >> return false; >> } >> +#ifdef CONFIG_X86_64 >> + if (!rip_valid(vcpu)) >> + return false; >> +#endif >> /* TODO: >> - * - Add checks on RIP >> * - Add checks on RFLAGS >> */ >> >> > > My > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html