Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1653304 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC] KVM: SVM: do not drop VMCB CPL to 0 if SS is not present Paolo Bonzini <pbonzini@redhat.com> - 2017-05-30 17:20 +0200
    Re: [RFC] KVM: SVM: do not drop VMCB CPL to 0 if SS is not present Roman Penyaev <roman.penyaev@profitbricks.com> - 2017-05-30 18:00 +0200
      Re: [RFC] KVM: SVM: do not drop VMCB CPL to 0 if SS is not present Paolo Bonzini <pbonzini@redhat.com> - 2017-05-30 18:10 +0200
        Re: [RFC] KVM: SVM: do not drop VMCB CPL to 0 if SS is not present Gi-Oh Kim <gi-oh.kim@profitbricks.com> - 2017-05-30 18:40 +0200

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

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.

Maybe if 1) in 64-bit mode 2) SS.P=0 3) SS selector != 0, then the CPL
can be taken from SS.RPL?

Thanks,

Paolo

[toc] | [next] | [standalone]

#1653336

On Tue, May 30, 2017 at 5:13 PM, Paolo Bonzini <pbonzini@redhat.com> 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@qumranet.com>
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@redhat.com>
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@redhat.com>
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

[toc] | [prev] | [next] | [standalone]

#1653345

On 30/05/2017 17:58, Roman Penyaev wrote:
> Indeed, what is left is eventually take it from SS.RPL. J.

Ahah! :)  But I only suggested that in specific cases.

> 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".

Yes, exactly.  The symptom was a crash (triple fault) when you kept
interrupting with "info cpus" a guest that repeatedly went to protected
mode and back to real mode.

> 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.

You're right.  So what do you think of the other suggestion (svm.c
doesn't clear attributes for unusable registers, QEMU only clears P for
unusable registers)?

Thanks,

Paolo

[toc] | [prev] | [next] | [standalone]

#1653379

Hi,

I found a code to set unusable flag of segment register incorrectly.
https://lkml.org/lkml/2017/5/30/459
I guess above patch and current discussion could be related.

I guess following sequence could happen.
1. svm_get_segment() sets var->unusable of Stack Segment incorrectly
2. svm_set_segment() clears both of s->attrib and svm->vmcb->save.cpl.

Is it possible scenario?


On Tue, May 30, 2017 at 6:05 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 30/05/2017 17:58, Roman Penyaev wrote:
>> Indeed, what is left is eventually take it from SS.RPL. J.
>
> Ahah! :)  But I only suggested that in specific cases.
>
>> 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".
>
> Yes, exactly.  The symptom was a crash (triple fault) when you kept
> interrupting with "info cpus" a guest that repeatedly went to protected
> mode and back to real mode.
>
>> 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.
>
> You're right.  So what do you think of the other suggestion (svm.c
> doesn't clear attributes for unusable registers, QEMU only clears P for
> unusable registers)?
>
> Thanks,
>
> Paolo



-- 
Best regards,
Gi-Oh Kim
TEL: 0176 2697 8962

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web