Re: [RFC] KVM: SVM: do not drop VMCB CPL to 0 if SS is not present

[Roman Penyaev]

On Tue, May 30, 2017 at 5:13 PM, Paolo Bonzini <pbonzini@xxxxxxxxxx> 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 <avi@xxxxxxxxxxxx>
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 <kwolf@xxxxxxxxxx>
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 <pbonzini@xxxxxxxxxx>
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