From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751129AbdE3P7R (ORCPT ); Tue, 30 May 2017 11:59:17 -0400 Received: from mail-it0-f50.google.com ([209.85.214.50]:38637 "EHLO mail-it0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750929AbdE3P7P (ORCPT ); Tue, 30 May 2017 11:59:15 -0400 MIME-Version: 1.0 In-Reply-To: References: From: Roman Penyaev Date: Tue, 30 May 2017 17:58:54 +0200 Message-ID: Subject: Re: [RFC] KVM: SVM: do not drop VMCB CPL to 0 if SS is not present To: Paolo Bonzini Cc: Mikhail Sennikovskii , Gleb Natapov , kvm@vger.kernel.org, linux-kernel@vger.kernel.org, Andy Lutomirski Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 30, 2017 at 5:13 PM, Paolo Bonzini wrote: > > > On 19/05/2017 18:14, Roman Penyaev wrote: >> >> 1. Simple one, KVM SVM side, which makes sure that CPL is not updated >> if segment is unusable: >> >> --- a/arch/x86/kvm/svm.c >> +++ b/arch/x86/kvm/svm.c >> @@ -1549,7 +1549,7 @@ static void svm_set_segment(struct kvm_vcpu *vcpu, >> * forces SS.DPL to 3 on sysret, so we ignore that case; fixing it >> * would entail passing the CPL to userspace and back. >> */ >> - if (seg == VCPU_SREG_SS) >> + if (seg == VCPU_SREG_SS && !var->unusable) >> svm->vmcb->save.cpl = (s->attrib >> >> SVM_SELECTOR_DPL_SHIFT) & 3; > > Based on the discussion between you and Andy, my understanding is that > it would not be enough to ensure that the attributes are preserved > across a roundtrip through KVM_GET_SEGMENT and KVM_SET_SEGMENT. We need > a workaround in the hypervisor if we don't want to pass the CPL to > userspace and back. We just need to decide where to store CPL on the way to userspace and back and unconditionally follow that convention, regardless what we have in unusable or present flags. > Maybe if 1) in 64-bit mode 2) SS.P=0 3) SS selector != 0, then the CPL > can be taken from SS.RPL? Huh, I just want to show the history of changes of CPL value: original, CPL is taken from CS.DPL: ----------------------------------- commit 6aa8b732ca01c3d7a54e93f4d701b8aabbe60fb7 Author: Avi Kivity Date: Sun Dec 10 02:21:36 2006 -0800 + if (seg == VCPU_SREG_CS) + vcpu->svm->vmcb->save.cpl + = (vcpu->svm->vmcb->save.cs.attrib + >> SVM_SELECTOR_DPL_SHIFT) & 3; then use RPL rather than DPL (not so much in description): ----------------------------------- commit ea5e97e8bf1d56a4d9461c39e082b9c31a7be4ff Author: Kevin Wolf Date: Wed Feb 8 14:34:40 2012 +0100 + svm->vmcb->save.cpl = svm->vmcb->save.cs.selector & 0x3; then get CPL from SS.DPL: ----------------------------------- commit ae9fedc793c4d98aa9bb298585b2b9246096ce65 Author: Paolo Bonzini Date: Wed May 14 09:39:49 2014 +0200 + svm->vmcb->save.cpl = (s->attrib >> SVM_SELECTOR_DPL_SHIFT) & 3; Indeed, what is left is eventually take it from SS.RPL. J. But jokes aside, with your last patch you seems fixed a race problem when "CS.RPL is not equal to the CPL in the few instructions between setting CR0.PE and reloading CS". You also touched svm_get_segment() which does the following: case VCPU_SREG_SS: ... var->dpl = to_svm(vcpu)->vmcb->save.cpl; So even CPU returned the following state: ss = { selector = 0x2b, attrib = 0x400, limit = 0xffffffff, base = 0x0 }, cpl = 0x3 We will have CPL in var->dpl, and it seems ok. All we need is not to lose it on the way kernel->userspace->kernel. -- Roman