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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 30 May 2017 16:47:04 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Roman Penyaev <roman.penyaev@...fitbricks.com>,
        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 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;

>    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;
	} else {
	    ...
	}

This would preserve the SS.DPL field.  This should still work fine with
QEMU commit 4cae9c9 (the loading side would set lhs->unusable).

Thanks,

Paolo

> 
> Current email is an RFC since for us is not fully clear is it really
> needed to preserve DPL across KVM_SET/GET_SREGS calls when segment
> is unusable.  E.g. there was a commit:
> 
> 4cae9c97967a ("target-i386: kvm: clear unusable segments' flags in migration")
> 
> which in purpose drops all segment flags to zero on QEMU side in order
> to fix guests migration.