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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 30 May 2017 19:35:16 +0200
From:   Roman Penyaev <roman.penyaev@...fitbricks.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Mikhail Sennikovskii <mikhail.sennikovskii@...fitbricks.com>,
        Gleb Natapov <gleb@...nel.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC] KVM: SVM: do not drop VMCB CPL to 0 if SS is not present

On Tue, May 30, 2017 at 4:47 PM, Paolo Bonzini <pbonzini@...hat.com> wrote:
>
>
> On 19/05/2017 18:14, Roman Penyaev wrote:
>> 2. A bit complicated, which makes sure the CPL field is preserved across
>>    KVM_GET/SET_SREGS calls and makes svm_set_segment() and svm_get_segment()
>>    functionality symmethric:
>
> I think I prefer this solution.
>
>>    KVM SVM side:
>>    -------------
>>
>>    --- a/arch/x86/kvm/svm.c
>>    +++ b/arch/x86/kvm/svm.c
>>    @@ -1999,7 +1999,7 @@ static void svm_set_segment(struct kvm_vcpu *vcpu,
>>             * would entail passing the CPL to userspace and back.
>>             */
>>            if (seg == VCPU_SREG_SS)
>>    -               svm->vmcb->save.cpl = (s->attrib >>
>> SVM_SELECTOR_DPL_SHIFT) & 3;
>>    +               svm->vmcb->save.cpl = (var->dpl & 3);
>>
>>            mark_dirty(svm->vmcb, VMCB_SEG);
>>    }
>
> I wonder why svm_set_segment is setting s->attrib = 0 at all.  The
> manual only mentions checking P=0.  What about something like:
>
>         s->base = var->base;
>         s->limit = var->limit;
>         s->selector = var->selector;
>         s->attrib = (var->type & SVM_SELECTOR_TYPE_MASK);
>         s->attrib |= (var->s & 1) << SVM_SELECTOR_S_SHIFT;
>         s->attrib |= (var->dpl & 3) << SVM_SELECTOR_DPL_SHIFT;
>         s->attrib |= (var->present && !var->unusable) << SVM_SELECTOR_P_SHIFT;
>         s->attrib |= (var->avl & 1) << SVM_SELECTOR_AVL_SHIFT;
>         s->attrib |= (var->l & 1) << SVM_SELECTOR_L_SHIFT;
>         s->attrib |= (var->db & 1) << SVM_SELECTOR_DB_SHIFT;
>         s->attrib |= (var->g & 1) << SVM_SELECTOR_G_SHIFT;

Do we care about compatibility issues?  I mean can any old qemu send
us "garbage" in other members of 'var' structure if 'var->unused' == 1 ?

Oh, it seems we require one more field in 'struct kvm_segment' for CPL.

>>    QEMU side:
>>    ----------
>>
>>    --- a/target/i386/kvm.c
>>    +++ b/target/i386/kvm.c
>>    @@ -1979,6 +1979,8 @@ static int kvm_get_sregs(X86CPU *cpu)
>>         get_seg(&env->segs[R_FS], &sregs.fs);
>>         get_seg(&env->segs[R_GS], &sregs.gs);
>>         get_seg(&env->segs[R_SS], &sregs.ss);
>>    +    if (sregs.ss.unusable)
>>    +        env->segs[R_SS].flags |= sregs.ss.dpl << DESC_DPL_SHIFT;
>>
>>         get_seg(&env->tr, &sregs.tr);
>>         get_seg(&env->ldt, &sregs.ldt);
>
> I think what QEMU should do is, in get_seg
>
>         if (rhs->unusable) {
>             lhs->flags &= ~DESC_P_MASK;
>
> This would preserve the SS.DPL field.  This should still work fine with
> QEMU commit 4cae9c9 (the loading side would set lhs->unusable).

Indeed, it will preserve the *old* SS.DPL field, but will not take the *new*
one from kvm side.  And what if extend get_seg() with additional 'segtype'
argument:

static void get_seg(SegmentCache *lhs, const struct kvm_segment *rhs,
int segtype)
{
...
         if (rhs->unusable) {
             /* Clear P and DPL bits */
             lhs->flags &= ~(DESC_P_MASK | (3 << DESC_DPL_SHIFT));
             if (segtype == R_SS)
                /* Set DPL */
                lhs->flags |= rhs->dpl << DESC_DPL_SHIFT;
         }

Then we always keep convention and keep dpl alive along the way U->K->U to
restore it as cpl.

--
Roman