From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Lutomirski Subject: Re: [PATCH] kvm: x86: vmx: add checks on guest RIP Date: Wed, 03 Dec 2014 14:56:19 -0800 Message-ID: <547F9513.3090600@amacapital.net> References: <20141129152726.GA25370@gnote> <547C9700.6020407@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit To: Paolo Bonzini , Eugene Korenevsky , kvm@vger.kernel.org Return-path: Received: from mail-pa0-f51.google.com ([209.85.220.51]:45961 "EHLO mail-pa0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751153AbaLCW4Y (ORCPT ); Wed, 3 Dec 2014 17:56:24 -0500 Received: by mail-pa0-f51.google.com with SMTP id ey11so16550365pad.24 for ; Wed, 03 Dec 2014 14:56:24 -0800 (PST) In-Reply-To: <547C9700.6020407@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 12/01/2014 08:27 AM, Paolo Bonzini wrote: > > > On 29/11/2014 16:27, Eugene Korenevsky wrote: >> Signed-off-by: Eugene Korenevsky >> --- >> >> 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >